Skip to contentSkip to conversation

do not use sg if not properly supported by usb controller

PATCH RESEND Applied [email protected] 4 participants 19 messages Feb 12, 2019 – Feb 18, 2019

Subsystems (2)
Arm MediaTek MediaTek MT76 wireless LAN driver
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

cover do not use sg if not properly supported by usb controller

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.
This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+

Changes since RFC:
- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/

I am resending the series since the first attempt seems to be rejected by the ML

Lorenzo Bianconi (4):
mt76: usb: move mt76u_check_sg in usb.c
mt76: usb: do not use sg buffers for mcu messages
mt76: usb: use a linear buffer for tx/rx datapath if sg is not
supported
mt76: usb: introduce disable_usb_sg parameter

 drivers/net/wireless/mediatek/mt76/mt76.h     |  14 +-
 .../net/wireless/mediatek/mt76/mt76x0/usb.c   |   2 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |   3 +-
 .../wireless/mediatek/mt76/mt76x2/usb_init.c  |   2 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 133 +++++++++++++-----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |   5 +-
 6 files changed, 105 insertions(+), 54 deletions(-)
Signature · 1 line
2.20.1
author maintainer
#1 lore raw

PATCH RESEND 1/4 mt76: usb: move mt76u_check_sg in usb.c

From: Lorenzo Bianconi <[email protected]>

Move mt76u_check_sg routine in usb.c and introduce sg_en variable in mt76_usb in order to check if scatter-gather is supported by mt76u layer

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt76.h          | 11 +----------
 drivers/net/wireless/mediatek/mt76/mt76x0/usb.c    |  2 +-
 .../net/wireless/mediatek/mt76/mt76x2/usb_init.c   |  2 +-
 drivers/net/wireless/mediatek/mt76/usb.c           | 14 +++++++++++++-
 4 files changed, 16 insertions(+), 13 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76.h +1 -10
