mt76x0u: Probing issues on Raspberry Pi 3 B+

BUG Lorenzo Bianconi 1 participant 1 message Feb 20, 2019 USB

Share & tools

Lorenzo Bianconi

author

2019-02-20 13:22 UTC #0 lore raw

Quoted 27 lines · Felix Fietkau

Quoted 15 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
The way I see it, we have two choices.
1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
2. Rely on urb->transfer_buffer and keep urb->sg NULL
I agree, if this is only needed for dwc2. Though I would investigate if this is not a bug on other platforms as well.
From what I can see, using Lorenzo's patches seems to be the better
Quoted 8 lines
Quoted 3 lines
solution, since they avoid these corner cases in dwc2 (and maybe other drivers as well). I will apply them and then we'll see if we need to do any further improvements later on.
They work on rpi dwc2, but they do not address root of the problem. There is clearly something wrong how mt76usb handle SG, what is not fixed. And adding disable_usb_sg module parameter for hcd's supporting SG should be red flag.
Quoted 5 lines
I think we're simply dealing with multiple issues here, only some of which are fixed by Lorenzo's patches.
I'm pretty sure it's still wrong for mt76 to try to align its buffers, since the Linux USB API supports non-aligned transfer buffers and it should be up to the controller driver to deal with that.
Agree.
Quoted 2 lines
dwc2 tries to do that, but that has limitations which I already pointed out and which are properly dealt with by Lorenzo's patches.

Quoted 8 lines

I planed to just accept current solution, but I started to work on patch that remove len, sglen arguments from mt76u_buf_alloc() and use q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how related code is now tangled.
Would be ok to send this patch with proper changelog as fix for RPI against wireless-drivers and cc:stable (assuming it works and really fix things on RPI) and revert Lorenzo's patches in -next ?

Hi Stanislaw,

what is the advantage of doing so? You have duplicated most of the fields that are already in the urb data structure and you use transfer_buffer (no SG I/O). Moreover I have ready a series that removes 99% of the dual allocation code and maintain it in the control path (instead of the datapath one).
I need just to rebase it ontop of your series. I will post it soon.

Regards,
Lorenzo

Quoted 234 lines

Stanislaw

From 4f8d7d3f4031b0a97b3bb147cb7e52533886e7cc Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <[email protected]>
Date: Wed, 20 Feb 2019 13:29:42 +0100
Subject: [PATCH] mt76usb: use urb transfer_buffer for one segment

Signed-off-by Stanislaw Gruszka <[email protected]>

---
 drivers/net/wireless/mediatek/mt76/mt76.h     |  2 +
 .../wireless/mediatek/mt76/mt76x02_usb_mcu.c  |  4 +-
 drivers/net/wireless/mediatek/mt76/usb.c      | 75 +++++++++++--------
 3 files changed, 46 insertions(+), 35 deletions(-)

drivers/net/wireless/mediatek/mt76/mt76.h +2 -0

index 2e5bcb3fdff7..7e0680daeee6 100644
@@ -86,6 +86,8 @@ struct mt76_queue_buf {
 struct mt76u_buf {
 	struct mt76_dev *dev;
 	struct urb *urb;
+	struct scatterlist *sg;
+	int num_sgs;
 	size_t len;
 	bool done;
 };

drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c +2 -2

index da299b8a1334..75561910d630 100644
@@ -90,7 +90,7 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
 		if (urb->status)
 			return -EIO;
 
-		data = sg_virt(&urb->sg[0]);
+		data = sg_virt(&buf->sg[0]);
 		if (usb->mcu.rp)
 			mt76x02u_multiple_mcu_reads(dev, data + 4,
 						    urb->actual_length - 8);

@@ -266,7 +266,7 @@ 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]);
+	u8 *data = sg_virt(&buf->sg[0]);
 	DECLARE_COMPLETION_ONSTACK(cmpl);
 	__le32 info;
 	u32 val;

drivers/net/wireless/mediatek/mt76/usb.c +42 ~~-33~~

index a1811c39415e..57bb16eaff06 100644
@@ -277,7 +277,6 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 		 int nsgs, int len, int sglen)
 {
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
-	struct urb *urb = buf->urb;
 	int i;
 
 	spin_lock_bh(&q->rx_page_lock);

@@ -292,21 +291,21 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 
 		page = virt_to_head_page(data);
 		offset = data - page_address(page);
-		sg_set_page(&urb->sg[i], page, sglen, offset);
+		sg_set_page(&buf->sg[i], page, sglen, offset);
 	}
 	spin_unlock_bh(&q->rx_page_lock);
 
 	if (i < nsgs) {
 		int j;
 
-		for (j = nsgs; j < urb->num_sgs; j++)
-			skb_free_frag(sg_virt(&urb->sg[j]));
-		urb->num_sgs = i;
+		for (j = nsgs; j < buf->num_sgs; j++)
+			skb_free_frag(sg_virt(&buf->sg[j]));
+		buf->num_sgs = i;
 	}
 
