Patchwork [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+

login
register
mail settings
Submitter Stanislaw Gruszka
Date Feb. 12, 2019, 9:30 a.m.
Message ID <20190212093035.GB12906@redhat.com>
Download mbox | patch
Permalink /patch/723699/
State New
Headers show

Comments

Stanislaw Gruszka - Feb. 12, 2019, 9:30 a.m.
On Tue, Feb 12, 2019 at 01:06:00AM +0100, Lorenzo Bianconi wrote:
> >
> > On Mon, 11 Feb 2019, Stanislaw Gruszka wrote:
> >
> > > On Mon, Feb 11, 2019 at 10:12:57AM -0500, Alan Stern wrote:
> > > > On Mon, 11 Feb 2019, Lorenzo Bianconi wrote:
> > > >
> > > > > Here it is a different issue respect to the AMD IOMMU one, dwc2 host driver
> > > > > does not implement SG I/O so probing fails. I guess it is still useful to
> > > > > implement a 'legacy' mode that enable mt76 on host controllers that do not implement
> > > > > SG I/O (rpi is a very common device so it will be cool to have mt76 working on
> > > > > it). Moreover we are not removing functionalities, user experience will remain
> > > > > the same
> > > >
> > > > Has anyone considered adding SG support to dwc2?  It shouldn't be very
> > > > difficult.  The corresponding change for ehci-hcd required adding no
> > > > more than about 30 lines of code.
> > >
> > > That would be cool. Perhaps somebody with dwc2 hardware could do this.
> > >
> > > However in mt76x02u we possibly would like to support other usb host
> > > drivers with sg_tablesize = 0 . I would like to clarify what is correct
> > > to do with such drivers.
> > >
> > > Is ok to pass buffer via urb->sg with urb->num_sgs = 1 ? Or maybe
> > > urb->num_sgs should be 0 to pass buffer via urb->sg on such drivers ?
> > > Or maybe non of above is correct and the only option that will work
> > > in 100% is pass buffer via urb->transfer_buffer ?
> >
> > See the kerneldoc for usb_sg_init(), usb_sg_wait(), and usb_sg_cancel()
> > in drivers/usb/core/message.c.  These routines will always do the right
> > thing -- however usb_sg_wait() must be called in process context.
> >
> > Alan Stern
> >
> 
> Hi Alan,
> 
> I actually used usb_sg_init()/usb_sg_wait() as reference to implement
> mt76u {tx/rx} datapath, but I will double-check.
> I guess we should even consider if there are other usb host drivers
> that do not implement SG I/O and it is worth to support.
> I am wondering if the right approach is to add SG to the controller
> one by one or have legacy I/O in mt76 (not sure what is the 'best'
> approach)

In usb_sg_init() urb->num_sgs is set 0 for sg_tablesize = 0 controllers.
In mt76 we set urb->num_sgs to 1. I thought it is fine, but now I think
this is bug. We can fix that without changing allocation method and
still use SG allocation. Attached patch do this, please check if it works
on rpi. Patch is on top of your error path fixes.

Stanislaw
From f79ac0df967d406523d0a1c03a138d1394e7665a Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Tue, 12 Feb 2019 10:02:53 +0100
Subject: [PATCH] mt76usb: do not set urb->num_sgs to 1 for non SG usb host
 drivers

Track number of segments in mt76u_buf structure and do not
submit urbs with urb->num_sgs = 1 if usb host driver
sg_tablesize is zero.

This suppose fix problem of mt76 not working with some usb
host controllers like dwc2.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
 drivers/net/wireless/mediatek/mt76/usb.c  | 55 ++++++++++++++---------
 2 files changed, 36 insertions(+), 20 deletions(-)
Lorenzo Bianconi - Feb. 12, 2019, 11:58 a.m.
[...]

> In usb_sg_init() urb->num_sgs is set 0 for sg_tablesize = 0 controllers.
> In mt76 we set urb->num_sgs to 1. I thought it is fine, but now I think
> this is bug. We can fix that without changing allocation method and
> still use SG allocation. Attached patch do this, please check if it works
> on rpi. Patch is on top of your error path fixes.
> 
> Stanislaw

> From f79ac0df967d406523d0a1c03a138d1394e7665a Mon Sep 17 00:00:00 2001
> From: Stanislaw Gruszka <sgruszka@redhat.com>
> Date: Tue, 12 Feb 2019 10:02:53 +0100
> Subject: [PATCH] mt76usb: do not set urb->num_sgs to 1 for non SG usb host
>  drivers
> 
> Track number of segments in mt76u_buf structure and do not
> submit urbs with urb->num_sgs = 1 if usb host driver
> sg_tablesize is zero.
> 
> This suppose fix problem of mt76 not working with some usb
> host controllers like dwc2.
> 
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  drivers/net/wireless/mediatek/mt76/mt76.h |  1 +
>  drivers/net/wireless/mediatek/mt76/usb.c  | 55 ++++++++++++++---------
>  2 files changed, 36 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 2e5bcb3fdff7..eadc913c37b6 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -86,6 +86,7 @@ struct mt76_queue_buf {
>  struct mt76u_buf {
>  	struct mt76_dev *dev;
>  	struct urb *urb;
> +	int num_sgs;
>  	size_t len;
>  	bool done;
>  };
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index a1811c39415e..d82de59ec6dc 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -299,14 +299,14 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
>  	if (i < nsgs) {
>  		int j;
>  
> -		for (j = nsgs; j < urb->num_sgs; j++)
> +		for (j = nsgs; j < buf->num_sgs; j++)
>  			skb_free_frag(sg_virt(&urb->sg[j]));
> -		urb->num_sgs = i;
> +		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(urb->sg, buf->num_sgs);
>  
>  	return i ? : -ENOMEM;
>  }
> @@ -325,6 +325,7 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
>  
>  	sg_init_table(buf->urb->sg, nsgs);
>  	buf->dev = dev;
> +	buf->num_sgs = nsgs;
>  
>  	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
>  }
> @@ -336,7 +337,7 @@ void mt76u_buf_free(struct mt76u_buf *buf)
>  	struct scatterlist *sg;
>  	int i;
>  
> -	for (i = 0; i < urb->num_sgs; i++) {
> +	for (i = 0; i < buf->num_sgs; i++) {
>  		sg = &urb->sg[i];
>  		if (!sg)
>  			continue;
> @@ -347,9 +348,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 +362,25 @@ 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 (udev->bus->sg_tablesize > 0) {
> +		buf->urb->num_sgs = buf->num_sgs;
> +	} else {
> +		WARN_ON_ONCE(buf->num_sgs != 1);
> +		/* See usb_sg_init() */
> +		buf->urb->num_sgs = 0;
> +		if (!PageHighMem(sg_page(buf->urb->sg)))
> +			buf->urb->transfer_buffer = sg_virt(buf->urb->sg);

please correct me if I am wrong but doing so we will use transfer_buffer
for 95% of the use cases, right? If so why not keep it simple and always
use it when sg_tablesize is 0 (avoiding SG I/O)?
Btw this is what I proposed with the RFC series actually :) (always use
transfer_buffer in the usb 2.0 case)

Regards,
Lorenzo

> +	}
> +}
>  
> +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,10 +690,11 @@ 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;
> +	struct urb *urb = buf->urb;
>  
>  	skb_walk_frags(skb, iter)
>  		nsgs += 1 + skb_shinfo(iter)->nr_frags;
> @@ -684,7 +703,8 @@ mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
>  
>  	nsgs = min_t(int, MT_SG_MAX_SIZE, nsgs);
>  	sg_init_marker(urb->sg, nsgs);
> -	urb->num_sgs = nsgs;
> +	buf->num_sgs = nsgs;
> +	buf->len = skb->len;
>  
>  	return skb_to_sgvec_nomark(skb, urb->sg, 0, skb->len);
>  }
> @@ -694,12 +714,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 +729,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;
> -- 
> 2.19.2
>
Stanislaw Gruszka - Feb. 12, 2019, 1:15 p.m.
On Tue, Feb 12, 2019 at 12:58:43PM +0100, Lorenzo Bianconi wrote:
> > +	usb_fill_bulk_urb(buf->urb, udev, pipe, NULL, buf->len, complete_fn,
> > +			  context);
> > +
> > +	if (udev->bus->sg_tablesize > 0) {
> > +		buf->urb->num_sgs = buf->num_sgs;
> > +	} else {
> > +		WARN_ON_ONCE(buf->num_sgs != 1);
> > +		/* See usb_sg_init() */
> > +		buf->urb->num_sgs = 0;
> > +		if (!PageHighMem(sg_page(buf->urb->sg)))
> > +			buf->urb->transfer_buffer = sg_virt(buf->urb->sg);
> 
> please correct me if I am wrong but doing so we will use transfer_buffer
> for 95% of the use cases, right? If so why not keep it simple and always
> use it when sg_tablesize is 0 (avoiding SG I/O)?
>
> Btw this is what I proposed with the RFC series actually :) (always use
> transfer_buffer in the usb 2.0 case)

