[RFC 0/4] do not use sg if not properly supported by usb controller

[RFC 0/4] do not use sg if not properly supported by usb controller

[Lorenzo Bianconi]

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. Some users have reported sg issues on AMD IOMMU

Lorenzo Bianconi (4):
  mt76: usb: move mt76u_check_sg in usb.c
  mt76: usb: do not use sg buffer for fw upload
  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  |   9 +-
 .../wireless/mediatek/mt76/mt76x2/usb_init.c  |   2 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 132 +++++++++++++-----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |   5 +-
 6 files changed, 107 insertions(+), 57 deletions(-)

-- 
2.20.1

[RFC 1/4] mt76: usb: move mt76u_check_sg in usb.c

[Lorenzo Bianconi]

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 <lorenzo.bianconi@redhat.com>
---
 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(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 8ef430d8afc4..d31681f90ff2 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -368,6 +368,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;
@@ -704,16 +705,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));
-}
-
 int mt76u_vendor_request(struct mt76_dev *dev, u8 req,
 			 u8 req_type, u16 val, u16 offset,
 			 void *buf, size_t len);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 0e6b43bb4678..2c140f411972 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -196,7 +196,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;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
index 0be3784f44fb..e39bd876b040 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
@@ -256,7 +256,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;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 6a2507524c6c..44e9f5d66326 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -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)
@@ -530,7 +540,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 {
@@ -882,6 +892,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);
-- 
2.20.1

[RFC 2/4] mt76: usb: do not use sg buffer for fw upload

[Lorenzo Bianconi]

Do not use scatter-gather buffers for fw uploading.
Introduce mt76u_buf_alloc and mt76u_buf_alloc_sg routines.
mt76u_buf_alloc will be reused by mt76u for buffer allocation if
scatter-gather is not supported

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h     |  3 +-
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |  9 ++---
 drivers/net/wireless/mediatek/mt76/usb.c      | 37 +++++++++++++++----
 drivers/net/wireless/mediatek/mt76/usb_mcu.c  |  5 +--
 4 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index d31681f90ff2..05ab6672a154 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -86,6 +86,7 @@ struct mt76u_buf {
 	struct mt76_dev *dev;
 	struct urb *urb;
 	size_t len;
+	void *buf;
 	bool done;
 };
 
@@ -713,7 +714,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,
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index 6db789f90269..925aeee56a3e 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -78,9 +78,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,
@@ -90,7 +90,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);
@@ -271,8 +270,8 @@ static int
 __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
 			    const void *fw_data, int len, u32 dst_addr)
 {
-	u8 *data = sg_virt(&buf->urb->sg[0]);
 	DECLARE_COMPLETION_ONSTACK(cmpl);
+	u8 *data = buf->buf;
 	__le32 info;
 	u32 val;
 	int err;
@@ -325,8 +324,8 @@ int mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, const void *data,
 	int err, len, pos = 0, max_len = max_payload - 8;
 	struct mt76u_buf buf;
 
-	err = mt76u_buf_alloc(&dev->mt76, &buf, 1, max_payload, max_payload,
-			      GFP_KERNEL);
+	err = mt76u_buf_alloc(&dev->mt76, &buf, max_payload,
+			      max_payload, GFP_KERNEL);
 	if (err < 0)
 		return err;
 
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 44e9f5d66326..4dc288e86fd1 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -320,7 +320,28 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 }
 
 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)
+{
+	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;
+}
+EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
+
+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)
@@ -336,7 +357,6 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 
 	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
 }
-EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
 
 void mt76u_buf_free(struct mt76u_buf *buf)
 {
@@ -345,6 +365,8 @@ void mt76u_buf_free(struct mt76u_buf *buf)
 
 	for (i = 0; i < urb->num_sgs; i++)
 		skb_free_frag(sg_virt(&urb->sg[i]));
+	if (buf->buf)
+		skb_free_frag(buf->buf);
 	usb_free_urb(buf->urb);
 }
 EXPORT_SYMBOL_GPL(mt76u_buf_free);
@@ -355,6 +377,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)
@@ -362,7 +385,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);
 
@@ -549,10 +572,10 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 	}
 
 	for (i = 0; i < MT_NUM_RX_ENTRIES; 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;
 	}
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 036be4163e69..e14e82a9e7c0 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -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;
 
-- 
2.20.1

[RFC 3/4] mt76: usb: use a linear buffer for tx/rx datapath if sg is not supported

