Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <a3c72b07e229e1d16e93438d741d69abba6a18cf.1559822991.git.lorenzo@kernel.org>
 <20190606114228.7a0ba115@cakuba.netronome.com> <CAJ0CqmUySwtgzPCFR0QsZ-XA7Lm7xU8eMsELV8LEd2MEkhfnLA@mail.gmail.com>
 <20190606142622.7e5f11de@cakuba.netronome.com>
In-Reply-To: <20190606142622.7e5f11de@cakuba.netronome.com>
From:   Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date:   Thu, 6 Jun 2019 23:59:52 +0200
Message-ID: <CAJ0CqmWWrQnc55+LVS9L2tXN7MGEFUcaxfLbUbiCRpFitjaJfw@mail.gmail.com>
Subject: Re: [PATCH] mt7601u: do not schedule rx_tasklet when the device has
 been disconnected
To:     Jakub Kicinski <kubakici@wp.pl>
Cc:     Lorenzo Bianconi <lorenzo@kernel.org>,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

>
> On Thu, 6 Jun 2019 23:02:08 +0200, Lorenzo Bianconi wrote:
> > > On Thu,  6 Jun 2019 14:26:12 +0200, Lorenzo Bianconi wrote:
> > Hi Jakub,
> >
> > thx for the fast review :)
>
> I guess I'm used to the 24h "review timeout" on netdev :)
>

:-)

> > > Is this a new thing?  I def tested unplugging the dongle under
> > > traffic, but that must had been in 3.19 days :S
> >
> > I do not know if the issue has been introduced in recent kernel, I am
> > able to trigger it in a vm running
> > wireless-drivers-next and it has been reported here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=203249
>
> I see.  I'm just worried that we make a mistake here and it will get
> backported all the way back to very old kernels because of the fixes
> tag.  If old kernels worked perhaps it's not worth disturbing them.
>
> > > > Fixes: c869f77d6abb ("add mt7601u driver")
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > > I will post a patch to fix tx side as well
> > > > ---
> > > >  drivers/net/wireless/mediatek/mt7601u/dma.c | 33 ++++++++++-----------
> > > >  1 file changed, 16 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > > index f7edeffb2b19..e7703990b291 100644
> > > > --- a/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > > +++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
> > > > @@ -193,10 +193,20 @@ static void mt7601u_complete_rx(struct urb *urb)
> > > >       struct mt7601u_rx_queue *q = &dev->rx_q;
> > > >       unsigned long flags;
> > > >
> > > > -     spin_lock_irqsave(&dev->rx_lock, flags);
> > > > +     switch (urb->status) {
> > > > +     case -ECONNRESET:
> > > > +     case -ESHUTDOWN:
> > > > +     case -ENOENT:
> > > > +             return;
> > >
> > > So we assume this is non-recoverable?  Everything will fail after?
> > > Because pending is incremented linearly :S  That's why there is a
> > > warning here.
> >
> > AFAIK -ECONNRESET/-ENOENT are related to urb unlink/kill while
> > -ESHUTDOWN is related to device disconnection.
> > I guess we can assume the device has been removed if we get these errors
>
> Makes sense.  A bit of an implicit assumption, USB subsystem may break
> this for us.  Let's at least put a comment here so we can go back and
> know that at the time of committing we did double check?
>

ack, I will put a comment in v2

> > > > +     default:
> > > > +             dev_err_ratelimited(dev->dev, "rx urb failed: %d\n",
> > > > +                                 urb->status);
> > > > +             /* fall through */
> > > > +     case 0:
> > > > +             break;
> > > > +     }
> > > >
> > > > -     if (mt7601u_urb_has_error(urb))
> > > > -             dev_err(dev->dev, "Error: RX urb failed:%d\n", urb->status);
> > > > +     spin_lock_irqsave(&dev->rx_lock, flags);
> > > >       if (WARN_ONCE(q->e[q->end].urb != urb, "RX urb mismatch"))
> > > >               goto out;
> > > >
> > > > @@ -363,19 +373,10 @@ int mt7601u_dma_enqueue_tx(struct mt7601u_dev *dev, struct sk_buff *skb,
> > > >  static void mt7601u_kill_rx(struct mt7601u_dev *dev)
> > > >  {
> > > >       int i;
> > > > -     unsigned long flags;
> > > > -
> > > > -     spin_lock_irqsave(&dev->rx_lock, flags);
> > > >
> > > > -     for (i = 0; i < dev->rx_q.entries; i++) {
> > > > -             int next = dev->rx_q.end;
> > > > -
> > > > -             spin_unlock_irqrestore(&dev->rx_lock, flags);
> > > > -             usb_poison_urb(dev->rx_q.e[next].urb);
> > > > -             spin_lock_irqsave(&dev->rx_lock, flags);
> > > > -     }
> > >
> > > Why is there no need to take the lock?  Admittedly it's not clear what
> > > this lock is protecting here :P  Perhaps a separate patch to remove the
> > > unnecessary locking with an explanation?
> >
> > usb_poison_urb() can run concurrently with urb completion so I guess
> > we do not need locks here.
> > I guess we need to maintain this chunk in the same patch since now
> > when the device is disconnected
> > we do not increment dev->rx_q.end. What do you think?
>
> Ah, I see!  The completion used to run in between the unlock/lock!
> Now we just poison out of order, because completion doesn't care about
> ordering for errored URBs.  Perhaps worth putting in the commit message?

ack, I will put a comment in v2

Regards,
Lorenzo