index 13f6febc9b0f..0eb9152c5d18 100644
@@ -379,6 +379,7 @@ struct mt76_usb {
 	u16 out_max_packet;
 	u8 in_ep[__MT_EP_IN_MAX];
 	u16 in_max_packet;
+	bool sg_en;
 
 	struct mt76u_mcu {
 		struct mutex mutex;

@@ -726,16 +727,6 @@ static inline u8 q2ep(u8 qid)
 	return qid + 1;
 }
 
-static inline bool mt76u_check_sg(struct mt76_dev *dev)
-{
-	struct usb_interface *intf = to_usb_interface(dev->dev);
-	struct usb_device *udev = interface_to_usbdev(intf);
-
-	return (udev->bus->sg_tablesize > 0 &&
-		(udev->bus->no_sg_constraint ||
-		 udev->speed == USB_SPEED_WIRELESS));
-}
-
 static inline int
 mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int timeout)
 {
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
index 2f259bc0ad9e..da9d05f6074d 100644
@@ -206,7 +206,7 @@ static int mt76x0u_register_device(struct mt76x02_dev *dev)
 		goto out_err;
 
 	/* check hw sg support in order to enable AMSDU */
-	if (mt76u_check_sg(&dev->mt76))
+	if (dev->mt76.usb.sg_en)
 		hw->max_tx_fragments = MT_SG_MAX_SIZE;
 	else
 		hw->max_tx_fragments = 1;
drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c +1 -1
index 006e430e374e..090aaf71b3ef 100644
@@ -228,7 +228,7 @@ int mt76x2u_register_device(struct mt76x02_dev *dev)
 		goto fail;
 
 	/* check hw sg support in order to enable AMSDU */
-	if (mt76u_check_sg(&dev->mt76))
+	if (dev->mt76.usb.sg_en)
 		hw->max_tx_fragments = MT_SG_MAX_SIZE;
 	else
 		hw->max_tx_fragments = 1;
drivers/net/wireless/mediatek/mt76/usb.c +13 -1
index c7db8a9f6acc..829128a07701 100644
@@ -241,6 +241,16 @@ mt76u_rd_rp(struct mt76_dev *dev, u32 base,
 		return mt76u_req_rd_rp(dev, base, data, n);
 }
 
+static bool mt76u_check_sg(struct mt76_dev *dev)
+{
+	struct usb_interface *intf = to_usb_interface(dev->dev);
+	struct usb_device *udev = interface_to_usbdev(intf);
+
+	return (udev->bus->sg_tablesize > 0 &&
+		(udev->bus->no_sg_constraint ||
+		 udev->speed == USB_SPEED_WIRELESS));
+}
+
 static int
 mt76u_set_endpoints(struct usb_interface *intf,
 		    struct mt76_usb *usb)

@@ -536,7 +546,7 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 	if (!q->entry)
 		return -ENOMEM;
 
-	if (mt76u_check_sg(dev)) {
+	if (dev->usb.sg_en) {
 		q->buf_size = MT_RX_BUF_SIZE;
 		nsgs = MT_SG_MAX_SIZE;
 	} else {

@@ -881,6 +891,8 @@ int mt76u_init(struct mt76_dev *dev,
 	dev->bus = &mt76u_ops;
 	dev->queue_ops = &usb_queue_ops;
 
+	usb->sg_en = mt76u_check_sg(dev);
+
 	return mt76u_set_endpoints(intf, usb);
 }
 EXPORT_SYMBOL_GPL(mt76u_init);
Signature · 1 line
2.20.1
author maintainer
#2 lore raw

PATCH RESEND 2/4 mt76: usb: do not use sg buffers for mcu messages

In reply to [email protected]

From: Lorenzo Bianconi <[email protected]>

Do not use scatter-gather buffers for mcu commands. Introduce mt76u_buf_alloc and mt76u_buf_alloc_sg routines.

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt76.h     |  3 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |  3 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 38 +++++++++++++++----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |  5 +--
 4 files changed, 36 insertions(+), 13 deletions(-)
drivers/net/wireless/mediatek/mt76/mt76.h +2 -1
index 0eb9152c5d18..f55dc621e060 100644
@@ -87,6 +87,7 @@ struct mt76u_buf {
 	struct mt76_dev *dev;
 	struct urb *urb;
 	size_t len;
+	void *buf;
 	bool done;
 };
 

@@ -748,7 +749,7 @@ void mt76u_single_wr(struct mt76_dev *dev, const u8 req,
 int mt76u_init(struct mt76_dev *dev, struct usb_interface *intf);
 void mt76u_deinit(struct mt76_dev *dev);
 int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
-		    int nsgs, int len, int sglen, gfp_t gfp);
+		    int len, int data_len, gfp_t gfp);
 void mt76u_buf_free(struct mt76u_buf *buf);
 int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
 		     struct mt76u_buf *buf, gfp_t gfp,
drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c +1 -2
index 64c777298cee..e469e383cb88 100644
@@ -63,9 +63,9 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
 	struct mt76_usb *usb = &dev->usb;
 	struct mt76u_buf *buf = &usb->mcu.res;
 	struct urb *urb = buf->urb;
+	u8 *data = buf->buf;
 	int i, ret;
 	u32 rxfce;
-	u8 *data;
 
 	for (i = 0; i < 5; i++) {
 		if (!wait_for_completion_timeout(&usb->mcu.cmpl,

@@ -75,7 +75,6 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
 		if (urb->status)
 			return -EIO;
 
-		data = sg_virt(&urb->sg[0]);
 		if (usb->mcu.rp)
 			mt76x02u_multiple_mcu_reads(dev, data + 4,
 						    urb->actual_length - 8);
drivers/net/wireless/mediatek/mt76/usb.c +31 -7
index 829128a07701..66c9451cb6f3 100644
@@ -319,8 +319,9 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 	return i ? : -ENOMEM;
 }
 
-int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
-		    int nsgs, int len, int sglen, gfp_t gfp)
+static int
+mt76u_buf_alloc_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
+		   int nsgs, int len, int sglen, gfp_t gfp)
 {
 	buf->urb = usb_alloc_urb(0, gfp);
 	if (!buf->urb)

@@ -337,6 +338,25 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
 }
 
+int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
+		    int len, int data_len, gfp_t gfp)
+{
+	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
+
+	buf->urb = usb_alloc_urb(0, gfp);
+	if (!buf->urb)
+		return -ENOMEM;
+
+	buf->buf = page_frag_alloc(&q->rx_page, len, gfp);
+	if (!buf->buf)
+		return -ENOMEM;
+
+	buf->len = data_len;
+	buf->dev = dev;
+
+	return 0;
+}
+
 void mt76u_buf_free(struct mt76u_buf *buf)
 {
 	struct urb *urb = buf->urb;

@@ -350,6 +370,9 @@ void mt76u_buf_free(struct mt76u_buf *buf)
 
 		skb_free_frag(sg_virt(sg));
 	}
+	if (buf->buf)
+		skb_free_frag(buf->buf);
+
 	usb_free_urb(buf->urb);
 }
 EXPORT_SYMBOL_GPL(mt76u_buf_free);