[Lorenzo Bianconi]

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 <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 87 +++++++++++++++---------
 1 file changed, 54 insertions(+), 33 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 4dc288e86fd1..491b17756ccd 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -426,10 +426,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;
 
@@ -440,7 +441,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;
 
@@ -452,7 +454,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]),
@@ -497,12 +499,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();
 
@@ -511,11 +527,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;
 		}
@@ -553,7 +567,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);
@@ -563,19 +577,18 @@ 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;
 	for (i = 0; i < MT_NUM_RX_ENTRIES; 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;
 	}
@@ -724,7 +737,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;
@@ -741,12 +754,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;
@@ -782,10 +799,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);
@@ -807,9 +822,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;
-- 
2.20.1

[RFC 4/4] mt76: usb: introduce disable_usb_sg parameter

[Lorenzo Bianconi]

Add disable_usb_sg module parameter to disable scatter-gather on demand

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 491b17756ccd..0b056f04630f 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -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));
 }
-- 
2.20.1

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Stanislaw Gruszka]

On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote:
> 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. Some users have reported sg issues on AMD IOMMU

Not sure what is the problem , but this patch set look like a workaround
not fix. If this an issue with IOMMU and sg, seems there is something wrong
in sg page mappings eigher on mt76 dirver or IOMMU driver.

If things need to be fixed in mt76 I whould check if page mappings for
sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO
version do not use sg for framgments, so most likely USB don't need it
as well.

Thanks
Stanislaw 

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Lorenzo Bianconi]

> On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote:
> > 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. Some users have reported sg issues on AMD IOMMU

Hi Stanislaw,

> 
> Not sure what is the problem , but this patch set look like a workaround
> not fix. If this an issue with IOMMU and sg, seems there is something wrong
> in sg page mappings eigher on mt76 dirver or IOMMU driver.

The main point here I guess is we do not need sg if fragment number is one (e.g
usb2.0). Moreover this can fix IOMMU reported issues.

@Rosen: could you please try this series enabling IOMMU?

> 
> If things need to be fixed in mt76 I whould check if page mappings for
> sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO
> version do not use sg for framgments, so most likely USB don't need it
> as well.

usb scatter-gather is used to properly support non-linear skbs (A-MSDU,
with usb3.0) since the hw (unlike pci counterpart) does not support it,
so we need it.

Regards,
Lorenzo

> 
> Thanks
> Stanislaw 

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Stanislaw Gruszka]

On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> Hi Stanislaw,

Hi :-)

> > Not sure what is the problem , but this patch set look like a workaround
> > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> 
> The main point here I guess is we do not need sg if fragment number is one (e.g
> usb2.0). Moreover this can fix IOMMU reported issues.

So there if diffrence for USB host driver when we have one usb->sg
sengment and if we just pass the buffer via urb->transfer_buf . I think
most USB host drivers behave the same in such cases. For what USB
hardware/driver this is needed ? Perhaps simpler fix could be done
in USB host driver?

Also I'm not sure for what this new module parameter is needed ?
 
> @Rosen: could you please try this series enabling IOMMU?
> 
> > 
> > If things need to be fixed in mt76 I whould check if page mappings for
> > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO
> > version do not use sg for framgments, so most likely USB don't need it
> > as well.
> 
> usb scatter-gather is used to properly support non-linear skbs (A-MSDU,
> with usb3.0) since the hw (unlike pci counterpart) does not support it,
> so we need it.

Ok, having separe URB for each fragment make no sense since we have
embedded sg structure in URB.

Regards
Stanislaw

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Lorenzo Bianconi]

> On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> > Hi Stanislaw,
> 
> Hi :-)
> 
> > > Not sure what is the problem , but this patch set look like a workaround
> > > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> > 
> > The main point here I guess is we do not need sg if fragment number is one (e.g
> > usb2.0). Moreover this can fix IOMMU reported issues.
> 
> So there if diffrence for USB host driver when we have one usb->sg
> sengment and if we just pass the buffer via urb->transfer_buf . I think
> most USB host drivers behave the same in such cases. For what USB
> hardware/driver this is needed ? Perhaps simpler fix could be done
> in USB host driver?

According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557
single sg urb and urb with a configured transfer_buf are managed in a different way.
Please not I have not received any confirm that this series fixes the reported issue
yet :)

> 
> Also I'm not sure for what this new module parameter is needed ?
>  

This is used to disable sg on usb3.0 since it is enabled by default