Because this way we do not change allocation method (use SG allocation)
what is simpler IMHO than having two buffer allocation methods.

And this way we do not hide problems with SG allocation and test it.

Additionally this patch address actual bug: using wrong urb->num_sgs and
can be easier to backport to older releases than 4 patch series. 

Stanislaw
Stefan Wahren - Feb. 14, 2019, 6:49 a.m.
Hi Stanislaw,

> Stanislaw Gruszka <sgruszka@redhat.com> hat am 12. Februar 2019 um 10:30 geschrieben:
> 
> 
> 
> In usb_sg_init() urb->num_sgs is set 0 for sg_tablesize = 0 controllers.
> In mt76 we set urb->num_sgs to 1. I thought it is fine, but now I think
> this is bug. We can fix that without changing allocation method and
> still use SG allocation. Attached patch do this, please check if it works
> on rpi. Patch is on top of your error path fixes.

your patch didn't apply cleanly to yesterdays next. After some minor manual fixup, i was able to build them and here are the results starting from boot (please ignore the invalid time in the kernel log):
https://gist.github.com/lategoodbye/33bd5bc75b9fc935faa231bc472defa8

Using multi_v7_defconfig i'm getting a warning on the first connect and always this flood of rx urb failed on disconnect. The driver seems to probe but isn't functional even after 2 tries.

