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

From: Stanislaw Gruszka <sgruszka@redhat.com>
To: Lorenzo Bianconi <lorenzo.bianconi@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+
Date: Mon, 11 Feb 2019 08:44:56 +0100	[thread overview]
Message-ID: <20190211074455.GA6292@redhat.com> (raw)
In-Reply-To: <CAJ0CqmXp2a-zwPMyMFu+UCqTHP7qNRiJFopDSdZWdms7dVef4Q@mail.gmail.com>

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 ?

> 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 ? ).

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.

Stanislaw