@@ -360,6 +383,7 @@ int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
 {
 	struct usb_interface *intf = to_usb_interface(dev->dev);
 	struct usb_device *udev = interface_to_usbdev(intf);
+	u8 *data = buf->urb->num_sgs ? NULL : buf->buf;
 	unsigned int pipe;
 
 	if (dir == USB_DIR_IN)

@@ -367,7 +391,7 @@ int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
 	else
 		pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[index]);
 
-	usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, buf->len,
+	usb_fill_bulk_urb(buf->urb, udev, pipe, data, buf->len,
 			  complete_fn, context);
 	trace_submit_urb(dev, buf->urb);
 

@@ -556,10 +580,10 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 
 	q->ndesc = MT_NUM_RX_ENTRIES;
 	for (i = 0; i < q->ndesc; i++) {
-		err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
-				      nsgs, q->buf_size,
-				      SKB_WITH_OVERHEAD(q->buf_size),
-				      GFP_KERNEL);
+		err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
+					 nsgs, q->buf_size,
+					 SKB_WITH_OVERHEAD(q->buf_size),
+					 GFP_KERNEL);
 		if (err < 0)
 			return err;
 	}
drivers/net/wireless/mediatek/mt76/usb_mcu.c +2 -3
index 9527e1216f3d..72c8607da4b4 100644
@@ -29,9 +29,8 @@ int mt76u_mcu_init_rx(struct mt76_dev *dev)
 	struct mt76_usb *usb = &dev->usb;
 	int err;
 
-	err = mt76u_buf_alloc(dev, &usb->mcu.res, 1,
-			      MCU_RESP_URB_SIZE, MCU_RESP_URB_SIZE,
-			      GFP_KERNEL);
+	err = mt76u_buf_alloc(dev, &usb->mcu.res, MCU_RESP_URB_SIZE,
+			      MCU_RESP_URB_SIZE, GFP_KERNEL);
 	if (err < 0)
 		return err;
Signature · 1 line
2.20.1
author maintainer
#3 lore raw

PATCH RESEND 3/4 mt76: usb: use a linear buffer for tx/rx datapath if sg is not supported

In reply to [email protected]

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 87 +++++++++++++++---------
 1 file changed, 54 insertions(+), 33 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +54 -33
index 66c9451cb6f3..7e7da6b49a88 100644
@@ -432,10 +432,11 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
 }
 
 static int
-mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
+mt76u_process_rx_entry(struct mt76_dev *dev, struct mt76u_buf *buf)
 {
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
-	u8 *data = sg_virt(&urb->sg[0]);
+	struct urb *urb = buf->urb;
+	u8 *data = urb->num_sgs ? sg_virt(&urb->sg[0]) : buf->buf;
 	int data_len, len, nsgs = 1;
 	struct sk_buff *skb;
 

@@ -446,7 +447,8 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
 	if (len < 0)
 		return 0;
 
-	data_len = min_t(int, len, urb->sg[0].length - MT_DMA_HDR_LEN);
+	data_len = urb->num_sgs ? urb->sg[0].length : buf->len;
+	data_len = min_t(int, len, data_len - MT_DMA_HDR_LEN);
 	if (MT_DMA_HDR_LEN + data_len > SKB_WITH_OVERHEAD(q->buf_size))
 		return 0;
 

@@ -458,7 +460,7 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
 	__skb_put(skb, data_len);
 	len -= data_len;
 
-	while (len > 0) {
+	while (len > 0 && urb->num_sgs) {
 		data_len = min_t(int, len, urb->sg[nsgs].length);
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
 				sg_page(&urb->sg[nsgs]),

@@ -504,12 +506,26 @@ static void mt76u_complete_rx(struct urb *urb)
 	spin_unlock_irqrestore(&q->lock, flags);
 }
 
+static int
+mt76u_refill_rx(struct mt76_dev *dev, struct mt76_queue *q,
+		struct mt76u_buf *buf, int nsgs)
+{
+	if (dev->usb.sg_en) {
+		return mt76u_fill_rx_sg(dev, buf, nsgs, q->buf_size,
+					SKB_WITH_OVERHEAD(q->buf_size));
+	} else {
+		buf->buf = page_frag_alloc(&q->rx_page, q->buf_size,
+					   GFP_ATOMIC);
+		return buf->buf ? 0 : -ENOMEM;
+	}
+}
+
 static void mt76u_rx_tasklet(unsigned long data)
 {
 	struct mt76_dev *dev = (struct mt76_dev *)data;
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
-	int err, nsgs, buf_len = q->buf_size;
 	struct mt76u_buf *buf;
+	int err, count;
 
 	rcu_read_lock();
 

@@ -518,11 +534,9 @@ static void mt76u_rx_tasklet(unsigned long data)
 		if (!buf)
 			break;
 
-		nsgs = mt76u_process_rx_entry(dev, buf->urb);
-		if (nsgs > 0) {
-			err = mt76u_fill_rx_sg(dev, buf, nsgs,
-					       buf_len,
-					       SKB_WITH_OVERHEAD(buf_len));
+		count = mt76u_process_rx_entry(dev, buf);
+		if (count > 0) {
+			err = mt76u_refill_rx(dev, q, buf, count);
 			if (err < 0)
 				break;
 		}

@@ -560,7 +574,7 @@ EXPORT_SYMBOL_GPL(mt76u_submit_rx_buffers);
 static int mt76u_alloc_rx(struct mt76_dev *dev)
 {
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
-	int i, err, nsgs;
+	int i, err;
 
 	spin_lock_init(&q->rx_page_lock);
 	spin_lock_init(&q->lock);

@@ -570,20 +584,19 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 	if (!q->entry)
 		return -ENOMEM;
 
-	if (dev->usb.sg_en) {
-		q->buf_size = MT_RX_BUF_SIZE;
-		nsgs = MT_SG_MAX_SIZE;
-	} else {
-		q->buf_size = PAGE_SIZE;
-		nsgs = 1;
-	}
-
+	q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
 	q->ndesc = MT_NUM_RX_ENTRIES;
 	for (i = 0; i < q->ndesc; i++) {
-		err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
-					 nsgs, q->buf_size,
-					 SKB_WITH_OVERHEAD(q->buf_size),
-					 GFP_KERNEL);
+		if (dev->usb.sg_en)
+			err = mt76u_buf_alloc_sg(dev, &q->entry[i].ubuf,
+					MT_SG_MAX_SIZE, q->buf_size,
+					SKB_WITH_OVERHEAD(q->buf_size),
+					GFP_KERNEL);
+		else
+			err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
+					      q->buf_size,
+					      SKB_WITH_OVERHEAD(q->buf_size),
+					      GFP_KERNEL);
 		if (err < 0)
 			return err;
 	}

@@ -731,7 +744,7 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
 {
 	struct usb_interface *intf = to_usb_interface(dev->dev);
 	struct usb_device *udev = interface_to_usbdev(intf);
-	u8 ep = q2ep(q->hw_idx);
+	u8 *data = NULL, ep = q2ep(q->hw_idx);
 	struct mt76u_buf *buf;
 	u16 idx = q->tail;
 	unsigned int pipe;

@@ -748,12 +761,16 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
 	buf = &q->entry[idx].ubuf;
 	buf->done = false;
 
-	err = mt76u_tx_build_sg(skb, buf->urb);
-	if (err < 0)
-		return err;
+	if (dev->usb.sg_en) {
+		err = mt76u_tx_build_sg(skb, buf->urb);
+		if (err < 0)
+			return err;
+	} else {
+		data = skb->data;
+	}
 
 	pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[ep]);
-	usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, skb->len,
+	usb_fill_bulk_urb(buf->urb, udev, pipe, data, skb->len,
 			  mt76u_complete_tx, buf);
 
 	q->tail = (q->tail + 1) % q->ndesc;

@@ -789,10 +806,8 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
 {
 	struct mt76u_buf *buf;
 	struct mt76_queue *q;
-	size_t size;
 	int i, j;
 
-	size = MT_SG_MAX_SIZE * sizeof(struct scatterlist);
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		q = &dev->q_tx[i];
 		spin_lock_init(&q->lock);

@@ -814,9 +829,15 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
 			if (!buf->urb)
 				return -ENOMEM;
 
-			buf->urb->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
-			if (!buf->urb->sg)
-				return -ENOMEM;
+			if (dev->usb.sg_en) {
+				size_t size = MT_SG_MAX_SIZE *
+					      sizeof(struct scatterlist);
+
+				buf->urb->sg = devm_kzalloc(dev->dev, size,
+							    GFP_KERNEL);
+				if (!buf->urb->sg)
+					return -ENOMEM;
+			}
 		}
 	}
 	return 0;
Signature · 1 line
2.20.1
author maintainer
#4 lore raw

PATCH RESEND 4/4 mt76: usb: introduce disable_usb_sg parameter

In reply to [email protected]

From: Lorenzo Bianconi <[email protected]>

Add disable_usb_sg module parameter to disable scatter-gather on demand

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/net/wireless/mediatek/mt76/usb.c +5 -1
index 7e7da6b49a88..78191968b4fa 100644
@@ -22,6 +22,10 @@
 #define MT_VEND_REQ_MAX_RETRY	10
 #define MT_VEND_REQ_TOUT_MS	300
 
+static bool disable_usb_sg;
+module_param_named(disable_usb_sg, disable_usb_sg, bool, 0644);
+MODULE_PARM_DESC(disable_usb_sg, "Disable usb scatter-gather support");
+
 /* should be called with usb_ctrl_mtx locked */
 static int __mt76u_vendor_request(struct mt76_dev *dev, u8 req,
 				  u8 req_type, u16 val, u16 offset,

@@ -246,7 +250,7 @@ static bool mt76u_check_sg(struct mt76_dev *dev)
 	struct usb_interface *intf = to_usb_interface(dev->dev);
 	struct usb_device *udev = interface_to_usbdev(intf);
 
-	return (udev->bus->sg_tablesize > 0 &&
+	return (!disable_usb_sg && udev->bus->sg_tablesize > 0 &&
 		(udev->bus->no_sg_constraint ||
 		 udev->speed == USB_SPEED_WIRELESS));
 }
Signature · 1 line
2.20.1
#5 lore raw
In reply to [email protected]
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Quoted 12 lines

This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+

- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/

Lorenzo Bianconi (4):
mt76: usb: move mt76u_check_sg in usb.c
mt76: usb: do not use sg buffers for mcu messages
mt76: usb: use a linear buffer for tx/rx datapath if sg is not
supported
mt76: usb: introduce disable_usb_sg parameter

Quoted 7 lines
 drivers/net/wireless/mediatek/mt76/mt76.h     |  14 +-
 .../net/wireless/mediatek/mt76/mt76x0/usb.c   |   2 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |   3 +-
 .../wireless/mediatek/mt76/mt76x2/usb_init.c  |   2 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 133 +++++++++++++-----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |   5 +-
 6 files changed, 105 insertions(+), 54 deletions(-)

I really think my approach is simpler. Diffstat from my proposed patch is:

  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
  drivers/net/wireless/mediatek/mt76/usb.c  | 55 ++++++++++++++---------
  2 files changed, 36 insertions(+), 20 deletions(-)

Also this fix bug(s), presumably regression for mt76x0, should be backported.

Stanislaw

#6 lore raw

(repost with corrected Lorenzo email)

Quoted 43 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Quoted 12 lines

This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+

- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/

Lorenzo Bianconi (4):
mt76: usb: move mt76u_check_sg in usb.c
mt76: usb: do not use sg buffers for mcu messages
mt76: usb: use a linear buffer for tx/rx datapath if sg is not
supported
mt76: usb: introduce disable_usb_sg parameter

Quoted 7 lines
 drivers/net/wireless/mediatek/mt76/mt76.h     |  14 +-
 .../net/wireless/mediatek/mt76/mt76x0/usb.c   |   2 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |   3 +-
 .../wireless/mediatek/mt76/mt76x2/usb_init.c  |   2 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 133 +++++++++++++-----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |   5 +-
 6 files changed, 105 insertions(+), 54 deletions(-)

I really think my approach is simpler. Diffstat from my proposed patch is:

  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
  drivers/net/wireless/mediatek/mt76/usb.c  | 55 ++++++++++++++---------
  2 files changed, 36 insertions(+), 20 deletions(-)

Also this fix bug(s), presumably regression for mt76x0, should be backported.

Stanislaw

#7 lore raw
Quoted 15 lines

(repost with corrected Lorenzo email)

Quoted 11 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Hi Stanislaw,

here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue.
Moreover with this approach we avoid some unnecessary operation in the hotpath

Regards,
Lorenzo

Quoted 32 lines
Quoted 32 lines
Quoted 13 lines

This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+

- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/

Lorenzo Bianconi (4):
mt76: usb: move mt76u_check_sg in usb.c
mt76: usb: do not use sg buffers for mcu messages
mt76: usb: use a linear buffer for tx/rx datapath if sg is not
supported
mt76: usb: introduce disable_usb_sg parameter

Quoted 7 lines
 drivers/net/wireless/mediatek/mt76/mt76.h     |  14 +-
 .../net/wireless/mediatek/mt76/mt76x0/usb.c   |   2 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |   3 +-
 .../wireless/mediatek/mt76/mt76x2/usb_init.c  |   2 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 133 +++++++++++++-----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |   5 +-
 6 files changed, 105 insertions(+), 54 deletions(-)

I really think my approach is simpler. Diffstat from my proposed patch is:

  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
  drivers/net/wireless/mediatek/mt76/usb.c  | 55 ++++++++++++++---------
  2 files changed, 36 insertions(+), 20 deletions(-)

Also this fix bug(s), presumably regression for mt76x0, should be backported.

Stanislaw

#8 lore raw
Quoted 13 lines · Lorenzo Bianconi
Quoted 13 lines

(repost with corrected Lorenzo email)

Quoted 9 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

Again. This is not right approach. SG issues should be fixed not workarounded.

Hi Stanislaw,

here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue.

By avoiding SG buffer allocation and configuration which most likely need to be fixed.

Quoted 1 line

Moreover with this approach we avoid some unnecessary operation in the hotpath

What unnecessary operation ?

Stanislaw

#9 lore raw
Quoted 17 lines · Lorenzo Bianconi
Quoted 16 lines
Quoted 15 lines

(repost with corrected Lorenzo email)

Quoted 11 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue.

By avoiding SG buffer allocation and configuration which most likely need to be fixed.

In my series I:
1- set num_sg to 0
2- use transfer_buffer

please correct me if I am wrong but in your solution you did the same since AFAIK PageHighMem is always 0 so you end up setting num_sg to 0 and using transfer_buffer as well. Is my understanding correct?

Quoted 4 lines
Quoted 2 lines

Moreover with this approach we avoid some unnecessary operation in the hotpath

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Regards,
Lorenzo

Quoted 2 lines

Stanislaw

#10 lore raw
Quoted 25 lines · Lorenzo Bianconi
Quoted 17 lines · Lorenzo Bianconi
Quoted 16 lines
Quoted 15 lines

(repost with corrected Lorenzo email)

Quoted 11 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue.

By avoiding SG buffer allocation and configuration which most likely need to be fixed.

Quoted 7 lines

In my series I:
1- set num_sg to 0
2- use transfer_buffer

please correct me if I am wrong but in your solution you did the same since AFAIK PageHighMem is always 0 so you end up setting num_sg to 0 and using transfer_buffer as well. Is my understanding correct?

Yes. But it still using all existing SG allocation and setup code and buffer is tracked in urb->sg[0].

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Moreover with this approach we avoid some unnecessary operation in the hotpath

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

Stanislaw

#11 lore raw
Quoted 30 lines · Lorenzo Bianconi
Quoted 29 lines
Quoted 17 lines · Lorenzo Bianconi
Quoted 16 lines
Quoted 15 lines

(repost with corrected Lorenzo email)

Quoted 11 lines · Stanislaw Gruszka
Quoted 7 lines · On Tue, Feb 12, 2019 at 02:24:47PM +0100, [email protected]…

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.

Again. This is not right approach. SG issues should be fixed not workarounded.

Quoted 7 lines
Quoted 4 lines

Hi Stanislaw,

here we do not use SG, so num_sg is 0 and we use transfer_buffer. I do not see how I am working around the issue.

By avoiding SG buffer allocation and configuration which most likely need to be fixed.

In my series I:
1- set num_sg to 0
2- use transfer_buffer

Quoted 6 lines
Quoted 3 lines

please correct me if I am wrong but in your solution you did the same since AFAIK PageHighMem is always 0 so you end up setting num_sg to 0 and using transfer_buffer as well. Is my understanding correct?

Yes. But it still using all existing SG allocation and setup code and buffer is tracked in urb->sg[0].

both of them are from page_frag_alloc()

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Moreover with this approach we avoid some unnecessary operation in the hotpath

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

Moreover the RFC series has been tested by multiple users and on multiple devices

Regards,
Lorenzo

Quoted 2 lines

Stanislaw

#12 lore raw
Quoted 10 lines · Lorenzo Bianconi
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

I know. Perhaps you could test my patch on rpi ?

Stanislaw

#13 lore raw
Quoted 12 lines · Lorenzo Bianconi
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

Quoted 8 lines

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

I know. Perhaps you could test my patch on rpi ?

sure, I will do it later

Regards,
Lorenzo

Quoted 4 lines

Stanislaw

#14 lore raw
Quoted 16 lines
Quoted 16 lines · Lorenzo Bianconi
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

I know. Perhaps you could test my patch on rpi ?

sure, I will do it later

I confirm that even with Stanislaw's patch the usb dongle is properly working on rpi3+

Regards,
Lorenzo

Quoted 8 lines

Regards,
Lorenzo

Quoted 4 lines

Stanislaw

#15 lore raw
Quoted 19 lines · Lorenzo Bianconi
Quoted 19 lines
Quoted 19 lines · Lorenzo Bianconi
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

Quoted 6 lines
Quoted 3 lines
Quoted 1 line

I know. Perhaps you could test my patch on rpi ?

sure, I will do it later

I confirm that even with Stanislaw's patch the usb dongle is properly working on rpi3+

Thanks for testing. Would be ok to you to post my patch against wireless-drivers tree and cc stable as fix for non-SG usb hosts, drop your set for -next and work for fix for SG issue on AMD IOMMU?

Stanislaw

#16 lore raw
Quoted 22 lines · Stanislaw Gruszka
Quoted 21 lines · Lorenzo Bianconi
Quoted 21 lines
Quoted 20 lines · Lorenzo Bianconi
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

I know. Perhaps you could test my patch on rpi ?

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

sure, I will do it later

I confirm that even with Stanislaw's patch the usb dongle is properly working on rpi3+

Thanks for testing. Would be ok to you to post my patch against wireless-drivers tree and cc stable as fix for non-SG usb hosts, drop your set for -next and work for fix for SG issue on AMD IOMMU?

I agree that your patch works (since it does not use SG I/O :)) but
I think it is more clear (and manageable) to have two separated routines for memory allocation.
Moreover I think that this check has to be done in the control plane instead of the data plane, so I would like to spend some more time in order to see if it is possible to remove some checks in the hot-path

Regards,
Lorenzo

Quoted 2 lines

Stanislaw

#17 lore raw
Quoted 25 lines · Lorenzo Bianconi
Quoted 24 lines · Stanislaw Gruszka
Quoted 23 lines · Lorenzo Bianconi
Quoted 22 lines
Quoted 12 lines · Lorenzo Bianconi
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

What unnecessary operation ?

the ones in mt76u_fill_bulk_urb()

Your patches also add extra operations on hotpath due to urb->num_sgs and dev->usb.sg_en checks.

here I guess you should use mt76u_check_sg() instead of udev->bus->sg_tablesize > 0 so I think you need to precompute it since it is in the hotpath

Quoted 8 lines

It's not necessary. If udev->bus->sg_tablesize > 0 is true and rest of mt76u_check_sg() is not, we will submit urb with urb->num_sgs = 1 to usb host driver, what is fine.

Quoted 2 lines

Moreover the RFC series has been tested by multiple users and on multiple devices

I know. Perhaps you could test my patch on rpi ?

sure, I will do it later

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

I confirm that even with Stanislaw's patch the usb dongle is properly working on rpi3+

Thanks for testing. Would be ok to you to post my patch against wireless-drivers tree and cc stable as fix for non-SG usb hosts, drop your set for -next and work for fix for SG issue on AMD IOMMU?

I agree that your patch works (since it does not use SG I/O :)) but

It still use SG in mt76usb driver (urb->sg, sg_set_page(), sg_init_marker(), etc ...) for all usb host controllers. It just not submit urb->num_sgs > 1 to USB host driver if it does not support SG.

Quoted 2 lines

I think it is more clear (and manageable) to have two separated routines for memory allocation.

Hmm, I disagree on that and don't understand how you consider clearer and manageable design with two separate buffer allocation methods and bunch of extra ->num_sgs and ->sg_en checks, compered to solution with one allocation method and without those checks.

Also some additional cleanups/simplification can be done after applying my patch in mt76usb not possible after applying your set.

Quoted 3 lines

Moreover I think that this check has to be done in the control plane instead of the data plane, so I would like to spend some more time in order to see if it is possible to remove some checks in the hot-path

I'm not sure what you mean by data-pane and control-plane in this context.

I considered to do mt76u_fill_buk_urb:

urb->num_sgs = (buf->num_sgs > 1) ? buf->num_sgs : 0;

Instead of usb->bus->sg_tablesize check. It should work, but it's not what usb_sg_init() does and as pointed by Alan that function always do the right thing, so I just copied code from there.

Beside that I don't think this check in mt76u_fill_buk_urb() affects performance whatsoever.

Stanislaw

maintainer Applied
#18 lore raw
In reply to [email protected]
Quoted 9 lines · On 2019-02-12 14:42, [email protected] wrote:

From: Lorenzo Bianconi <[email protected]>

Use linear fragment and not a single usb scatter-gather buffer in mt76u {tx,rx} datapath if the usb controller has sg data length constraints. Moreover add disable_usb_sg module parameter in order to explicitly disable scatter-gather. SG I/O is not supported by all host drivers and some users have reported sg issues on AMD IOMMU.
This series has been tested on AMD IOMMU cpus/motherboards and on rpi3+

Quoted 6 lines

Changes since RFC:
- rebased on top of 'fix multiple issues in mt76u error path'
https://patchwork.kernel.org/cover/10804919/

I am resending the series since the first attempt seems to be rejected by the ML

Applied, thanks.

- Felix