[PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Eduardo Abinader]

During removal of usb dongle, noticed many unhandled rx urb
below. This this patch, make it possible and early completion
of the rx tasklet.

mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
...

Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 5f0faf07c346..dc33df9cd155 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -446,6 +446,7 @@ static void mt76u_complete_rx(struct urb *urb)
 	case -ECONNRESET:
 	case -ESHUTDOWN:
 	case -ENOENT:
+	case -EPROTO:
 		return;
 	default:
 		dev_err(dev->dev, "rx urb failed: %d\n", urb->status);
-- 
2.19.1

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Lorenzo Bianconi]

> During removal of usb dongle, noticed many unhandled rx urb
> below. This this patch, make it possible and early completion
> of the rx tasklet.
>
> mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> ...

Hi Eduardo,

I think EPROTO is a more general error (it is not strictly related to
device removal)
and it could happen even during normal operation. In this case I guess we should
reinsert the urb to usb-core.

Regards,
Lorenzo

>
> Signed-off-by: Eduardo Abinader <eduardoabinader@gmail.com>
> ---
>  drivers/net/wireless/mediatek/mt76/usb.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index 5f0faf07c346..dc33df9cd155 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -446,6 +446,7 @@ static void mt76u_complete_rx(struct urb *urb)
>         case -ECONNRESET:
>         case -ESHUTDOWN:
>         case -ENOENT:
> +       case -EPROTO:
>                 return;
>         default:
>                 dev_err(dev->dev, "rx urb failed: %d\n", urb->status);
> --
> 2.19.1
>

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Stanislaw Gruszka]

On Sat, Nov 24, 2018 at 10:25:52AM +0100, Lorenzo Bianconi wrote:
> > During removal of usb dongle, noticed many unhandled rx urb
> > below. This this patch, make it possible and early completion
> > of the rx tasklet.
> >
> > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71

If problem here is just printing errors, I think we are ok. If problem
is some hung or infinity loop, it should be fixed.

> Hi Eduardo,
> 
> I think EPROTO is a more general error (it is not strictly related to
> device removal)
> and it could happen even during normal operation. In this case I guess we should
> reinsert the urb to usb-core.

Some (out of tree) usb host drivers returns EPROTO all the time
in some error conditions. On rt2x00 resubmitting urb resulted in
infinity loop:
https://marc.info/?t=153375128700002&r=1&w=2

Fix I proposed for that was counting EPROTO errors and 
mark device as removed if 10 of them happened in row:
https://marc.info/?l=linux-wireless&m=153441755529960&w=2

Thanks
Stanislaw 

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Eduardo Abinader]

On Mon, 26 Nov 2018 at 10:59, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
>
> On Sat, Nov 24, 2018 at 10:25:52AM +0100, Lorenzo Bianconi wrote:
> > > During removal of usb dongle, noticed many unhandled rx urb
> > > below. This this patch, make it possible and early completion
> > > of the rx tasklet.
> > >
> > > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
> > > mt76x2u 1-3.4.3.1.2:1.0: rx urb failed: -71
>
> If problem here is just printing errors, I think we are ok. If problem
> is some hung or infinity loop, it should be fixed.
>
> > Hi Eduardo,
> >
> > I think EPROTO is a more general error (it is not strictly related to
> > device removal)
> > and it could happen even during normal operation. In this case I guess we should
> > reinsert the urb to usb-core.
>
> Some (out of tree) usb host drivers returns EPROTO all the time
> in some error conditions. On rt2x00 resubmitting urb resulted in
> infinity loop:
> https://marc.info/?t=153375128700002&r=1&w=2
>
> Fix I proposed for that was counting EPROTO errors and
> mark device as removed if 10 of them happened in row:
> https://marc.info/?l=linux-wireless&m=153441755529960&w=2

Thanks for the feedback, guys.

Would it be ok for you if I respin the patch with the suggested
approach by Stanislaw?

Regards,
Eduardo
>
> Thanks
> Stanislaw

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Lorenzo Bianconi]

> > > > During removal of usb dongle, noticed many unhandled rx urb
> > > > below. This this patch, make it possible and early completion
> > > > of the rx tasklet.

