Linux-Wireless Archive on lore.kernel.org
 help / Atom feed
* [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
@ 2019-02-09 12:08 Stefan Wahren
  2019-02-09 18:46 ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-09 12:08 UTC (permalink / raw)
  To: Felix Fietkau, Lorenzo Bianconi, Stanislaw Gruszka,
	Doug Anderson, Minas Harutyunyan
  Cc: linux-wireless, linux-usb

Hi,

as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.

I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:

[   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
[   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
[   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
[   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
[   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
[   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
[   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
[   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
[   33.454829] usbcore: registered new interface driver mt76x0u

I took the firmware files from here [2].

Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:

mt76x0u 1.1.3:1.0: rx urb failed: -71

Any chance to narrow down these issues?

[1] - https://marc.info/?l=linux-wireless&m=154875316703367
[2] - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 12:08 [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+ Stefan Wahren
@ 2019-02-09 18:46 ` Lorenzo Bianconi
  2019-02-09 20:29   ` Stefan Wahren
  2019-02-10  9:29   ` Stanislaw Gruszka
  0 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-09 18:46 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Felix Fietkau, Stanislaw Gruszka, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

>
> Hi,
>
> as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
>
> I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
>
> [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> [   33.454829] usbcore: registered new interface driver mt76x0u
>
> I took the firmware files from here [2].
>
> Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
>
> mt76x0u 1.1.3:1.0: rx urb failed: -71
>
> Any chance to narrow down these issues?

Hi Stefan,

could you please test the following series:
https://patchwork.kernel.org/cover/10764453/
Applying this series I am able to connect to my home AP using an Asus
USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git

Regards,
Lorenzo

>
> [1] - https://marc.info/?l=linux-wireless&m=154875316703367
> [2] - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 18:46 ` Lorenzo Bianconi
@ 2019-02-09 20:29   ` Stefan Wahren
  2019-02-09 20:33     ` Lorenzo Bianconi
  2019-02-10  9:41     ` Stanislaw Gruszka
  2019-02-10  9:29   ` Stanislaw Gruszka
  1 sibling, 2 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-09 20:29 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Felix Fietkau, Stanislaw Gruszka, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

Hi Lorenzo,

> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> hat am 9. Februar 2019 um 19:46 geschrieben:
> 
> 
> >
> > Hi,
> >
> > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> >
> > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> >
> > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > [   33.454829] usbcore: registered new interface driver mt76x0u
> >
> > I took the firmware files from here [2].
> >
> > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> >
> > mt76x0u 1.1.3:1.0: rx urb failed: -71
> >
> > Any chance to narrow down these issues?
> 
> Hi Stefan,
> 
> 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.

> Applying this series I am able to connect to my home AP using an Asus
> USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git

The foundation kernel uses a different USB driver called dwc-otg which make use of FIQ.

Okay, now we only need to find a solution for the "rx urb failed" flood ;-)

Thank you very much
Stefan

> 
> Regards,
> Lorenzo
> 
> >
> > [1] - https://marc.info/?l=linux-wireless&m=154875316703367
> > [2] - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 20:29   ` Stefan Wahren
@ 2019-02-09 20:33     ` Lorenzo Bianconi
  2019-02-09 22:47       ` Stefan Wahren
  2019-02-10  9:41     ` Stanislaw Gruszka
  1 sibling, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-09 20:33 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Felix Fietkau, Stanislaw Gruszka, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

>
> Hi Lorenzo,
>
> > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> hat am 9. Februar 2019 um 19:46 geschrieben:
> >
> >
> > >
> > > Hi,
> > >
> > > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> > >
> > > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> > >
> > > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > > [   33.454829] usbcore: registered new interface driver mt76x0u
> > >
> > > I took the firmware files from here [2].
> > >
> > > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> > >
> > > mt76x0u 1.1.3:1.0: rx urb failed: -71
> > >
> > > Any chance to narrow down these issues?
> >
> > Hi Stefan,
> >
> > 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.
>

Hi Stefan,

is the dongle working properly now? Are you able to connect to your AP?

> > Applying this series I am able to connect to my home AP using an Asus
> > USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> > I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git
>
> The foundation kernel uses a different USB driver called dwc-otg which make use of FIQ.
>
> Okay, now we only need to find a solution for the "rx urb failed" flood ;-)

Does it happen just if you remove the dongle during probe?

Regards,
Lorenzo

>
> Thank you very much
> Stefan
>
> >
> > Regards,
> > Lorenzo
> >
> > >
> > > [1] - https://marc.info/?l=linux-wireless&m=154875316703367
> > > [2] - https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 20:33     ` Lorenzo Bianconi
@ 2019-02-09 22:47       ` Stefan Wahren
  0 siblings, 0 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-09 22:47 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Felix Fietkau, Stanislaw Gruszka, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

Hi Lorenzo,

> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> hat am 9. Februar 2019 um 21:33 geschrieben:
> 
> 
> >
> > Hi Lorenzo,
> >
> > > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> hat am 9. Februar 2019 um 19:46 geschrieben:
> > >
> > >
> > > >
> > > > Hi,
> > > >
> > > > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> > > >
> > > > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> > > >
> > > > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > > > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > > > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > > > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > > > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > > > [   33.454829] usbcore: registered new interface driver mt76x0u
> > > >
> > > > I took the firmware files from here [2].
> > > >
> > > > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> > > >
> > > > mt76x0u 1.1.3:1.0: rx urb failed: -71
> > > >
> > > > Any chance to narrow down these issues?
> > >
> > > Hi Stefan,
> > >
> > > 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.
> >
> 
> Hi Stefan,
> 
> is the dongle working properly now? Are you able to connect to your AP?

i successful tested STA mode with 2.4 GHz and 5 GHz.

> 
> > > Applying this series I am able to connect to my home AP using an Asus
> > > USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> > > I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git
> >
> > The foundation kernel uses a different USB driver called dwc-otg which make use of FIQ.
> >
> > Okay, now we only need to find a solution for the "rx urb failed" flood ;-)
> 
> Does it happen just if you remove the dongle during probe?

I also see those messages while disconnecting after a successful probe but in this case the messages stopps after a few seconds. In case of a disconnect while probing the flood never ends. It looks like the rx tasklet is triggered forever.

Stefan

> 
> Regards,
> Lorenzo
>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 18:46 ` Lorenzo Bianconi
  2019-02-09 20:29   ` Stefan Wahren
@ 2019-02-10  9:29   ` Stanislaw Gruszka
  2019-02-10 16:38     ` Stefan Wahren
  1 sibling, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-10  9:29 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stefan Wahren, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	linux-wireless, linux-usb

On Sat, Feb 09, 2019 at 07:46:37PM +0100, Lorenzo Bianconi wrote:
> > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> >
> > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> >
> > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > [   33.454829] usbcore: registered new interface driver mt76x0u
> >
> > I took the firmware files from here [2].
> >
> > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> >
> > mt76x0u 1.1.3:1.0: rx urb failed: -71
> >
> > Any chance to narrow down these issues?
> 
> Hi Stefan,
> 
> could you please test the following series:
> https://patchwork.kernel.org/cover/10764453/
> Applying this series I am able to connect to my home AP using an Asus
> USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git

How the patch series fixed the issue since it was reported before that 
it does not work on 4.19.x which do not use scatter gatter I/O ?

Or it is regression ? 4.19.x driver works with rasperrypi/linux but -next
does not ?

Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-09 20:29   ` Stefan Wahren
  2019-02-09 20:33     ` Lorenzo Bianconi
@ 2019-02-10  9:41     ` Stanislaw Gruszka
  2019-02-10 10:22       ` Lorenzo Bianconi
  1 sibling, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-10  9:41 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

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.

Stanislaw

[-- Attachment #2: 0001-mt76x02-usb-mcu-limit-sg-length.patch --]
[-- Type: text/plain, Size: 1161 bytes --]

From bc09bc7fa604019a5ef90184390e7c2a3899869d Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Sun, 10 Feb 2019 08:09:48 +0100
Subject: [PATCH] mt76x02: usb_mcu: limit sg length

When sending fw data we limting urb transfer length by changing buf->len,
while keeping segment length at max_payload value. That may confuse
underlying drivers responsible for DMA.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index da299b8a1334..cfa14506eca6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -287,6 +287,7 @@ __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
 			MT_FCE_DMA_LEN, len << 16);
 
 	buf->len = MT_CMD_HDR_LEN + len + sizeof(info);
+	buf->urb->sg[0].length = buf->len;
 	err = mt76u_submit_buf(&dev->mt76, USB_DIR_OUT,
 			       MT_EP_OUT_INBAND_CMD,
 			       buf, GFP_KERNEL,
-- 
2.19.2


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10  9:41     ` Stanislaw Gruszka
@ 2019-02-10 10:22       ` Lorenzo Bianconi
  2019-02-11  7:44         ` Stanislaw Gruszka
  0 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 10:22 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Stefan Wahren, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	linux-wireless, linux-usb

> 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().
Moreover applying this patch I got the following crash (rpi-5.0.y):

[   38.856623] mt76x0u 1-1.2:1.0: ASIC revision: 76100002 MAC
revision: 76502000
[   38.865999] ------------[ cut here ]------------
[   38.871817] WARNING: CPU: 3 PID: 751 at lib/refcount.c:187
refcount_sub_and_test_checked+0xa4/0xb8
[   38.883277] refcount_t: underflow; use-after-free.
[   38.889358] Modules linked in: mt76x0u(+) mt76x0_common mt76x02_usb
mt76_usb mt76x02_lib mt76 mac80211 bnep hci_uart btbcm serdev
bluetooth ecdh_generic spidev brcmfmac brcmutil sha256_generic
cfg80211 rfkill raspberrypi_hwmon hwmon b
cm2835_v4l2(C) i2c_bcm2835 v4l2_common videobuf2_vmalloc
videobuf2_memops snd_bcm2835(C) spi_bcm2835 videobuf2_v4l2
videobuf2_common snd_pcm videodev snd_timer media snd uio_pdrv_genirq
uio fixed i2c_dev ip_tables x_tables ipv6
[   38.938953] CPU: 3 PID: 751 Comm: systemd-udevd Tainted: G
C        5.0.0-rc5-v7+ #1
[   38.950574] Hardware name: BCM2835
[   38.955503] Backtrace:
[   38.959410] [<8010c91c>] (dump_backtrace) from [<8010cc00>]
(show_stack+0x20/0x24)
[   38.969952]  r6:60000013 r5:ffffffff r4:00000000 r3:a50bade6
[   38.977198] [<8010cbe0>] (show_stack) from [<807ca5f4>]
(dump_stack+0xc8/0x114)
[   38.986183] [<807ca52c>] (dump_stack) from [<8011e65c>]
(__warn+0xf4/0x120)
[   38.994804]  r9:000000bb r8:804d0138 r7:00000009 r6:8099dc84
r5:00000000 r4:b9631b58
[   39.005708] [<8011e568>] (__warn) from [<8011e6d0>]
(warn_slowpath_fmt+0x48/0x50)
[   39.016372]  r9:7f652128 r8:80d1419c r7:80c0bac4 r6:b34a2044
r5:b6096780 r4:00000000
[   39.027368] [<8011e68c>] (warn_slowpath_fmt) from [<804d0138>]
(refcount_sub_and_test_checked+0xa4/0xb8)
[   39.040150]  r3:80c91c25 r2:8099dc94
[   39.045296]  r4:00000000
[   39.049320] [<804d0094>] (refcount_sub_and_test_checked) from
[<804d0164>] (refcount_dec_and_test_checked+0x18/0x1c)
[   39.062966]  r4:b6096780 r3:00000001
[   39.068043] [<804d014c>] (refcount_dec_and_test_checked) from
[<805db49c>] (usb_free_urb+0x20/0x4c)
[   39.080279] [<805db47c>] (usb_free_urb) from [<7f62d804>]
(mt76u_buf_free+0x98/0xac [mt76_usb])
[   39.092224]  r4:00000001 r3:00000001
[   39.097386] [<7f62d76c>] (mt76u_buf_free [mt76_usb]) from
[<7f62def8>] (mt76u_queues_deinit+0x44/0x100 [mt76_usb])
[   39.111016]  r8:b791f600 r7:b3628480 r6:b3628e20 r5:00000001
r4:00000000 r3:00000080
[   39.122039] [<7f62deb4>] (mt76u_queues_deinit [mt76_usb]) from
[<7f650040>] (mt76x0u_cleanup+0x40/0x4c [mt76x0u])
[   39.135636]  r7:b3628480 r6:b791f600 r5:ffffffea r4:b3628e20
[   39.142968] [<7f650000>] (mt76x0u_cleanup [mt76x0u]) from
[<7f650564>] (mt76x0u_probe+0x1f0/0x354 [mt76x0u])
[   39.156063]  r4:b3628e20 r3:00000000
[   39.161202] [<7f650374>] (mt76x0u_probe [mt76x0u]) from
[<805e0b6c>] (usb_probe_interface+0x104/0x240)
[   39.173805]  r7:00000000 r6:7f652034 r5:b6299000 r4:b791f620
[   39.181165] [<805e0a68>] (usb_probe_interface) from [<8056a8bc>]
(really_probe+0x224/0x2f8)
[   39.192856]  r10:b626d800 r9:00000019 r8:7f652034 r7:80d3e124
r6:00000000 r5:80d3e120
[   39.204057]  r4:b791f620 r3:805e0a68
[   39.209269] [<8056a698>] (really_probe) from [<8056ab60>]
(driver_probe_device+0x6c/0x180)
[   39.220854]  r10:b626d800 r9:7f6522c0 r8:b791f620 r7:00000000
r6:7f652034 r5:7f652034
[   39.232057]  r4:b791f620 r3:00000000
[   39.237265] [<8056aaf4>] (driver_probe_device) from [<8056ad54>]
(__driver_attach+0xe0/0xe4)
[   39.248982]  r9:7f6522c0 r8:7f65122c r7:00000000 r6:b791f654
r5:7f652034 r4:b791f620
[   39.260002] [<8056ac74>] (__driver_attach) from [<8056880c>]
(bus_for_each_dev+0x68/0xa0)
[   39.271498]  r6:8056ac74 r5:7f652034 r4:00000000 r3:00000027
[   39.278885] [<805687a4>] (bus_for_each_dev) from [<8056a1cc>]
(driver_attach+0x28/0x30)
[   39.290252]  r6:80c6ddc8 r5:b6096a00 r4:7f652034
[   39.296557] [<8056a1a4>] (driver_attach) from [<80569c24>]
(bus_add_driver+0x194/0x21c)
[   39.307899] [<80569a90>] (bus_add_driver) from [<8056b504>]
(driver_register+0x8c/0x124)
[   39.319328]  r7:80c6ddc8 r6:7f652034 r5:00000000 r4:7f652034
[   39.326730] [<8056b478>] (driver_register) from [<805df510>]
(usb_register_driver+0x74/0x140)
[   39.338655]  r5:00000000 r4:7f652000
[   39.343880] [<805df49c>] (usb_register_driver) from [<7f655024>]
(mt76x0_driver_init+0x24/0x1000 [mt76x0u])
[   39.357002]  r9:00000001 r8:7f652308 r7:00000000 r6:80c08d48
r5:7f655000 r4:7f6522c0
[   39.368143] [<7f655000>] (mt76x0_driver_init [mt76x0u]) from
[<80102f6c>] (do_one_initcall+0x4c/0x210)
[   39.380894] [<80102f20>] (do_one_initcall) from [<801ae63c>]
(do_init_module+0x6c/0x21c)
[   39.392392]  r8:7f652308 r7:80c08d48 r6:b6116880 r5:7f6522c0
r4:7f6522c0
[   39.400876] [<801ae5d0>] (do_init_module) from [<801ad68c>]
(load_module+0x1d10/0x2304)

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).
Are you concerned about increasing code complexity?

Regards,
Lorenzo

>
> Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10  9:29   ` Stanislaw Gruszka
@ 2019-02-10 16:38     ` Stefan Wahren
  2019-02-10 16:52       ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-10 16:38 UTC (permalink / raw)
  To: Stanislaw Gruszka, Lorenzo Bianconi
  Cc: Felix Fietkau, Doug Anderson, Minas Harutyunyan, linux-wireless,
	linux-usb

Hi,

> Stanislaw Gruszka <sgruszka@redhat.com> hat am 10. Februar 2019 um 10:29 geschrieben:
> 
> 
> On Sat, Feb 09, 2019 at 07:46:37PM +0100, Lorenzo Bianconi wrote:
> > > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> > >
> > > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> > >
> > > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > > [   33.454829] usbcore: registered new interface driver mt76x0u
> > >
> > > I took the firmware files from here [2].
> > >
> > > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> > >
> > > mt76x0u 1.1.3:1.0: rx urb failed: -71
> > >
> > > Any chance to narrow down these issues?
> > 
> > Hi Stefan,
> > 
> > could you please test the following series:
> > https://patchwork.kernel.org/cover/10764453/
> > Applying this series I am able to connect to my home AP using an Asus
> > USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> > I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git
> 
> How the patch series fixed the issue since it was reported before that 
> it does not work on 4.19.x which do not use scatter gatter I/O ?
> 
> Or it is regression ? 4.19.x driver works with rasperrypi/linux but -next
> does not ?

sorry for all the confusion (i never tested the foundation kernel). I made my functional tests with arm/multi_v7_defconfig which doesn't need any patches to work.

Here the current results for next-2019-02-08:

arm/multi_v7_defconfig w/o any patches -> wlan0 online
arm64/defconfig w/o any patches -> timeout during firmware upload
arm64/defconfig w Lorenzo's series -> driver probe, but dhcp doesn't work (could be a problem in my arm64 rootfs)
arm/multi_v7_defconfig w Stanislaw's patch -> NULL pointer dereference

I will test linux-4.19 and linux-5.0-rc5 to get a better picture ...

> 
> Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10 16:38     ` Stefan Wahren
@ 2019-02-10 16:52       ` Lorenzo Bianconi
  2019-02-10 17:39         ` Lorenzo Bianconi
  2019-02-11  7:50         ` Stanislaw Gruszka
  0 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 16:52 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

>
> Hi,
>
> > Stanislaw Gruszka <sgruszka@redhat.com> hat am 10. Februar 2019 um 10:29 geschrieben:
> >
> >
> > On Sat, Feb 09, 2019 at 07:46:37PM +0100, Lorenzo Bianconi wrote:
> > > > as already reported here [1], that there are probing issues of mt76 using TP-Link Archer T2UH (wifi USB dongle) on Raspberry Pi 3 B+.
> > > >
> > > > I retested it with recent linux-next 20190208 (aarch64 defconfig + enabling mt76 driver) and got the following output after plugin the wifi dongle:
> > > >
> > > > [   29.778524] usb 1-1.3: new high-speed USB device number 6 using dwc2
> > > > [   31.794581] usb 1-1.3: reset high-speed USB device number 6 using dwc2
> > > > [   31.919916] mt76x0u 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
> > > > [   32.304412] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   32.325724] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   32.346866] dwc2 3f980000.usb: dwc2_update_urb_state(): trimming xfer length
> > > > [   33.410559] mt76x0u 1-1.3:1.0: firmware upload timed out
> > > > [   33.435458] mt76x0u: probe of 1-1.3:1.0 failed with error -110
> > > > [   33.454829] usbcore: registered new interface driver mt76x0u
> > > >
> > > > I took the firmware files from here [2].
> > > >
> > > > Btw there is another issue, if i disconnect the wifi dongle during probe i'm getting a endless flood of the following output:
> > > >
> > > > mt76x0u 1.1.3:1.0: rx urb failed: -71
> > > >
> > > > Any chance to narrow down these issues?
> > >
> > > Hi Stefan,
> > >
> > > could you please test the following series:
> > > https://patchwork.kernel.org/cover/10764453/
> > > Applying this series I am able to connect to my home AP using an Asus
> > > USB-AC51 (mt76x0u) connected to a Raspberry Pi 3 B+.
> > > I am running rpi-5.0.y branch of https://github.com/raspberrypi/linux.git
> >
> > How the patch series fixed the issue since it was reported before that
> > it does not work on 4.19.x which do not use scatter gatter I/O ?
> >
> > Or it is regression ? 4.19.x driver works with rasperrypi/linux but -next
> > does not ?
>
> sorry for all the confusion (i never tested the foundation kernel). I made my functional tests with arm/multi_v7_defconfig which doesn't need any patches to work.
>
> Here the current results for next-2019-02-08:
>
> arm/multi_v7_defconfig w/o any patches -> wlan0 online
> arm64/defconfig w/o any patches -> timeout during firmware upload
> arm64/defconfig w Lorenzo's series -> driver probe, but dhcp doesn't work (could be a problem in my arm64 rootfs)
> arm/multi_v7_defconfig w Stanislaw's patch -> NULL pointer dereference
>

I am looking at this issue, it is not related to Stanislaw's patch,
there is a bug in the error code path.
Anyway we will need to avoid SG since dwc_otg controller does not support it

Regards,
Lorenzo

> I will test linux-4.19 and linux-5.0-rc5 to get a better picture ...
>
> >
> > Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10 16:52       ` Lorenzo Bianconi
@ 2019-02-10 17:39         ` Lorenzo Bianconi
  2019-02-11  7:50         ` Stanislaw Gruszka
  1 sibling, 0 replies; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 17:39 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]

> > sorry for all the confusion (i never tested the foundation kernel). I made my functional tests with arm/multi_v7_defconfig which doesn't need any patches to work.
> >
> > Here the current results for next-2019-02-08:
> >
> > arm/multi_v7_defconfig w/o any patches -> wlan0 online
> > arm64/defconfig w/o any patches -> timeout during firmware upload
> > arm64/defconfig w Lorenzo's series -> driver probe, but dhcp doesn't work (could be a problem in my arm64 rootfs)
> > arm/multi_v7_defconfig w Stanislaw's patch -> NULL pointer dereference
> >

Hi Stefan,

Could you please try the following patch? It should fix the  NULL
pointer dereference crash.
Anyway in order to enable mt76x0u on rpi3 we will need the RFC series

Regards,
Lorenzo

>
> I am looking at this issue, it is not related to Stanislaw's patch,
> there is a bug in the error code path.
> Anyway we will need to avoid SG since dwc_otg controller does not support it
>
> Regards,
> Lorenzo
>
> > I will test linux-4.19 and linux-5.0-rc5 to get a better picture ...
> >
> > >
> > > Stanislaw

[-- Attachment #2: 0001-mt76-fix-NULL-pointer-dereference-in-mt76u_mcu_deini.patch --]
[-- Type: text/x-patch, Size: 2319 bytes --]

From 181a696adeeb77bc4ac05190763240735234fdbb Mon Sep 17 00:00:00 2001
Message-Id: <181a696adeeb77bc4ac05190763240735234fdbb.1549820280.git.me@lorenzobianconi.net>
In-Reply-To: <cover.1549820280.git.me@lorenzobianconi.net>
References: <cover.1549820280.git.me@lorenzobianconi.net>
From: Lorenzo Bianconi <me@lorenzobianconi.net>
Date: Sun, 10 Feb 2019 18:37:38 +0100
Subject: [PATCH] mt76: fix NULL pointer dereference in mt76u_mcu_deinit

Signed-off-by: Lorenzo Bianconi <me@lorenzobianconi.net>
---
 drivers/net/wireless/mediatek/mt76/usb.c     | 15 ++++-----------
 drivers/net/wireless/mediatek/mt76/usb_mcu.c |  8 +++++---
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index b061263453d4..15ef1a8754ab 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -765,8 +765,7 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
 		if (!q->entry)
 			return -ENOMEM;
 
-		q->ndesc = MT_NUM_TX_ENTRIES;
-		for (j = 0; j < q->ndesc; j++) {
+		for (j = 0; j < MT_NUM_TX_ENTRIES; j++) {
 			buf = &q->entry[j].ubuf;
 			buf->dev = dev;
 
@@ -778,6 +777,7 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
 			if (!buf->urb->sg)
 				return -ENOMEM;
 		}
+		q->ndesc = MT_NUM_TX_ENTRIES;
 	}
 	return 0;
 }
@@ -838,16 +838,9 @@ int mt76u_alloc_queues(struct mt76_dev *dev)
 
 	err = mt76u_alloc_rx(dev);
 	if (err < 0)
-		goto err;
-
-	err = mt76u_alloc_tx(dev);
-	if (err < 0)
-		goto err;
+		return err;
 
-	return 0;
-err:
-	mt76u_queues_deinit(dev);
-	return err;
+	return mt76u_alloc_tx(dev);
 }
 EXPORT_SYMBOL_GPL(mt76u_alloc_queues);
 
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 036be4163e69..9527e1216f3d 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -48,9 +48,11 @@ EXPORT_SYMBOL_GPL(mt76u_mcu_init_rx);
 
 void mt76u_mcu_deinit(struct mt76_dev *dev)
 {
-	struct mt76_usb *usb = &dev->usb;
+	struct mt76u_buf *buf = &dev->usb.mcu.res;
 
-	usb_kill_urb(usb->mcu.res.urb);
-	mt76u_buf_free(&usb->mcu.res);
+	if (buf->urb) {
+		usb_kill_urb(buf->urb);
+		mt76u_buf_free(buf);
+	}
 }
 EXPORT_SYMBOL_GPL(mt76u_mcu_deinit);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10 10:22       ` Lorenzo Bianconi
@ 2019-02-11  7:44         ` Stanislaw Gruszka
  2019-02-11 10:04           ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-11  7:44 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stefan Wahren, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	linux-wireless, linux-usb

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-10 16:52       ` Lorenzo Bianconi
  2019-02-10 17:39         ` Lorenzo Bianconi
@ 2019-02-11  7:50         ` Stanislaw Gruszka
  2019-02-11  8:08           ` Stefan Wahren
  1 sibling, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-11  7:50 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stefan Wahren, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	linux-wireless, linux-usb

On Sun, Feb 10, 2019 at 05:52:30PM +0100, Lorenzo Bianconi wrote:
> Anyway we will need to avoid SG since dwc_otg controller does not support it

What does it mean the dwc_otg does not support SG ? Please be more specific.

Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11  7:50         ` Stanislaw Gruszka
@ 2019-02-11  8:08           ` Stefan Wahren
  2019-02-11  9:52             ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-11  8:08 UTC (permalink / raw)
  To: Stanislaw Gruszka, Lorenzo Bianconi
  Cc: Felix Fietkau, Doug Anderson, Minas Harutyunyan, linux-wireless,
	linux-usb

Hi,

Am 11.02.19 um 08:50 schrieb Stanislaw Gruszka:
> On Sun, Feb 10, 2019 at 05:52:30PM +0100, Lorenzo Bianconi wrote:
>> Anyway we will need to avoid SG since dwc_otg controller does not support it

this isn't correct. AFAIK the dwc2 host driver doesn't implement it,
which doesn't mean the controller does not support it.

Stefan

> What does it mean the dwc_otg does not support SG ? Please be more specific.
>
> Stanislaw


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11  8:08           ` Stefan Wahren
@ 2019-02-11  9:52             ` Lorenzo Bianconi
  0 siblings, 0 replies; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-11  9:52 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

> Hi,
> 
> Am 11.02.19 um 08:50 schrieb Stanislaw Gruszka:
> > On Sun, Feb 10, 2019 at 05:52:30PM +0100, Lorenzo Bianconi wrote:
> >> Anyway we will need to avoid SG since dwc_otg controller does not support it
> 
> this isn't correct. AFAIK the dwc2 host driver doesn't implement it,
> which doesn't mean the controller does not support it.

This is what I meant :)

Regards,
Lorenzo

> 
> Stefan
> 
> > What does it mean the dwc_otg does not support SG ? Please be more specific.
> >
> > Stanislaw
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11  7:44         ` Stanislaw Gruszka
@ 2019-02-11 10:04           ` Lorenzo Bianconi
  2019-02-11 10:33             ` Stefan Wahren
  0 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-11 10:04 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Stefan Wahren, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	linux-wireless, linux-usb

> 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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 10:04           ` Lorenzo Bianconi
@ 2019-02-11 10:33             ` Stefan Wahren
  2019-02-11 11:06               ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-11 10:33 UTC (permalink / raw)
  To: Lorenzo Bianconi, Stanislaw Gruszka
  Cc: Felix Fietkau, Doug Anderson, Minas Harutyunyan, linux-wireless,
	linux-usb

Hi,

Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
>> 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
>
i'm not sure that you understand my mail [1] with the summary of my test
results.

In case of using the arm/multi_v7_defconfig (32 bit) the mt76 works like
a charm without your sg avoid patch series, but the arm64/defconfig (64
bit) requires the series to probe at least. So i wouldn't conclude from
the fact that dwc2 doesn't support SG any probing issues on arm64. So we
need to investigate which config option triggers the problem.

Stefan

[1] - https://marc.info/?l=linux-usb&m=154981675724078


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 10:33             ` Stefan Wahren
@ 2019-02-11 11:06               ` Lorenzo Bianconi
  2019-02-11 14:04                 ` Stefan Wahren
  0 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-11 11:06 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

> Hi,
> 
> Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
> >> 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
> >
> i'm not sure that you understand my mail [1] with the summary of my test
> results.
> 

Yes right, I did not get it sorry :)
as indicated here https://www.raspberrypi.org/documentation/linux/kernel/building.md
I am using bcm2709_defconfig config (using it I spotted the mt76 crashes and
probe failure)

Regards,
Lorenzo

> In case of using the arm/multi_v7_defconfig (32 bit) the mt76 works like
> a charm without your sg avoid patch series, but the arm64/defconfig (64
> bit) requires the series to probe at least. So i wouldn't conclude from
> the fact that dwc2 doesn't support SG any probing issues on arm64. So we
> need to investigate which config option triggers the problem.
> 
> Stefan
> 
> [1] - https://marc.info/?l=linux-usb&m=154981675724078
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 11:06               ` Lorenzo Bianconi
@ 2019-02-11 14:04                 ` Stefan Wahren
  2019-02-11 15:10                   ` Lorenzo Bianconi
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-11 14:04 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

Hi Lorenzo,

Am 11.02.19 um 12:06 schrieb Lorenzo Bianconi:
>> Hi,
>>
>> Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
>>>> 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
>>>
>> i'm not sure that you understand my mail [1] with the summary of my test
>> results.
>>
> Yes right, I did not get it sorry :)
> as indicated here https://www.raspberrypi.org/documentation/linux/kernel/building.md
> I am using bcm2709_defconfig config (using it I spotted the mt76 crashes and
> probe failure)

no problem, at the beginning this could be very confusing. I only want
to clarify that this documentation refers to the vendor kernel (with a
different USB host driver) of the Raspberry Pi Foundation.

All my results refers to the mainline kernel we all should talk about. I
started a gist which try to describe the mainline variant:
https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75

Maybe this could be helpful.

Stefan

> Regards,
> Lorenzo
>
>> In case of using the arm/multi_v7_defconfig (32 bit) the mt76 works like
>> a charm without your sg avoid patch series, but the arm64/defconfig (64
>> bit) requires the series to probe at least. So i wouldn't conclude from
>> the fact that dwc2 doesn't support SG any probing issues on arm64. So we
>> need to investigate which config option triggers the problem.
>>
>> Stefan
>>
>> [1] - https://marc.info/?l=linux-usb&m=154981675724078
>>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 14:04                 ` Stefan Wahren
@ 2019-02-11 15:10                   ` Lorenzo Bianconi
  2019-02-11 15:27                     ` Stefan Wahren
  0 siblings, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-11 15:10 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

> Hi Lorenzo,
> 
> Am 11.02.19 um 12:06 schrieb Lorenzo Bianconi:
> >> Hi,
> >>
> >> Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
> >>>> 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
> >>>
> >> i'm not sure that you understand my mail [1] with the summary of my test
> >> results.
> >>
> > Yes right, I did not get it sorry :)
> > as indicated here https://www.raspberrypi.org/documentation/linux/kernel/building.md
> > I am using bcm2709_defconfig config (using it I spotted the mt76 crashes and
> > probe failure)
> 
> no problem, at the beginning this could be very confusing. I only want
> to clarify that this documentation refers to the vendor kernel (with a
> different USB host driver) of the Raspberry Pi Foundation.
> 
> All my results refers to the mainline kernel we all should talk about. I
> started a gist which try to describe the mainline variant:
> https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75

So to summarize:
- Raspberry Pi Foundation kernel works just with RFC series
- mainline kernel works out of the box

is my understanding correct? I am still considering adding a legacy mode since
there will not be any regression using it instead of SG I/O with just one SG
buffer

Regards,
Lorenzo

> 
> Maybe this could be helpful.
> 
> Stefan
> 
> > Regards,
> > Lorenzo
> >
> >> In case of using the arm/multi_v7_defconfig (32 bit) the mt76 works like
> >> a charm without your sg avoid patch series, but the arm64/defconfig (64
> >> bit) requires the series to probe at least. So i wouldn't conclude from
> >> the fact that dwc2 doesn't support SG any probing issues on arm64. So we
> >> need to investigate which config option triggers the problem.
> >>
> >> Stefan
> >>
> >> [1] - https://marc.info/?l=linux-usb&m=154981675724078
> >>

^ permalink raw reply	[flat|nested] 37+ messages in thread

* [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 15:10                   ` Lorenzo Bianconi
@ 2019-02-11 15:27                     ` Stefan Wahren
  2019-02-11 15:57                       ` Lorenzo Bianconi
  2019-02-11 17:22                       ` Stanislaw Gruszka
  0 siblings, 2 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-11 15:27 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

Hi,

Am 11.02.19 um 16:10 schrieb Lorenzo Bianconi:
>> Hi Lorenzo,
>>
>> Am 11.02.19 um 12:06 schrieb Lorenzo Bianconi:
>>>> Hi,
>>>>
>>>> Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
>>>>>> 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
>>>>>
>>>> i'm not sure that you understand my mail [1] with the summary of my test
>>>> results.
>>>>
>>> Yes right, I did not get it sorry :)
>>> as indicated here https://www.raspberrypi.org/documentation/linux/kernel/building.md
>>> I am using bcm2709_defconfig config (using it I spotted the mt76 crashes and
>>> probe failure)
>> no problem, at the beginning this could be very confusing. I only want
>> to clarify that this documentation refers to the vendor kernel (with a
>> different USB host driver) of the Raspberry Pi Foundation.
>>
>> All my results refers to the mainline kernel we all should talk about. I
>> started a gist which try to describe the mainline variant:
>> https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75
> So to summarize:
> - Raspberry Pi Foundation kernel works just with RFC series
> - mainline kernel works out of the box
>
> is my understanding correct? 

not really.

Compiling the mainline kernel with arm/multi_v7_defconfig it works.
Using the same kernel but with arm64/defconfig doesn't work. But i don't
think this is a 32/64 bit issue. The arm64 defconfig is much more
complex (e.g. enables more IOMMU stuff).

Regards
Stefan


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 15:27                     ` Stefan Wahren
@ 2019-02-11 15:57                       ` Lorenzo Bianconi
  2019-02-13  7:05                         ` Stefan Wahren
  2019-02-11 17:22                       ` Stanislaw Gruszka
  1 sibling, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-11 15:57 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

> Hi,
> 
> Am 11.02.19 um 16:10 schrieb Lorenzo Bianconi:
> >> Hi Lorenzo,
> >>
> >> Am 11.02.19 um 12:06 schrieb Lorenzo Bianconi:
> >>>> Hi,
> >>>>
> >>>> Am 11.02.19 um 11:04 schrieb Lorenzo Bianconi:
> >>>>>> 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
> >>>>>
> >>>> i'm not sure that you understand my mail [1] with the summary of my test
> >>>> results.
> >>>>
> >>> Yes right, I did not get it sorry :)
> >>> as indicated here https://www.raspberrypi.org/documentation/linux/kernel/building.md
> >>> I am using bcm2709_defconfig config (using it I spotted the mt76 crashes and
> >>> probe failure)
> >> no problem, at the beginning this could be very confusing. I only want
> >> to clarify that this documentation refers to the vendor kernel (with a
> >> different USB host driver) of the Raspberry Pi Foundation.
> >>
> >> All my results refers to the mainline kernel we all should talk about. I
> >> started a gist which try to describe the mainline variant:
> >> https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75
> > So to summarize:
> > - Raspberry Pi Foundation kernel works just with RFC series
> > - mainline kernel works out of the box
> >
> > is my understanding correct? 
> 
> not really.
> 
> Compiling the mainline kernel with arm/multi_v7_defconfig it works.
> Using the same kernel but with arm64/defconfig doesn't work. But i don't
> think this is a 32/64 bit issue. The arm64 defconfig is much more
> complex (e.g. enables more IOMMU stuff).

thx for the clarification :)

Regards,
Lorenzo

> 
> Regards
> Stefan
> 

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 15:27                     ` Stefan Wahren
  2019-02-11 15:57                       ` Lorenzo Bianconi
@ 2019-02-11 17:22                       ` Stanislaw Gruszka
  1 sibling, 0 replies; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-11 17:22 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

On Mon, Feb 11, 2019 at 04:27:40PM +0100, Stefan Wahren wrote:
> >> All my results refers to the mainline kernel we all should talk about. I
> >> started a gist which try to describe the mainline variant:
> >> https://gist.github.com/lategoodbye/c7317a42bf7f9c07f5a91baed8c68f75
> > So to summarize:
> > - Raspberry Pi Foundation kernel works just with RFC series
> > - mainline kernel works out of the box
> >
> > is my understanding correct? 
> 
> not really.
> 
> Compiling the mainline kernel with arm/multi_v7_defconfig it works.
> Using the same kernel but with arm64/defconfig doesn't work. But i don't
> think this is a 32/64 bit issue. The arm64 defconfig is much more
> complex (e.g. enables more IOMMU stuff).

One possible thing that could be broken with IOMMU is allocating
big buffers via page_fraq_alloc(). Theoretically that should work,
but who knows. You can check my patch posted recently, it make
the driver stop doing big allocations via page_frag_alloc():

https://lore.kernel.org/linux-wireless/1549872974-7268-1-git-send-email-sgruszka@redhat.com/

Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-11 15:57                       ` Lorenzo Bianconi
@ 2019-02-13  7:05                         ` Stefan Wahren
  0 siblings, 0 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-13  7:05 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stanislaw Gruszka, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, linux-wireless, linux-usb

Hi,

> Lorenzo Bianconi <lorenzo.bianconi@redhat.com> hat am 11. Februar 2019 um 16:57 geschrieben:
> 
> 
> > 
> > Compiling the mainline kernel with arm/multi_v7_defconfig it works.
> > Using the same kernel but with arm64/defconfig doesn't work. But i don't
> > think this is a 32/64 bit issue. The arm64 defconfig is much more
> > complex (e.g. enables more IOMMU stuff).
> 
> thx for the clarification :)
> 

i was busy to enable the mt76xx wifi driver in my build system. Usually i handle this via scripts/config but i didn't manage to enable the driver. Finally i gave up and enabled it via defconfig patch.

Here are my test results for Raspberry Pi 3 B+ (without any additional patches):

multi_v7_defconfig 5.0.0-rc6 -> firmware timeout
multi_v7_defconfig next-2019-02-12 -> firmware timeout
arm64_defconfig 5.0.0-rc6 -> vendor request 07: -110 (timeout)
arm64_defconfig next-2019-02-12 -> vendor request 07: -110 (timeout)

Shame on me for the wrong result before (assume to not properly cleanup the kernel modules on sd card). So there is no config option in arm64_defconfig which breaks mt76xx.

I will try the recent patches later.

Thanks
Stefan

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-16 14:07                 ` Stanislaw Gruszka
@ 2019-02-16 19:17                   ` Stefan Wahren
  0 siblings, 0 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-16 19:17 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

Hi Stanislaw,

> Stanislaw Gruszka <sgruszka@redhat.com> hat am 16. Februar 2019 um 15:07 geschrieben:
> 
> 
> On Sat, Feb 16, 2019 at 12:05:18PM +0100, Stefan Wahren wrote:
> > sorry for the delay, but i do this all in my spare time.
> 
> Stefan, thanks for sacrifing your spare time for testing this!
> 
> > The results for your recent patch series are better (no firmware timeout), but still no working wifi and still a warning:
> > https://gist.github.com/lategoodbye/c4864e446821717419cbe65df07f8d8d
> > 
> > I've identified the reason for the warning in dwc2:
> > 
> > /*
> >  * We assume that DMA is always aligned in non-split
> >  * case or split out case. Warn if not.
> >  */
> > WARN_ON_ONCE(hsotg->params.host_dma && (chan->xfer_dma & 0x3));
> 
> I think I understand why that happen, we first allocate 1024 mcu 
> buffer then standard 4096 rx buffers, that couse 4096 buffers are
> not contained in single page and need split by dwc2 driver.

this is a misunderstanding. The warning is about memory alignment to 32 bit addresses, not about page alignment. This is a typical ARM restriction. Maybe we need to make sure in mt76 that the DMA buffer needs to be aligned. But it's also possible that the warning isn't the root cause of our problem.

> 
> Attached patch should fix this, plese test, thanks in advance.

Anyway i tested the following patch combinations against next with the same results as 1,2,3 (no wifi, alignment warning):
1,3
1,2,3,4

> 
> > Btw i can confirm a regression was introduced after 4.19, because in 4.19 there was no firmware timeout but even no working wifi:
> 
> You ment 'no working wifi' or 'working wifi'?

Wifi is broken in 4.19, 4.20, 5.0 and next. It only worked with Lorenzo's SG avoid patches so far. Btw the regression (firmware timeout) started in 4.20. I also tested it today.

Thanks
Stefan

> 
> Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-16 11:05               ` Stefan Wahren
@ 2019-02-16 14:07                 ` Stanislaw Gruszka
  2019-02-16 19:17                   ` Stefan Wahren
  0 siblings, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-16 14:07 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

On Sat, Feb 16, 2019 at 12:05:18PM +0100, Stefan Wahren wrote:
> sorry for the delay, but i do this all in my spare time.

Stefan, thanks for sacrifing your spare time for testing this!

> The results for your recent patch series are better (no firmware timeout), but still no working wifi and still a warning:
> https://gist.github.com/lategoodbye/c4864e446821717419cbe65df07f8d8d
> 
> I've identified the reason for the warning in dwc2:
> 
> /*
>  * We assume that DMA is always aligned in non-split
>  * case or split out case. Warn if not.
>  */
> WARN_ON_ONCE(hsotg->params.host_dma && (chan->xfer_dma & 0x3));

I think I understand why that happen, we first allocate 1024 mcu 
buffer then standard 4096 rx buffers, that couse 4096 buffers are
not contained in single page and need split by dwc2 driver.

Attached patch should fix this, plese test, thanks in advance.

> Btw i can confirm a regression was introduced after 4.19, because in 4.19 there was no firmware timeout but even no working wifi:

You ment 'no working wifi' or 'working wifi'?

Stanislaw

[-- Attachment #2: 0004-mt76usb-allocate-page-contained-rx-buffers.patch --]
[-- Type: text/plain, Size: 5102 bytes --]

From 35dadd09bd3193b33b10f56e0210da680c6d915f Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Sat, 16 Feb 2019 14:53:19 +0100
Subject: [PATCH] mt76usb: allocate page contained rx buffers

If we first allocate 1024 bytes buffer and then 4096 bytes
buffer via page_frag_alloc() the second buffer will be crossing
page boundaries what is most likely not appropriate for
dma_map_{sg/page} and make buffer split misalign issues on
dwc2 usb host driver.

Since patch changed arguments of mt76u_buf_alloc() function I also
changed name to indicate it is only used for RX allocation.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h    |  5 ++---
 drivers/net/wireless/mediatek/mt76/usb.c     | 24 ++++++++++--------------
 drivers/net/wireless/mediatek/mt76/usb_mcu.c |  4 +---
 3 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 364f3571c033..9e24f603664a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -364,7 +364,6 @@ enum mt76u_out_ep {
 #define MT_SG_MAX_SIZE		8
 #define MT_NUM_TX_ENTRIES	256
 #define MT_NUM_RX_ENTRIES	128
-#define MCU_RESP_URB_SIZE	1024
 struct mt76_usb {
 	struct mutex usb_ctrl_mtx;
 	u8 data[32];
@@ -753,8 +752,8 @@ void mt76u_single_wr(struct mt76_dev *dev, const u8 req,
 		     const u16 offset, const u32 val);
 int mt76u_init(struct mt76_dev *dev, struct usb_interface *intf);
 void mt76u_deinit(struct mt76_dev *dev);
-int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
-		    int nsgs, int len, int sglen, gfp_t gfp);
+int mt76u_rx_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf, int nsgs,
+		       gfp_t gfp);
 void mt76u_buf_free(struct mt76u_buf *buf);
 int mt76u_submit_buf(struct mt76_dev *dev, int dir, int index,
 		     struct mt76u_buf *buf, gfp_t gfp,
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index de906f07e85a..472642ef25a4 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -271,11 +271,11 @@ mt76u_set_endpoints(struct usb_interface *intf,
 }
 
 static int
-mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
-		 int nsgs, int len, int sglen)
+mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf, int nsgs)
 {
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
 	struct urb *urb = buf->urb;
+	int sglen = SKB_WITH_OVERHEAD(q->buf_size);
 	int i;
 
 	spin_lock_bh(&q->rx_page_lock);
@@ -284,7 +284,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 		void *data;
 		int offset;
 
-		data = page_frag_alloc(&q->rx_page, len, GFP_ATOMIC);
+		data = page_frag_alloc(&q->rx_page, q->buf_size, GFP_ATOMIC);
 		if (!data)
 			break;
 
@@ -309,8 +309,8 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 	return i ? : -ENOMEM;
 }
 
-int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
-		    int nsgs, int len, int sglen, gfp_t gfp)
+int mt76u_rx_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
+		       int nsgs, gfp_t gfp)
 {
 	buf->urb = usb_alloc_urb(0, gfp);
 	if (!buf->urb)
@@ -325,7 +325,7 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 	buf->dev = dev;
 	buf->num_sgs = nsgs;
 
-	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
+	return mt76u_fill_rx_sg(dev, buf, nsgs);
 }
 
 void mt76u_buf_free(struct mt76u_buf *buf)