Using arm64_defconfig i don't get any warning. But except of this i'm getting similiar results to multi_v7_defconfig.

So in comparison, Lorenzo's workaround behaves better.

Stefan

> 
> Stanislaw

Patch

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2e5bcb3fdff7..eadc913c37b6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -86,6 +86,7 @@  struct mt76_queue_buf {
 struct mt76u_buf {
 	struct mt76_dev *dev;
 	struct urb *urb;
+	int num_sgs;
 	size_t len;
 	bool done;
 };
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index a1811c39415e..d82de59ec6dc 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -299,14 +299,14 @@  mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 	if (i < nsgs) {
 		int j;
 
-		for (j = nsgs; j < urb->num_sgs; j++)
+		for (j = nsgs; j < buf->num_sgs; j++)
 			skb_free_frag(sg_virt(&urb->sg[j]));
-		urb->num_sgs = i;
+		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(urb->sg, buf->num_sgs);
 
 	return i ? : -ENOMEM;
 }
@@ -325,6 +325,7 @@  int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 
 	sg_init_table(buf->urb->sg, nsgs);
 	buf->dev = dev;
+	buf->num_sgs = nsgs;
 
 	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
 }
@@ -336,7 +337,7 @@  void mt76u_buf_free(struct mt76u_buf *buf)
 	struct scatterlist *sg;
 	int i;
 
-	for (i = 0; i < urb->num_sgs; i++) {
+	for (i = 0; i < buf->num_sgs; i++) {
 		sg = &urb->sg[i];
 		if (!sg)
 			continue;
@@ -347,9 +348,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 +362,25 @@  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 (udev->bus->sg_tablesize > 0) {
+		buf->urb->num_sgs = buf->num_sgs;
+	} else {
+		WARN_ON_ONCE(buf->num_sgs != 1);
+		/* See usb_sg_init() */
+		buf->urb->num_sgs = 0;
+		if (!PageHighMem(sg_page(buf->urb->sg)))
+			buf->urb->transfer_buffer = sg_virt(buf->urb->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,10 +690,11 @@  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;
+	struct urb *urb = buf->urb;
 
 	skb_walk_frags(skb, iter)
 		nsgs += 1 + skb_shinfo(iter)->nr_frags;
@@ -684,7 +703,8 @@  mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
 
 	nsgs = min_t(int, MT_SG_MAX_SIZE, nsgs);
 	sg_init_marker(urb->sg, nsgs);
-	urb->num_sgs = nsgs;
+	buf->num_sgs = nsgs;
+	buf->len = skb->len;
 
 	return skb_to_sgvec_nomark(skb, urb->sg, 0, skb->len);
 }
@@ -694,12 +714,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 +729,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;