[...]

> > > Hi Eduardo,
> > >
> > > I think EPROTO is a more general error (it is not strictly related to
> > > device removal)
> > > and it could happen even during normal operation. In this case I guess we should
> > > reinsert the urb to usb-core.
> >
> > Some (out of tree) usb host drivers returns EPROTO all the time
> > in some error conditions. On rt2x00 resubmitting urb resulted in
> > infinity loop:
> > https://marc.info/?t=153375128700002&r=1&w=2
> >
> > Fix I proposed for that was counting EPROTO errors and
> > mark device as removed if 10 of them happened in row:
> > https://marc.info/?l=linux-wireless&m=153441755529960&w=2
>
> Thanks for the feedback, guys.
>
> Would it be ok for you if I respin the patch with the suggested
> approach by Stanislaw?

Hi Eduardo,

AFAIU the -EPROTO error is reported only during device removal or just
sporadically
(you have never experienced any infinity loop). Is it correct?
If so I guess we do not need to develop any intrusive workaround since
it is just an annoying log message

Regards,
Lorenzo

>
> Regards,
> Eduardo
> >
> > Thanks
> > Stanislaw

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Eduardo Abinader]

On Mon, 26 Nov 2018 at 22:31, Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> > > > > During removal of usb dongle, noticed many unhandled rx urb
> > > > > below. This this patch, make it possible and early completion
> > > > > of the rx tasklet.
>
> [...]
>
> > > > Hi Eduardo,
> > > >
> > > > I think EPROTO is a more general error (it is not strictly related to
> > > > device removal)
> > > > and it could happen even during normal operation. In this case I guess we should
> > > > reinsert the urb to usb-core.
> > >
> > > Some (out of tree) usb host drivers returns EPROTO all the time
> > > in some error conditions. On rt2x00 resubmitting urb resulted in
> > > infinity loop:
> > > https://marc.info/?t=153375128700002&r=1&w=2
> > >
> > > Fix I proposed for that was counting EPROTO errors and
> > > mark device as removed if 10 of them happened in row:
> > > https://marc.info/?l=linux-wireless&m=153441755529960&w=2
> >
> > Thanks for the feedback, guys.
> >
> > Would it be ok for you if I respin the patch with the suggested
> > approach by Stanislaw?
>
> Hi Eduardo,
>
> AFAIU the -EPROTO error is reported only during device removal or just
> sporadically
> (you have never experienced any infinity loop). Is it correct?
> If so I guess we do not need to develop any intrusive workaround since
> it is just an annoying log message
>

It is reported only during device removal as I've been seen with my
comfast dongle.
The device removal does not lead to an infinity loop, but there's a
bouncing around 100 urb failed msgs...

Thanks,
Eduardo

> Regards,
> Lorenzo
>
> >
> > Regards,
> > Eduardo
> > >
> > > Thanks
> > > Stanislaw

Re: [PATCH] mt76: handle protocol error to proper deinit rx_tasklet

[Lorenzo Bianconi]

> On Mon, 26 Nov 2018 at 22:31, Lorenzo Bianconi
> <lorenzo.bianconi@redhat.com> wrote:

[...]

> > Hi Eduardo,
> >
> > AFAIU the -EPROTO error is reported only during device removal or just
> > sporadically
> > (you have never experienced any infinity loop). Is it correct?
> > If so I guess we do not need to develop any intrusive workaround since
> > it is just an annoying log message
> >
>
> It is reported only during device removal as I've been seen with my
> comfast dongle.
> The device removal does not lead to an infinity loop, but there's a
> bouncing around 100 urb failed msgs...
>

Thx for the clarification Eduardo. So is it better to mute all -EPROTO errors
or tolerate these annoying messages during dongle removal? I would prefer the
second choice since would be useful to know if the controller reports
that kind of errors.

Regards,
Lorenzo
> Thanks,
> Eduardo
>
> > Regards,
> > Lorenzo
> >
> > >
> > > Regards,
> > > Eduardo
> > > >
> > > > Thanks
> > > > Stanislaw