@@ -485,7 +485,7 @@ static void mt76u_rx_tasklet(unsigned long data)
 {
 	struct mt76_dev *dev = (struct mt76_dev *)data;
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
-	int err, nsgs, buf_len = q->buf_size;
+	int err, nsgs;
 	struct mt76u_buf *buf;
 
 	rcu_read_lock();
@@ -497,9 +497,7 @@ static void mt76u_rx_tasklet(unsigned long data)
 
 		nsgs = mt76u_process_rx_entry(dev, buf->urb);
 		if (nsgs > 0) {
-			err = mt76u_fill_rx_sg(dev, buf, nsgs,
-					       buf_len,
-					       SKB_WITH_OVERHEAD(buf_len));
+			err = mt76u_fill_rx_sg(dev, buf, nsgs);
 			if (err < 0)
 				break;
 		}
@@ -556,10 +554,8 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 	}
 
 	for (i = 0; i < MT_NUM_RX_ENTRIES; i++) {
-		err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
-				      nsgs, q->buf_size,
-				      SKB_WITH_OVERHEAD(q->buf_size),
-				      GFP_KERNEL);
+		err = mt76u_rx_buf_alloc(dev, &q->entry[i].ubuf, nsgs,
+					 GFP_KERNEL);
 		if (err < 0)
 			return err;
 	}
diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 036be4163e69..7c43738f5a6e 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -29,9 +29,7 @@ int mt76u_mcu_init_rx(struct mt76_dev *dev)
 	struct mt76_usb *usb = &dev->usb;
 	int err;
 
-	err = mt76u_buf_alloc(dev, &usb->mcu.res, 1,
-			      MCU_RESP_URB_SIZE, MCU_RESP_URB_SIZE,
-			      GFP_KERNEL);
+	err = mt76u_rx_buf_alloc(dev, &usb->mcu.res, 1, GFP_KERNEL);
 	if (err < 0)
 		return err;
 
-- 
2.7.5


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-15  7:12             ` Stanislaw Gruszka
@ 2019-02-16 11:05               ` Stefan Wahren
  2019-02-16 14:07                 ` Stanislaw Gruszka
  0 siblings, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-16 11:05 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

Hi Stanislaw,

> Stanislaw Gruszka <sgruszka@redhat.com> hat am 15. Februar 2019 um 08:12 geschrieben:
> 
> 
> On Thu, Feb 14, 2019 at 10:48:15AM +0100, Stefan Wahren wrote:
> > Hi Stanislaw,
> > 
> > Am 14.02.19 um 10:25 schrieb Stanislaw Gruszka:
> > > On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
> > >> 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
> > > I think this is due to urb->transfer_length and sg[0]->length mismatch,
> > > which should be addressed by my other patch. I'm attaching the patch
> > > rebased on -next with this line integrated, please test. 
> > >
> > > But there could be other bug's in mt76-usb SG code.
> 
> I found another bug in mt76usb SG code. We set sg->offset bigger than
> PAGE_SIZE that actually make sg point to memory in different page than
> sg->page. Correcting this with another patch that avoid using sg
> mapping with sg->length > PAGE_SIZE, fixed mt76x0u with AMD IOMMU:
> https://bugzilla.kernel.org/show_bug.cgi?id=202241
> 
> I'm attaching 3 patches, they should also fix issue on rpi.
> It's also possible that only 2 patches are sufficient:
> 
> 0001-mt76usb-do-not-set-urb-num_sgs-to-1-for-non-SG-usb-h.patch
> 0003-mt76usb-do-not-use-compound-head-page-for-SG-I-O.patch
> 
> to fix the issue, if doing dma_map_{page,sg} is fine with
> sg->offset == 0 and sg->length bigger than one page.
> 
> Please test, thanks.
> 

sorry for the delay, but i do this all in my spare time.

The results for your recent patch series are better (no firmware timeout), but still no working wifi and still a warning:
https://gist.github.com/lategoodbye/c4864e446821717419cbe65df07f8d8d

I've identified the reason for the warning in dwc2:

/*
 * We assume that DMA is always aligned in non-split
 * case or split out case. Warn if not.
 */
WARN_ON_ONCE(hsotg->params.host_dma && (chan->xfer_dma & 0x3));

Btw i can confirm a regression was introduced after 4.19, because in 4.19 there was no firmware timeout but even no working wifi:

Feb 15 19:10:51 raspberrypi kernel: [   79.818414] usb 1-1.3: new high-speed USB device number 6 using dwc2
Feb 15 19:10:51 raspberrypi kernel: [   80.178470] usb 1-1.3: reset high-speed USB device number 6 using dwc2
Feb 15 19:10:51 raspberrypi kernel: [   80.314388] mt76x0 1-1.3:1.0: ASIC revision: 76100002 MAC revision: 76502000
Feb 15 19:10:52 raspberrypi kernel: [   81.118751] BBP version f000f200
Feb 15 19:10:52 raspberrypi kernel: [   81.152995] mt76x0 1-1.3:1.0: EEPROM ver:02 fae:01
Feb 15 19:10:52 raspberrypi kernel: [   81.153232] mt76x0 1-1.3:1.0: EEPROM country region 01 (channels 1-13)
Feb 15 19:10:52 raspberrypi kernel: [   81.176968] ieee80211 phy0: Selected rate control algorithm 'minstrel_ht'
Feb 15 19:10:52 raspberrypi kernel: [   81.178255] usbcore: registered new interface driver mt76x0
Feb 15 19:10:53 raspberrypi kernel: [   81.578982] IPv6: ADDRCONF(NETDEV_UP): wlan0: link is not ready

Stefan

> Stanislaw

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-14  9:48           ` Stefan Wahren
  2019-02-14  9:54             ` Stanislaw Gruszka
@ 2019-02-15  7:12             ` Stanislaw Gruszka
  2019-02-16 11:05               ` Stefan Wahren
  1 sibling, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-15  7:12 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]

On Thu, Feb 14, 2019 at 10:48:15AM +0100, Stefan Wahren wrote:
> Hi Stanislaw,
> 
> Am 14.02.19 um 10:25 schrieb Stanislaw Gruszka:
> > On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
> >> 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
> > I think this is due to urb->transfer_length and sg[0]->length mismatch,
> > which should be addressed by my other patch. I'm attaching the patch
> > rebased on -next with this line integrated, please test. 
> >
> > But there could be other bug's in mt76-usb SG code.

I found another bug in mt76usb SG code. We set sg->offset bigger than
PAGE_SIZE that actually make sg point to memory in different page than
sg->page. Correcting this with another patch that avoid using sg
mapping with sg->length > PAGE_SIZE, fixed mt76x0u with AMD IOMMU:
https://bugzilla.kernel.org/show_bug.cgi?id=202241

I'm attaching 3 patches, they should also fix issue on rpi.
It's also possible that only 2 patches are sufficient:

0001-mt76usb-do-not-set-urb-num_sgs-to-1-for-non-SG-usb-h.patch
0003-mt76usb-do-not-use-compound-head-page-for-SG-I-O.patch

to fix the issue, if doing dma_map_{page,sg} is fine with
sg->offset == 0 and sg->length bigger than one page.

Please test, thanks.

Stanislaw

[-- Attachment #2: 0001-mt76usb-do-not-set-urb-num_sgs-to-1-for-non-SG-usb-h.patch --]
[-- Type: text/plain, Size: 5445 bytes --]

From 49654f00b5071602a9182f0cb488f346f01ec5ac Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Fri, 15 Feb 2019 07:39:00 +0100
Subject: [PATCH v5 1/3] 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  | 57 +++++++++++++++++++------------
 2 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2bb9db4ed80a..97ad0270f8a6 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 6a2507524c6c..4f92732506cc 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -297,14 +297,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;
 }
@@ -323,6 +323,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);
 }
@@ -333,15 +334,16 @@ void mt76u_buf_free(struct mt76u_buf *buf)
 	struct urb *urb = buf->urb;
 	int i;
 
-	for (i = 0; i < urb->num_sgs; i++)
+	for (i = 0; i < buf->num_sgs; i++)
 		skb_free_frag(sg_virt(&urb->sg[i]));
 	usb_free_urb(buf->urb);
 }
 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);
@@ -352,12 +354,28 @@ 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);
-	trace_submit_urb(dev, buf->urb);
+	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);
 
 static inline struct mt76u_buf
@@ -667,10 +685,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;
@@ -679,7 +698,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);
 }
@@ -689,12 +709,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)
@@ -708,13 +725,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.7.5


[-- Attachment #3: 0002-mt76x02u-use-usb_bulk_msg-to-upload-firmware.patch --]
[-- Type: text/plain, Size: 5701 bytes --]

From 3307dc2b47ea4442adebf09466e6bb9fdd6479db Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Mon, 11 Feb 2019 09:08:40 +0100
Subject: [PATCH v5 2/3] mt76x02u: use usb_bulk_msg to upload firmware

We don't need to send firmware data asynchronously, much simpler is just
use synchronous usb_bulk_msg().

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h          | 13 ++++++
 .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   | 52 +++++++---------------
 drivers/net/wireless/mediatek/mt76/usb.c           |  1 -
 3 files changed, 29 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 97ad0270f8a6..364f3571c033 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -733,6 +733,19 @@ static inline bool mt76u_check_sg(struct mt76_dev *dev)
 		 udev->speed == USB_SPEED_WIRELESS));
 }
 
+static inline int
+mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int timeout)
+{
+	struct usb_interface *intf = to_usb_interface(dev->dev);
+	struct usb_device *udev = interface_to_usbdev(intf);
+	struct mt76_usb *usb = &dev->usb;
+	unsigned int pipe;
+	int sent;
+
+	pipe = usb_sndbulkpipe(udev, usb->out_ep[MT_EP_OUT_INBAND_CMD]);
+	return usb_bulk_msg(udev, pipe, data, len, &sent, timeout);
+}
+
 int mt76u_vendor_request(struct mt76_dev *dev, u8 req,
 			 u8 req_type, u16 val, u16 offset,
 			 void *buf, size_t len);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index 6db789f90269..2ca393e267af 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -121,18 +121,14 @@ static int
 __mt76x02u_mcu_send_msg(struct mt76_dev *dev, struct sk_buff *skb,
 			int cmd, bool wait_resp)
 {
-	struct usb_interface *intf = to_usb_interface(dev->dev);
-	struct usb_device *udev = interface_to_usbdev(intf);
 	struct mt76_usb *usb = &dev->usb;
-	unsigned int pipe;
-	int ret, sent;
+	int ret;
 	u8 seq = 0;
 	u32 info;
 
 	if (test_bit(MT76_REMOVED, &dev->state))
 		return 0;
 
-	pipe = usb_sndbulkpipe(udev, usb->out_ep[MT_EP_OUT_INBAND_CMD]);
 	if (wait_resp) {
 		seq = ++usb->mcu.msg_seq & 0xf;
 		if (!seq)
@@ -146,7 +142,7 @@ __mt76x02u_mcu_send_msg(struct mt76_dev *dev, struct sk_buff *skb,
 	if (ret)
 		return ret;
 
-	ret = usb_bulk_msg(udev, pipe, skb->data, skb->len, &sent, 500);
+	ret = mt76u_bulk_msg(dev, skb->data, skb->len, 500);
 	if (ret)
 		return ret;
 
@@ -268,14 +264,12 @@ void mt76x02u_mcu_fw_reset(struct mt76x02_dev *dev)
 EXPORT_SYMBOL_GPL(mt76x02u_mcu_fw_reset);
 
 static int
-__mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
+__mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, u8 *data,
 			    const void *fw_data, int len, u32 dst_addr)
 {
-	u8 *data = sg_virt(&buf->urb->sg[0]);
-	DECLARE_COMPLETION_ONSTACK(cmpl);
 	__le32 info;
 	u32 val;
-	int err;
+	int err, data_len;
 
 	info = cpu_to_le32(FIELD_PREP(MT_MCU_MSG_PORT, CPU_TX_PORT) |
 			   FIELD_PREP(MT_MCU_MSG_LEN, len) |
@@ -291,25 +285,12 @@ __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
 	mt76u_single_wr(&dev->mt76, MT_VEND_WRITE_FCE,
 			MT_FCE_DMA_LEN, len << 16);
 
-	buf->len = MT_CMD_HDR_LEN + len + sizeof(info);
-	err = mt76u_submit_buf(&dev->mt76, USB_DIR_OUT,
-			       MT_EP_OUT_INBAND_CMD,
-			       buf, GFP_KERNEL,
-			       mt76u_mcu_complete_urb, &cmpl);
-	if (err < 0)
-		return err;
-
-	if (!wait_for_completion_timeout(&cmpl,
-					 msecs_to_jiffies(1000))) {
-		dev_err(dev->mt76.dev, "firmware upload timed out\n");
-		usb_kill_urb(buf->urb);
-		return -ETIMEDOUT;
-	}
+	data_len = MT_CMD_HDR_LEN + len + sizeof(info);
 
-	if (mt76u_urb_error(buf->urb)) {
-		dev_err(dev->mt76.dev, "firmware upload failed: %d\n",
-			buf->urb->status);
-		return buf->urb->status;
+	err = mt76u_bulk_msg(&dev->mt76, data, data_len, 1000);
+	if (err) {
+		dev_err(dev->mt76.dev, "firmware upload failed: %d\n", err);
+		return err;
 	}
 
 	val = mt76_rr(dev, MT_TX_CPU_FROM_FCE_CPU_DESC_IDX);
@@ -322,17 +303,16 @@ __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
 int mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, const void *data,
 			      int data_len, u32 max_payload, u32 offset)
 {
-	int err, len, pos = 0, max_len = max_payload - 8;
-	struct mt76u_buf buf;
+	int len, err = 0, pos = 0, max_len = max_payload - 8;
+	u8 *buf;
 
-	err = mt76u_buf_alloc(&dev->mt76, &buf, 1, max_payload, max_payload,
-			      GFP_KERNEL);
-	if (err < 0)
-		return err;
+	buf = kmalloc(max_payload, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
 	while (data_len > 0) {
 		len = min_t(int, data_len, max_len);
-		err = __mt76x02u_mcu_fw_send_data(dev, &buf, data + pos,
+		err = __mt76x02u_mcu_fw_send_data(dev, buf, data + pos,
 						  len, offset + pos);
 		if (err < 0)
 			break;
@@ -341,7 +321,7 @@ int mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, const void *data,
 		pos += len;
 		usleep_range(5000, 10000);
 	}
-	mt76u_buf_free(&buf);
+	kfree(buf);
 
 	return err;
 }
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 4f92732506cc..db31cae11911 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -327,7 +327,6 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
 
 	return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
 }
-EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
 
 void mt76u_buf_free(struct mt76u_buf *buf)
 {
-- 
2.7.5


[-- Attachment #4: 0003-mt76usb-do-not-use-compound-head-page-for-SG-I-O.patch --]
[-- Type: text/plain, Size: 1274 bytes --]

From 675fdc945c2d1fce01c72f520e82289ba5877ba2 Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Thu, 14 Feb 2019 19:48:05 +0100
Subject: [PATCH v5 3/3] mt76usb: do not use compound head page for SG I/O

We use head of compound page as base for SG, that results in sg->offset
bigger than PAGE_SIZE pointing to another page. Some DMA controllers
work ok with that, but for dma_map_sg() and dma_map_page() we should
provide buffer contained in single page (eventually with sg->offset = 0
and sg->length of more pages, but not with sg->offset pointed to
different page than sg->page).

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index db31cae11911..de906f07e85a 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -288,7 +288,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
 		if (!data)
 			break;
 
-		page = virt_to_head_page(data);
+		page = virt_to_page(data);
 		offset = data - page_address(page);
 		sg_set_page(&urb->sg[i], page, sglen, offset);
 	}
-- 
2.7.5


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-14  9:48           ` Stefan Wahren
@ 2019-02-14  9:54             ` Stanislaw Gruszka
  2019-02-15  7:12             ` Stanislaw Gruszka
  1 sibling, 0 replies; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-14  9:54 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