-	urb->num_sgs = max_t(int, i, urb->num_sgs);
-	buf->len = urb->num_sgs * sglen,
-	sg_init_marker(urb->sg, urb->num_sgs);
+	buf->num_sgs = max_t(int, i, buf->num_sgs);
+	buf->len = buf->num_sgs * sglen,
+	sg_init_marker(buf->sg, buf->num_sgs);
 
 	return i ? : -ENOMEM;
 }

@@ -318,13 +317,14 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 	if (!buf->urb)
 		return -ENOMEM;
 
-	buf->urb->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->urb->sg),
+	buf->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->sg),
 				    gfp);
-	if (!buf->urb->sg)
+	if (!buf->sg)
 		return -ENOMEM;
 
-	sg_init_table(buf->urb->sg, nsgs);
+	sg_init_table(buf->sg, nsgs);
 	buf->dev = dev;
+	buf->num_sgs = nsgs;
 
 	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
 }

@@ -332,12 +332,11 @@ EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
 
 void mt76u_buf_free(struct mt76u_buf *buf)
 {
-	struct urb *urb = buf->urb;
 	struct scatterlist *sg;
 	int i;
 
-	for (i = 0; i < urb->num_sgs; i++) {
-		sg = &urb->sg[i];
+	for (i = 0; i < buf->num_sgs; i++) {
+		sg = &buf->sg[i];
 		if (!sg)
 			continue;

@@ -347,9 +346,10 @@ void mt76u_buf_free(struct mt76u_buf *buf)
 }
 EXPORT_SYMBOL_GPL(mt76u_buf_free);
 
-int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
-		     struct mt76u_buf *buf, gfp_t gfp,
-		     usb_complete_t complete_fn, void *context)
+static void
+mt76u_fill_bulk_urb(struct mt76_dev *dev, int dir, int index,
+		    struct mt76u_buf *buf, usb_complete_t complete_fn,
+		    void *context)
 {
 	struct usb_interface *intf = to_usb_interface(dev->dev);
 	struct usb_device *udev = interface_to_usbdev(intf);

@@ -360,9 +360,22 @@ 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,
-			  complete_fn, context);
+	usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, buf->len, complete_fn,
+			  context);
+
+	if (buf->num_sgs > 1) {
+		buf->urb->num_sgs = buf->num_sgs;
+		buf->urb->sg = buf->sg;
+	} else {
+		buf->urb->transfer_buffer = sg_virt(buf->sg);
+	}
+}
 
+int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
+		     struct mt76u_buf *buf, gfp_t gfp,
+		     usb_complete_t complete_fn, void *context)
+{
+	mt76u_fill_bulk_urb(dev, dir, index, buf, complete_fn, context);
 	return usb_submit_urb(buf->urb, gfp);
 }
 EXPORT_SYMBOL_GPL(mt76u_submit_buf);

@@ -672,7 +685,7 @@ static void mt76u_complete_tx(struct urb *urb)
 }
 
 static int
-mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
+mt76u_tx_build_sg(struct sk_buff *skb, struct mt76u_buf *buf)
 {
 	int nsgs = 1 + skb_shinfo(skb)->nr_frags;
 	struct sk_buff *iter;

@@ -680,13 +693,14 @@ mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
 	skb_walk_frags(skb, iter)
 		nsgs += 1 + skb_shinfo(iter)->nr_frags;
 
-	memset(urb->sg, 0, sizeof(*urb->sg) * MT_SG_MAX_SIZE);
+	memset(buf->sg, 0, sizeof(*buf->sg) * MT_SG_MAX_SIZE);
 
 	nsgs = min_t(int, MT_SG_MAX_SIZE, nsgs);
-	sg_init_marker(urb->sg, nsgs);
-	urb->num_sgs = nsgs;
+	sg_init_marker(buf->sg, nsgs);
+	buf->num_sgs = nsgs;
+	buf->len = skb->len;
 
-	return skb_to_sgvec_nomark(skb, urb->sg, 0, skb->len);
+	return skb_to_sgvec_nomark(skb, buf->sg, 0, skb->len);
 }
 
 static int

@@ -694,12 +708,9 @@ mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
 		   struct sk_buff *skb, struct mt76_wcid *wcid,
 		   struct ieee80211_sta *sta)
 {
-	struct usb_interface *intf = to_usb_interface(dev->dev);
-	struct usb_device *udev = interface_to_usbdev(intf);
 	u8 ep = q2ep(q->hw_idx);
 	struct mt76u_buf *buf;
 	u16 idx = q->tail;
-	unsigned int pipe;
 	int err;
 
 	if (q->queued == q->ndesc)

@@ -712,13 +723,11 @@ 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);
+	err = mt76u_tx_build_sg(skb, buf);
 	if (err < 0)
 		return err;
 
-	pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[ep]);
-	usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, skb->len,
-			  mt76u_complete_tx, buf);
+	mt76u_fill_bulk_urb(dev, USB_DIR_OUT, ep, buf, mt76u_complete_tx, buf);
 
 	q->tail = (q->tail + 1) % q->ndesc;
 	q->entry[idx].skb = skb;

@@ -776,8 +785,8 @@ 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)
+			buf->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
+			if (!buf->sg)
 				return -ENOMEM;
 		}
 	}

Signature · 1 line

2.20.1