Date:   Mon, 11 Feb 2019 11:04:06 +0100
From:   Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
To:     Stanislaw Gruszka <sgruszka@redhat.com>
Cc:     Stefan Wahren <stefan.wahren@i2se.com>,
        Felix Fietkau <nbd@nbd.name>,
        Doug Anderson <dianders@chromium.org>,
        Minas Harutyunyan <hminas@synopsys.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        linux-usb@vger.kernel.org
Subject: Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
Message-ID: <20190211100405.GD3467@localhost.localdomain>
References: <2003727085.234456.1549714119945@email.ionos.de>
 <CAJ0CqmXSuNQ_tQOatyKwO9O2KsxqD5NZRQnngOme5PURssiv5Q@mail.gmail.com>
 <165515185.283024.1549744145982@email.ionos.de>
 <20190210094123.GB2913@redhat.com>
 <CAJ0CqmXp2a-zwPMyMFu+UCqTHP7qNRiJFopDSdZWdms7dVef4Q@mail.gmail.com>
 <20190211074455.GA6292@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190211074455.GA6292@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

> On Sun, Feb 10, 2019 at 11:22:25AM +0100, Lorenzo Bianconi wrote:
> > > On Sat, Feb 09, 2019 at 09:29:05PM +0100, Stefan Wahren wrote:
> > > > > could you please test the following series:
> > > > > https://patchwork.kernel.org/cover/10764453/
> > > >
> > > > yeah this fixed the probing timeout and the driver will probe successful. AFAIK the dwc2 host mode doesn't support scatter-gather yet.
> > >
> > > So this is either dwc2 scatter-gather problem which should be addressed in
> > > this driver or mt76x0u does something wrong when configuring SG.
> > >
> > > Disabling SG is just workaround, which do not address actual problem.
> > >
> > > I think I found mt76x0u issue that could cause this USB probe error
> > > (and possibly also address AMD IOMMU issue). We seems do not correctly
> > > set URB transfer length smaller than sg buffer length. Attached  patch
> > > should correct that.
> > 
> > Hi Stanislaw,
> > 
> > I think 'sg[0].length' is already set in mt76u_fill_rx_sg().
> 
> It is, buf->len and sg[0].length are initialized to the same value for 1
> segment. But then buf->len (assigned to urb->buffer_transfer_length) change
> to smaller value , but sg[0].length stay the same. What I think can be
> problem for usb host driver.
> 
> > Moreover applying this patch I got the following crash (rpi-5.0.y):
> 
> Ok, so with patch probe fail instantly and trigger yet another bug(s)
> on error path. You seems to address that already. 
> 
> > Moreover for mt76x0u SG is 'already' disabled since we use just one
> > buffer so from performance point of view I do not see any difference
> > of using a standard usb buffer.
> > This patch has been tested in multiple scenarios and seems to fix
> > reported issues (for usb2.0).
> 
> Ok, so passing buffer via urb->transfer_buffer works. But why urb->sg
> does not work for 1 segment ?

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

> 
> > Are you concerned about increasing code complexity?
> 
> That's one of my concerns. Another, more important one, is that
> changing to urb->transfer_buffer just hide the problems. And they will
> pop up when someone will start to use SG (BTW how this can be tested
> for more than one fragment, IOW how multiple fragments skb's can
> be generated ? ).

You need:
- usb 3.0 controller/device
- A-MSDU capable AP

> 
> And now I think the bugs can be in mt76 driver taking that problems
> happened on different platforms (rpi and AMD IOMMU), i.e. we do not
> correctly set urb->nub_seg or length or do some other thing wrong.

We still need to fix it on usb3.0 with AMD cpu/motherboard :)

Regards,
Lorenzo

> 
> Stanislaw