On Thu, Feb 14, 2019 at 10:48:15AM +0100, Stefan Wahren wrote:
> Hi Stanislaw,
> 
> Am 14.02.19 um 10:25 schrieb Stanislaw Gruszka:
> > On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
> >> 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
> > I think this is due to urb->transfer_length and sg[0]->length mismatch,
> > which should be addressed by my other patch. I'm attaching the patch
> > rebased on -next with this line integrated, please test. 
> >
> > But there could be other bug's in mt76-usb SG code.
> Will retry
> >
> >> 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.
> > I'm pretty sure problem is mt76x0u 4.19 -> 4.20 regression intrduced by
> > integrating mt76x0u in mt76-usb (things do not work from day 0
> > for mt76x2u). We should find fix(es) that will be proper for -stable.
> So i should also try 4.19 without any patches?

That would be good. Thanks.

Stanislaw


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-14  9:25         ` Stanislaw Gruszka
@ 2019-02-14  9:48           ` Stefan Wahren
  2019-02-14  9:54             ` Stanislaw Gruszka
  2019-02-15  7:12             ` Stanislaw Gruszka
  0 siblings, 2 replies; 37+ messages in thread
From: Stefan Wahren @ 2019-02-14  9:48 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

Hi Stanislaw,

Am 14.02.19 um 10:25 schrieb Stanislaw Gruszka:
> On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
>> 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
> I think this is due to urb->transfer_length and sg[0]->length mismatch,
> which should be addressed by my other patch. I'm attaching the patch
> rebased on -next with this line integrated, please test. 
>
> But there could be other bug's in mt76-usb SG code.
Will retry
>
>> 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.
> I'm pretty sure problem is mt76x0u 4.19 -> 4.20 regression intrduced by
> integrating mt76x0u in mt76-usb (things do not work from day 0
> for mt76x2u). We should find fix(es) that will be proper for -stable.
So i should also try 4.19 without any patches?
>
> Stanislaw 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-14  6:49       ` Stefan Wahren
@ 2019-02-14  9:25         ` Stanislaw Gruszka
  2019-02-14  9:48           ` Stefan Wahren
  0 siblings, 1 reply; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-14  9:25 UTC (permalink / raw)
  To: Stefan Wahren
  Cc: Lorenzo Bianconi, Alan Stern, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 1670 bytes --]

On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
> 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

I think this is due to urb->transfer_length and sg[0]->length mismatch,
which should be addressed by my other patch. I'm attaching the patch
rebased on -next with this line integrated, please test. 

But there could be other bug's in mt76-usb SG code.

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

I'm pretty sure problem is mt76x0u 4.19 -> 4.20 regression intrduced by
integrating mt76x0u in mt76-usb (things do not work from day 0
for mt76x2u). We should find fix(es) that will be proper for -stable.

Stanislaw 

[-- Attachment #2: 0001-mt76usb-do-not-set-urb-num_sgs-to-1-for-non-SG-usb-h.patch --]
[-- Type: text/plain, Size: 6116 bytes --]

From 2b20054fc424e0bb33962bbb05509a7ca16d01bc Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <sgruszka@redhat.com>
Date: Thu, 14 Feb 2019 10:01:06 +0100
Subject: [PATCH v5] 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 +
 .../net/wireless/mediatek/mt76/mt76x02_usb_mcu.c   |  1 +
 drivers/net/wireless/mediatek/mt76/usb.c           | 57 ++++++++++++++--------
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2bb9db4ed80a..97ad0270f8a6 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/mt76x02_usb_mcu.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
index 6db789f90269..80a259e1e931 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -292,6 +292,7 @@ __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
 			MT_FCE_DMA_LEN, len << 16);
 
 	buf->len = MT_CMD_HDR_LEN + len + sizeof(info);
+	buf->urb->sg[0].length = buf->len;
 	err = mt76u_submit_buf(&dev->mt76, USB_DIR_OUT,
 			       MT_EP_OUT_INBAND_CMD,
 			       buf, GFP_KERNEL,
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 6a2507524c6c..4f92732506cc 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -297,14 +297,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;
 }
@@ -323,6 +323,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);
 }
@@ -333,15 +334,16 @@ void mt76u_buf_free(struct mt76u_buf *buf)
 	struct urb *urb = buf->urb;
 	int i;
 
-	for (i = 0; i < urb->num_sgs; i++)
+	for (i = 0; i < buf->num_sgs; i++)
 		skb_free_frag(sg_virt(&urb->sg[i]));
 	usb_free_urb(buf->urb);
 }
 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);
@@ -352,12 +354,28 @@ 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);
-	trace_submit_urb(dev, buf->urb);
+	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);
 
 static inline struct mt76u_buf
@@ -667,10 +685,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;
@@ -679,7 +698,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);
 }
@@ -689,12 +709,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)
@@ -708,13 +725,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.7.5


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-12  9:30     ` Stanislaw Gruszka
  2019-02-12 11:58       ` Lorenzo Bianconi
@ 2019-02-14  6:49       ` Stefan Wahren
  2019-02-14  9:25         ` Stanislaw Gruszka
  1 sibling, 1 reply; 37+ messages in thread
From: Stefan Wahren @ 2019-02-14  6:49 UTC (permalink / raw)
  To: Stanislaw Gruszka, Lorenzo Bianconi
  Cc: Alan Stern, Felix Fietkau, Doug Anderson, Minas Harutyunyan,
	USB list, linux-wireless

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-12  0:06   ` Lorenzo Bianconi
  2019-02-12  9:30     ` Stanislaw Gruszka
@ 2019-02-12 15:27     ` Alan Stern
  1 sibling, 0 replies; 37+ messages in thread