> > @Rosen: could you please try this series enabling IOMMU?
> > 
> > > 
> > > If things need to be fixed in mt76 I whould check if page mappings for
> > > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO
> > > version do not use sg for framgments, so most likely USB don't need it
> > > as well.
> > 
> > usb scatter-gather is used to properly support non-linear skbs (A-MSDU,
> > with usb3.0) since the hw (unlike pci counterpart) does not support it,
> > so we need it.
> 
> Ok, having separe URB for each fragment make no sense since we have
> embedded sg structure in URB.

In this case each fragment will be mapped independently I guess

Regards,
Lorenzo

> 
> Regards
> Stanislaw

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Stanislaw Gruszka]

On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote:
> > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> > > Hi Stanislaw,
> > 
> > Hi :-)
> > 
> > > > Not sure what is the problem , but this patch set look like a workaround
> > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > > > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> > > 
> > > The main point here I guess is we do not need sg if fragment number is one (e.g
> > > usb2.0). Moreover this can fix IOMMU reported issues.
> > 
> > So there if diffrence for USB host driver when we have one usb->sg
> > sengment and if we just pass the buffer via urb->transfer_buf . I think
> > most USB host drivers behave the same in such cases. For what USB
> > hardware/driver this is needed ? Perhaps simpler fix could be done
> > in USB host driver?
> 
> According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557
> single sg urb and urb with a configured transfer_buf are managed in a different way.

But this should not make any difference for underlying low level USB
host driver, since we map 1 buffer of the same size, just by using
different routines for that.

> Please not I have not received any confirm that this series fixes the reported issue
> yet :)

What is reported issue ?

Thanks
Stanislaw

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Lorenzo Bianconi]

On Jan 16, Stanislaw Gruszka wrote:
> On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote:
> > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> > > > Hi Stanislaw,
> > > 
> > > Hi :-)
> > > 
> > > > > Not sure what is the problem , but this patch set look like a workaround
> > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> > > > 
> > > > The main point here I guess is we do not need sg if fragment number is one (e.g
> > > > usb2.0). Moreover this can fix IOMMU reported issues.
> > > 
> > > So there if diffrence for USB host driver when we have one usb->sg
> > > sengment and if we just pass the buffer via urb->transfer_buf . I think
> > > most USB host drivers behave the same in such cases. For what USB
> > > hardware/driver this is needed ? Perhaps simpler fix could be done
> > > in USB host driver?
> > 
> > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557
> > single sg urb and urb with a configured transfer_buf are managed in a different way.
> 
> But this should not make any difference for underlying low level USB
> host driver, since we map 1 buffer of the same size, just by using
> different routines for that.

probably amd iommu has some constraints on sg buffer layout respect to intel
one, not sure

> 
> > Please not I have not received any confirm that this series fixes the reported issue
> > yet :)
> 
> What is reported issue ?

https://marc.info/?l=linux-wireless&m=154716096506037&w=2

Regards,
Lorenzo

> 
> Thanks
> Stanislaw

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Stanislaw Gruszka]

On Wed, Jan 16, 2019 at 02:40:46PM +0100, Lorenzo Bianconi wrote:
> On Jan 16, Stanislaw Gruszka wrote:
> > On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote:
> > > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> > > > > Hi Stanislaw,
> > > > 
> > > > Hi :-)
> > > > 
> > > > > > Not sure what is the problem , but this patch set look like a workaround
> > > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> > > > > 
> > > > > The main point here I guess is we do not need sg if fragment number is one (e.g
> > > > > usb2.0). Moreover this can fix IOMMU reported issues.
> > > > 
> > > > So there if diffrence for USB host driver when we have one usb->sg
> > > > sengment and if we just pass the buffer via urb->transfer_buf . I think
> > > > most USB host drivers behave the same in such cases. For what USB
> > > > hardware/driver this is needed ? Perhaps simpler fix could be done
> > > > in USB host driver?
> > > 
> > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557
> > > single sg urb and urb with a configured transfer_buf are managed in a different way.
> > 
> > But this should not make any difference for underlying low level USB
> > host driver, since we map 1 buffer of the same size, just by using
> > different routines for that.
> 
> probably amd iommu has some constraints on sg buffer layout respect to intel
> one, not sure
> 
> > 
> > > Please not I have not received any confirm that this series fixes the reported issue
> > > yet :)
> > 
> > What is reported issue ?
> 
> https://marc.info/?l=linux-wireless&m=154716096506037&w=2

So, this if for AMD IOMMU issue as I thought initally. For the moment I
thought you are trying to fix some diffrent problem with some 
non-standart usb host controler.

I still think not using sg is just workaround for the problem not worth
to do, since we have already workaround in form of disabling IOMMU.
Right fix will be fixing AMD IOMMU driver or fix sg usage in mt76-usb
if it does something wrong.

Regards
Stanislaw 

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Lorenzo Bianconi]

> On Wed, Jan 16, 2019 at 02:40:46PM +0100, Lorenzo Bianconi wrote:
> > On Jan 16, Stanislaw Gruszka wrote:
> > > On Wed, Jan 16, 2019 at 12:44:33PM +0100, Lorenzo Bianconi wrote:
> > > > > On Tue, Jan 15, 2019 at 04:47:47PM +0100, Lorenzo Bianconi wrote:
> > > > > > Hi Stanislaw,
> > > > > 
> > > > > Hi :-)
> > > > > 
> > > > > > > Not sure what is the problem , but this patch set look like a workaround
> > > > > > > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > > > > > > in sg page mappings eigher on mt76 dirver or IOMMU driver.
> > > > > > 
> > > > > > The main point here I guess is we do not need sg if fragment number is one (e.g
> > > > > > usb2.0). Moreover this can fix IOMMU reported issues.
> > > > > 
> > > > > So there if diffrence for USB host driver when we have one usb->sg
> > > > > sengment and if we just pass the buffer via urb->transfer_buf . I think
> > > > > most USB host drivers behave the same in such cases. For what USB
> > > > > hardware/driver this is needed ? Perhaps simpler fix could be done
> > > > > in USB host driver?
> > > > 
> > > > According to https://github.com/torvalds/linux/blob/master/drivers/usb/core/hcd.c#L1557
> > > > single sg urb and urb with a configured transfer_buf are managed in a different way.
> > > 
> > > But this should not make any difference for underlying low level USB
> > > host driver, since we map 1 buffer of the same size, just by using
> > > different routines for that.
> > 
> > probably amd iommu has some constraints on sg buffer layout respect to intel
> > one, not sure
> > 
> > > 
> > > > Please not I have not received any confirm that this series fixes the reported issue
> > > > yet :)
> > > 
> > > What is reported issue ?
> > 
> > https://marc.info/?l=linux-wireless&m=154716096506037&w=2
> 
> So, this if for AMD IOMMU issue as I thought initally. For the moment I
> thought you are trying to fix some diffrent problem with some 
> non-standart usb host controler.

Why not standard? Most of net usb drivers do not use sg :)

> 
> I still think not using sg is just workaround for the problem not worth
> to do, since we have already workaround in form of disabling IOMMU.
> Right fix will be fixing AMD IOMMU driver or fix sg usage in mt76-usb
> if it does something wrong.

I agree with you that this is not strictly a fix for IOMMU issue but I think
we could avoid using sg when we are working just with linear skbs.
But first I guess we need some feedbacks from users

Regards,
Lorenzo

> 
> Regards
> Stanislaw 

Re: [RFC 0/4] do not use sg if not properly supported by usb controller

[Rosen Penev]

On Tue, Jan 15, 2019 at 7:47 AM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> > On Tue, Jan 15, 2019 at 01:33:41PM +0100, Lorenzo Bianconi wrote:
> > > 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. Some users have reported sg issues on AMD IOMMU
>
> Hi Stanislaw,
>
> >
> > Not sure what is the problem , but this patch set look like a workaround
> > not fix. If this an issue with IOMMU and sg, seems there is something wrong
> > in sg page mappings eigher on mt76 dirver or IOMMU driver.
>
> The main point here I guess is we do not need sg if fragment number is one (e.g
> usb2.0). Moreover this can fix IOMMU reported issues.
>
> @Rosen: could you please try this series enabling IOMMU?
I will try it later today.

Note that the same behavior occurs with USB 2.0 ports as well. I do
feel that the actual bug is in the IOMMU driver.
>
> >
> > If things need to be fixed in mt76 I whould check if page mappings for
> > sg are correct. Or remove sg usage from mt76_usb completly, mt76 MMIO
> > version do not use sg for framgments, so most likely USB don't need it
> > as well.
>
> usb scatter-gather is used to properly support non-linear skbs (A-MSDU,
> with usb3.0) since the hw (unlike pci counterpart) does not support it,
> so we need it.
>
> Regards,
> Lorenzo
>
> >
> > Thanks
> > Stanislaw