From: Alan Stern @ 2019-02-12 15:27 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Stanislaw Gruszka, Stefan Wahren, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

On Tue, 12 Feb 2019, Lorenzo Bianconi wrote:

> 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)
> What do you think?

If mt76u can use usb_sg_init/usb_sg_wait, that would be the simplest.  
It would allow you to remove a lot of code from the driver.  And then
adding SG support to the controller drivers one by one would be fine.

However, if that isn't feasible then you have to keep legacy I/O in 
mt76u as long as any controller drivers don't support SG.

Alan Stern


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-12 11:58       ` Lorenzo Bianconi
@ 2019-02-12 13:15         ` Stanislaw Gruszka
  0 siblings, 0 replies; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-12 13:15 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Alan Stern, Stefan Wahren, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

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

^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-12  9:30     ` Stanislaw Gruszka
@ 2019-02-12 11:58       ` Lorenzo Bianconi
  2019-02-12 13:15         ` Stanislaw Gruszka
  2019-02-14  6:49       ` Stefan Wahren
  1 sibling, 1 reply; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-12 11:58 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Alan Stern, Stefan Wahren, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless


[...]

> 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
> 


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
  2019-02-12  0:06   ` Lorenzo Bianconi
@ 2019-02-12  9:30     ` Stanislaw Gruszka
  2019-02-12 11:58       ` Lorenzo Bianconi
  2019-02-14  6:49       ` Stefan Wahren
  2019-02-12 15:27     ` Alan Stern
  1 sibling, 2 replies; 37+ messages in thread
From: Stanislaw Gruszka @ 2019-02-12  9:30 UTC (permalink / raw)
  To: Lorenzo Bianconi
  Cc: Alan Stern, Stefan Wahren, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

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

[-- Attachment #2: 0001-mt76usb-do-not-set-urb-num_sgs-to-1-for-non-SG-usb-h.patch --]
[-- Type: text/plain, Size: 5402 bytes --]

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);
+	}
+}
 
+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


^ permalink raw reply	[flat|nested] 37+ messages in thread

* Re: [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+
       [not found] ` <Pine.LNX.4.44L0.1902111246410.1543-100000@iolanthe.rowland.org>
@ 2019-02-12  0:06   ` Lorenzo Bianconi
  2019-02-12  9:30     ` Stanislaw Gruszka
  2019-02-12 15:27     ` Alan Stern
  0 siblings, 2 replies; 37+ messages in thread
From: Lorenzo Bianconi @ 2019-02-12  0:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Stanislaw Gruszka, Stefan Wahren, Felix Fietkau, Doug Anderson,
	Minas Harutyunyan, USB list, linux-wireless

>
> 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)
What do you think?

Regards,
Lorenzo

^ permalink raw reply	[flat|nested] 37+ messages in thread

end of thread, back to index

Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-09 12:08 [BUG] mt76x0u: Probing issues on Raspberry Pi 3 B+ Stefan Wahren
2019-02-09 18:46 ` Lorenzo Bianconi
2019-02-09 20:29   ` Stefan Wahren
2019-02-09 20:33     ` Lorenzo Bianconi
2019-02-09 22:47       ` Stefan Wahren
2019-02-10  9:41     ` Stanislaw Gruszka
2019-02-10 10:22       ` Lorenzo Bianconi
2019-02-11  7:44         ` Stanislaw Gruszka
2019-02-11 10:04           ` Lorenzo Bianconi
2019-02-11 10:33             ` Stefan Wahren
2019-02-11 11:06               ` Lorenzo Bianconi
2019-02-11 14:04                 ` Stefan Wahren
2019-02-11 15:10                   ` Lorenzo Bianconi
2019-02-11 15:27                     ` Stefan Wahren
2019-02-11 15:57                       ` Lorenzo Bianconi
2019-02-13  7:05                         ` Stefan Wahren
2019-02-11 17:22                       ` Stanislaw Gruszka
2019-02-10  9:29   ` Stanislaw Gruszka
2019-02-10 16:38     ` Stefan Wahren
2019-02-10 16:52       ` Lorenzo Bianconi
2019-02-10 17:39         ` Lorenzo Bianconi
2019-02-11  7:50         ` Stanislaw Gruszka
2019-02-11  8:08           ` Stefan Wahren
2019-02-11  9:52             ` Lorenzo Bianconi
     [not found] <20190211173315.GE6292@redhat.com>
     [not found] ` <Pine.LNX.4.44L0.1902111246410.1543-100000@iolanthe.rowland.org>
2019-02-12  0:06   ` Lorenzo Bianconi
2019-02-12  9:30     ` Stanislaw Gruszka
2019-02-12 11:58       ` Lorenzo Bianconi
2019-02-12 13:15         ` Stanislaw Gruszka
2019-02-14  6:49       ` Stefan Wahren
2019-02-14  9:25         ` Stanislaw Gruszka
2019-02-14  9:48           ` Stefan Wahren
2019-02-14  9:54             ` Stanislaw Gruszka
2019-02-15  7:12             ` Stanislaw Gruszka
2019-02-16 11:05               ` Stefan Wahren
2019-02-16 14:07                 ` Stanislaw Gruszka
2019-02-16 19:17                   ` Stefan Wahren
2019-02-12 15:27     ` Alan Stern

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox