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
>
> 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
Hi Lorenzo,
> Lorenzo Bianconi <[email protected]> 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
>
> Hi Lorenzo,
>
> > Lorenzo Bianconi <[email protected]> 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
Hi Lorenzo,
> Lorenzo Bianconi <[email protected]> hat am 9. Februar 2019 um 21:33 geschrieben:
>
>
> >
> > Hi Lorenzo,
> >
> > > Lorenzo Bianconi <[email protected]> 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
>
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
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
> 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
Hi,
> Stanislaw Gruszka <[email protected]> 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
>
> Hi,
>
> > Stanislaw Gruszka <[email protected]> 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
> > 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
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
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
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
> 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
>
> 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
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
> 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
>
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
>>
> 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
> >>
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
> 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
>
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/[email protected]/
Stanislaw
>
> 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
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
[...]
> 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 <[email protected]>
> 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 <[email protected]>
> ---
> 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
>
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
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
Hi,
> Lorenzo Bianconi <[email protected]> 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
Hi Stanislaw,
> Stanislaw Gruszka <[email protected]> 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
On Thu, Feb 14, 2019 at 07:49:57AM +0100, Stefan Wahren wrote:
> Hi Stanislaw,
>
> > Stanislaw Gruszka <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]> 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
Hi Stanislaw,
> Stanislaw Gruszka <[email protected]> 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 <[email protected]> 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
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
Hi Stanislaw,
> Stanislaw Gruszka <[email protected]> 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
On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
> 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.
>
I see, it needs 4 bytes alignment . There is already dwc2 code checks
that and allocate new buffer if the alignment is not right:
dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
is not NULL. I thought mt76usb already provide aligned buffers, but
looks it does not for one TX special case, which are PROBE REQUEST
frames. Other frames are aligned by inserting L2 header pad. One
solution for this would be just submit urb with NULL sg (same as
Lorenzo's patches do, but still allocating buffers via buf->sg),
but I think, you have right, we should provide 4 bytes aligned buffers
by default as other DMA hardware may require that. I'm attaching yet
another patch to test, which fix up alignment for PROBE REQUEST frames.
> > 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
I noticed on my setup that patch 4 can cause troubles, but still
device is workable here on my PC machines.
> > > 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.
That somewhat strange because 4.19 mt76x0u does not use SG.
On 4.19 there is phy calibration bug fixed in 4.19.5:
commit 0d9813319b40399a0d8fd761d2fcfedee5701487
Author: Lorenzo Bianconi <[email protected]>
Date: Fri Sep 7 23:13:12 2018 +0200
mt76x0: run vco calibration for each channel configuration
It's possible that without this vco calibration fix, MT7610U device
does not see AP if it's signal is weak.
Stanislaw
> commit 0d9813319b40399a0d8fd761d2fcfedee5701487
> Author: Lorenzo Bianconi <[email protected]>
> Date: Fri Sep 7 23:13:12 2018 +0200
[...]
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> index 062614ad0d51..08425b1d2c30 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> @@ -550,21 +550,33 @@ void mt76x02_sta_rate_tbl_update(struct ieee80211_hw *hw,
> }
> EXPORT_SYMBOL_GPL(mt76x02_sta_rate_tbl_update);
>
> -int mt76x02_insert_hdr_pad(struct sk_buff *skb)
> +void mt76x02_align_skb(struct sk_buff *skb)
> {
> - int len = ieee80211_get_hdrlen_from_skb(skb);
> + int align = ((unsigned long) skb->data) & 3;
> + int hdrlen, skblen;
>
> - if (len % 4 == 0)
> - return 0;
> + hdrlen = ieee80211_get_hdrlen_from_skb(skb);
> + WARN_ON_ONCE(align == 0 && (hdrlen & 3));
> +
> + if (align == 0)
> + return;
Hi Stanislaw,
is it possible that skb->data is 4 byte aligned but hdrlen is not? (e.g 4addr
data frames, not qos)?
>
> - skb_push(skb, 2);
> - memmove(skb->data, skb->data + 2, len);
> + if (hdrlen & 3) {
> + /* Align frame and add 2 bytes pad after header. */
> + skb_push(skb, 2);
> + memmove(skb->data, skb->data + 2, hdrlen);
>
> - skb->data[len] = 0;
> - skb->data[len + 1] = 0;
> - return 2;
> + skb->data[hdrlen] = 0;
> + skb->data[hdrlen + 1] = 0;
> + } else {
> + /* Only for probe request frames. */
are you sure this is true *only* for probe requests?
this could hit performances and it is used even in pci code
Regards,
Lorenzo
> + skblen = skb->len;
> + skb_push(skb, 2);
> + memmove(skb->data, skb->data + 2, skblen);
> + skb_trim(skb, skblen);
> + }
> }
> -EXPORT_SYMBOL_GPL(mt76x02_insert_hdr_pad);
> +EXPORT_SYMBOL_GPL(mt76x02_align_skb);
>
> void mt76x02_remove_hdr_pad(struct sk_buff *skb, int len)
> {
> --
> 2.7.5
>
On 2019-02-18 14:52, Stanislaw Gruszka wrote:
> On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
>> 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.
>>
>
> I see, it needs 4 bytes alignment . There is already dwc2 code checks
> that and allocate new buffer if the alignment is not right:
> dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
> is not NULL. I thought mt76usb already provide aligned buffers, but
> looks it does not for one TX special case, which are PROBE REQUEST
> frames. Other frames are aligned by inserting L2 header pad. One
> solution for this would be just submit urb with NULL sg (same as
> Lorenzo's patches do, but still allocating buffers via buf->sg),
> but I think, you have right, we should provide 4 bytes aligned buffers
> by default as other DMA hardware may require that. I'm attaching yet
> another patch to test, which fix up alignment for PROBE REQUEST frames.
This approach looks completely wrong to me. MMIO based hardware does not
need 4-byte aligned buffers at all, other USB controllers do not need
this either.
As Lorenzo already pointed out, re-aligning the buffer is *very*
expensive, so we should not do this in the driver just to work around
quirks in a particular USB host driver.
And I really don't think we can assume that this code path is going to
be hit for probe requests only.
The way I see it, we have two choices.
1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
2. Rely on urb->transfer_buffer and keep urb->sg NULL
- Felix
On Mon, Feb 18, 2019 at 03:25:28PM +0100, Lorenzo Bianconi wrote:
> > commit 0d9813319b40399a0d8fd761d2fcfedee5701487
> > Author: Lorenzo Bianconi <[email protected]>
> > Date: Fri Sep 7 23:13:12 2018 +0200
>
> [...]
>
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > index 062614ad0d51..08425b1d2c30 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
> > @@ -550,21 +550,33 @@ void mt76x02_sta_rate_tbl_update(struct ieee80211_hw *hw,
> > }
> > EXPORT_SYMBOL_GPL(mt76x02_sta_rate_tbl_update);
> >
> > -int mt76x02_insert_hdr_pad(struct sk_buff *skb)
> > +void mt76x02_align_skb(struct sk_buff *skb)
> > {
> > - int len = ieee80211_get_hdrlen_from_skb(skb);
> > + int align = ((unsigned long) skb->data) & 3;
> > + int hdrlen, skblen;
> >
> > - if (len % 4 == 0)
> > - return 0;
> > + hdrlen = ieee80211_get_hdrlen_from_skb(skb);
> > + WARN_ON_ONCE(align == 0 && (hdrlen & 3));
> > +
> > + if (align == 0)
> > + return;
>
> Hi Stanislaw,
>
> is it possible that skb->data is 4 byte aligned but hdrlen is not? (e.g 4addr
> data frames, not qos)?
It might be possible, so for now I add this
WARN_ON_ONCE(align == 0 && (hdrlen & 3));
and plan to do some investigation on frame aliment.
> are you sure this is true *only* for probe requests?
> this could hit performances and it is used even in pci code
I'm not 100% sure, but I haven't seen any other frames with
that. All other unaligned frames I've seen in my tests had
unaligned header and adding header pad aligned them.
But obviously I've haven't tested all possible scenarios.
Stanislaw
On Mon, Feb 18, 2019 at 03:43:26PM +0100, Felix Fietkau wrote:
> On 2019-02-18 14:52, Stanislaw Gruszka wrote:
> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
> >> 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.
> >>
> >
> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
> > that and allocate new buffer if the alignment is not right:
> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
> > is not NULL. I thought mt76usb already provide aligned buffers, but
> > looks it does not for one TX special case, which are PROBE REQUEST
> > frames. Other frames are aligned by inserting L2 header pad. One
> > solution for this would be just submit urb with NULL sg (same as
> > Lorenzo's patches do, but still allocating buffers via buf->sg),
> > but I think, you have right, we should provide 4 bytes aligned buffers
> > by default as other DMA hardware may require that. I'm attaching yet
> > another patch to test, which fix up alignment for PROBE REQUEST frames.
> This approach looks completely wrong to me. MMIO based hardware does not
> need 4-byte aligned buffers at all, other USB controllers do not need
> this either.
> As Lorenzo already pointed out, re-aligning the buffer is *very*
> expensive, so we should not do this in the driver just to work around
> quirks in a particular USB host driver.
I decided to this patch because I thought some other USB & MMIO DMA
platforms might also require this alignment. But it was never triggered
in MMIO because on those mt76 is used in AP mode, hence no PROBE
REQUEST frames (and scan can be passive on STA mode).
> The way I see it, we have two choices.
> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
I agree, if this is only needed for dwc2. Though I would investigate
if this is not a bug on other platforms as well.
Stanislaw
On 2019-02-18 16:03, Stanislaw Gruszka wrote:
> On Mon, Feb 18, 2019 at 03:43:26PM +0100, Felix Fietkau wrote:
>> On 2019-02-18 14:52, Stanislaw Gruszka wrote:
>> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
>> >> 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.
>> >>
>> >
>> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
>> > that and allocate new buffer if the alignment is not right:
>> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
>> > is not NULL. I thought mt76usb already provide aligned buffers, but
>> > looks it does not for one TX special case, which are PROBE REQUEST
>> > frames. Other frames are aligned by inserting L2 header pad. One
>> > solution for this would be just submit urb with NULL sg (same as
>> > Lorenzo's patches do, but still allocating buffers via buf->sg),
>> > but I think, you have right, we should provide 4 bytes aligned buffers
>> > by default as other DMA hardware may require that. I'm attaching yet
>> > another patch to test, which fix up alignment for PROBE REQUEST frames.
>> This approach looks completely wrong to me. MMIO based hardware does not
>> need 4-byte aligned buffers at all, other USB controllers do not need
>> this either.
>> As Lorenzo already pointed out, re-aligning the buffer is *very*
>> expensive, so we should not do this in the driver just to work around
>> quirks in a particular USB host driver.
>
> I decided to this patch because I thought some other USB & MMIO DMA
> platforms might also require this alignment. But it was never triggered
> in MMIO because on those mt76 is used in AP mode, hence no PROBE
> REQUEST frames (and scan can be passive on STA mode).
mt76 is regularly used and tested in STA and Mesh mode as well.
No DMA alignment related issues there.
>> The way I see it, we have two choices.
>> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
>> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
>
> I agree, if this is only needed for dwc2. Though I would investigate
> if this is not a bug on other platforms as well.
From what I can see, using Lorenzo's patches seems to be the better
solution, since they avoid these corner cases in dwc2 (and maybe other
drivers as well). I will apply them and then we'll see if we need to do
any further improvements later on.
- Felix
Hi,
> Stanislaw Gruszka <[email protected]> hat am 18. Februar 2019 um 14:52 geschrieben:
>
>
> On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
> > 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.
> >
>
> I see, it needs 4 bytes alignment . There is already dwc2 code checks
> that and allocate new buffer if the alignment is not right:
> dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
> is not NULL. I thought mt76usb already provide aligned buffers, but
> looks it does not for one TX special case, which are PROBE REQUEST
> frames. Other frames are aligned by inserting L2 header pad. One
> solution for this would be just submit urb with NULL sg (same as
> Lorenzo's patches do, but still allocating buffers via buf->sg),
> but I think, you have right, we should provide 4 bytes aligned buffers
> by default as other DMA hardware may require that. I'm attaching yet
> another patch to test, which fix up alignment for PROBE REQUEST frames.
>
> > > Attached patch should fix this, plese test, thanks in advance.
i saw Felix decided to use Lorenzo's approach.
The patches 1,3,5 applied on today's next fixed only the warning and wifi is still broken (authentication timeout).
Here are the logs for multi_v7_defconfig:
https://gist.github.com/lategoodbye/0a7c5cea7dbf25d0de7944c05d229d79
> >
> > 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
>
> I noticed on my setup that patch 4 can cause troubles, but still
> device is workable here on my PC machines.
>
> > > > 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.
>
> That somewhat strange because 4.19 mt76x0u does not use SG.
> On 4.19 there is phy calibration bug fixed in 4.19.5:
Sorry for being inprecise. I was talking about the branches not the exact tags. I tested 4.19.23 without luck.
Many thanks anyway
Stefan
On Mon, Feb 18, 2019 at 07:52:27PM +0100, Felix Fietkau wrote:
> On 2019-02-18 16:03, Stanislaw Gruszka wrote:
> > On Mon, Feb 18, 2019 at 03:43:26PM +0100, Felix Fietkau wrote:
> >> On 2019-02-18 14:52, Stanislaw Gruszka wrote:
> >> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
> >> >> 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.
> >> >>
> >> >
> >> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
> >> > that and allocate new buffer if the alignment is not right:
> >> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
> >> > is not NULL. I thought mt76usb already provide aligned buffers, but
> >> > looks it does not for one TX special case, which are PROBE REQUEST
> >> > frames. Other frames are aligned by inserting L2 header pad. One
> >> > solution for this would be just submit urb with NULL sg (same as
> >> > Lorenzo's patches do, but still allocating buffers via buf->sg),
> >> > but I think, you have right, we should provide 4 bytes aligned buffers
> >> > by default as other DMA hardware may require that. I'm attaching yet
> >> > another patch to test, which fix up alignment for PROBE REQUEST frames.
> >> This approach looks completely wrong to me. MMIO based hardware does not
> >> need 4-byte aligned buffers at all, other USB controllers do not need
> >> this either.
> >> As Lorenzo already pointed out, re-aligning the buffer is *very*
> >> expensive, so we should not do this in the driver just to work around
> >> quirks in a particular USB host driver.
> >
> > I decided to this patch because I thought some other USB & MMIO DMA
> > platforms might also require this alignment. But it was never triggered
> > in MMIO because on those mt76 is used in AP mode, hence no PROBE
> > REQUEST frames (and scan can be passive on STA mode).
> mt76 is regularly used and tested in STA and Mesh mode as well.
> No DMA alignment related issues there.
The question is why we need to do 2 bytes header pad ? Is this because
ieee802.11 header length for mt76 hardware has to be multiple of 4 or
it has to be aligned to 4 bytes? It would be strange if length has
to be fixed to 4 bytes and alignment not. However this could be
needed on some platforms and not on others.
> >> The way I see it, we have two choices.
> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> >
> > I agree, if this is only needed for dwc2. Though I would investigate
> > if this is not a bug on other platforms as well.
> >From what I can see, using Lorenzo's patches seems to be the better
> solution, since they avoid these corner cases in dwc2 (and maybe other
> drivers as well). I will apply them and then we'll see if we need to do
> any further improvements later on.
They work on rpi dwc2, but they do not address root of the problem.
There is clearly something wrong how mt76usb handle SG, what is not
fixed. And adding disable_usb_sg module parameter for hcd's supporting
SG should be red flag.
Stanislaw
On Mon, Feb 18, 2019 at 11:19:40PM +0100, Stefan Wahren wrote:
> Hi,
>
> > Stanislaw Gruszka <[email protected]> hat am 18. Februar 2019 um 14:52 geschrieben:
> >
> >
> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
> > > 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.
> > >
> >
> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
> > that and allocate new buffer if the alignment is not right:
> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
> > is not NULL. I thought mt76usb already provide aligned buffers, but
> > looks it does not for one TX special case, which are PROBE REQUEST
> > frames. Other frames are aligned by inserting L2 header pad. One
> > solution for this would be just submit urb with NULL sg (same as
> > Lorenzo's patches do, but still allocating buffers via buf->sg),
> > but I think, you have right, we should provide 4 bytes aligned buffers
> > by default as other DMA hardware may require that. I'm attaching yet
> > another patch to test, which fix up alignment for PROBE REQUEST frames.
> >
> > > > Attached patch should fix this, plese test, thanks in advance.
>
> i saw Felix decided to use Lorenzo's approach.
>
> The patches 1,3,5 applied on today's next fixed only the warning and wifi is still broken (authentication timeout).
>
> Here are the logs for multi_v7_defconfig:
> https://gist.github.com/lategoodbye/0a7c5cea7dbf25d0de7944c05d229d79
It would be interesting why urb->num_sgs = 0 & urb->sg cause
the troubles. This is how usb_sg_init() submit urbs for sg_tablesize = 0
controllers. So either are there are some requirement on urb->sg
mapped via dma_map_page() (which mt76usb does not meet) not needed
for urb->transfer_buffer mapped via dma_map_single() or there
is something wrong in dwc2 with sg and this driver will not
work with urb_sg_init() as well. I don't have hardware to investigate
this and don't want to bother you with more patches.
> > > 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
> >
> > I noticed on my setup that patch 4 can cause troubles, but still
> > device is workable here on my PC machines.
> >
> > > > > 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.
> >
> > That somewhat strange because 4.19 mt76x0u does not use SG.
> > On 4.19 there is phy calibration bug fixed in 4.19.5:
>
> Sorry for being inprecise. I was talking about the branches not the exact tags. I tested 4.19.23 without luck.
In github link you provided someone report mt76x0u working on 4.19 with
dwc_otg with disabled FIQ irq, so perhaps this is due to bug in dwc2
fixed in -next but not backported to 4.19.
> Many thanks anyway
Thanks for testing!
Stanislaw
On 2019-02-19 11:59, Stanislaw Gruszka wrote:
> On Mon, Feb 18, 2019 at 11:19:40PM +0100, Stefan Wahren wrote:
>> Hi,
>>
>> > Stanislaw Gruszka <[email protected]> hat am 18. Februar 2019 um 14:52 geschrieben:
>> >
>> >
>> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
>> > > 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.
>> > >
>> >
>> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
>> > that and allocate new buffer if the alignment is not right:
>> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
>> > is not NULL. I thought mt76usb already provide aligned buffers, but
>> > looks it does not for one TX special case, which are PROBE REQUEST
>> > frames. Other frames are aligned by inserting L2 header pad. One
>> > solution for this would be just submit urb with NULL sg (same as
>> > Lorenzo's patches do, but still allocating buffers via buf->sg),
>> > but I think, you have right, we should provide 4 bytes aligned buffers
>> > by default as other DMA hardware may require that. I'm attaching yet
>> > another patch to test, which fix up alignment for PROBE REQUEST frames.
>> >
>> > > > Attached patch should fix this, plese test, thanks in advance.
>>
>> i saw Felix decided to use Lorenzo's approach.
>>
>> The patches 1,3,5 applied on today's next fixed only the warning and wifi is still broken (authentication timeout).
>>
>> Here are the logs for multi_v7_defconfig:
>> https://gist.github.com/lategoodbye/0a7c5cea7dbf25d0de7944c05d229d79
>
> It would be interesting why urb->num_sgs = 0 & urb->sg cause
> the troubles. This is how usb_sg_init() submit urbs for sg_tablesize = 0
> controllers. So either are there are some requirement on urb->sg
> mapped via dma_map_page() (which mt76usb does not meet) not needed
> for urb->transfer_buffer mapped via dma_map_single() or there
> is something wrong in dwc2 with sg and this driver will not
> work with urb_sg_init() as well. I don't have hardware to investigate
> this and don't want to bother you with more patches.
I think the conditions for skipping the alloc of the DMA aligned buffer
in dwc2 are a bit quirky:
if (urb->num_sgs || urb->sg ||
urb->transfer_buffer_length == 0 ||
!((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
return 0;
It would probably make more sense to write:
if ((urb->num_sgs && urb->sg &&
urb->transfer_buffer_length == 0) ||
!((uintptr_t)urb->transfer_buffer & (DWC2_USB_DMA_ALIGN - 1)))
return 0;
It would still need some extra code for the urb->num_sgs==1 case though.
That code was originally written for the nvidia tegra host controller
driver and copied to dwc2. Maybe at the time they just didn't want to
write extra code dealing with urb->sg because nobody was using it for
single-buffer transfers, or they didn't have a test case.
Either way, while it might be a good idea to improve dwc2 and tegra, I
still think avoiding the use of urb->sg in single buffer cases is a good
idea. One advantage is that less memory needs to be allocated.
- Felix
On 2019-02-19 11:42, Stanislaw Gruszka wrote:
> On Mon, Feb 18, 2019 at 07:52:27PM +0100, Felix Fietkau wrote:
>> On 2019-02-18 16:03, Stanislaw Gruszka wrote:
>> > On Mon, Feb 18, 2019 at 03:43:26PM +0100, Felix Fietkau wrote:
>> >> On 2019-02-18 14:52, Stanislaw Gruszka wrote:
>> >> > On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
>> >> >> 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.
>> >> >>
>> >> >
>> >> > I see, it needs 4 bytes alignment . There is already dwc2 code checks
>> >> > that and allocate new buffer if the alignment is not right:
>> >> > dwc2_alloc_dma_aligned_buffer(), but it does nothing if urb->sg
>> >> > is not NULL. I thought mt76usb already provide aligned buffers, but
>> >> > looks it does not for one TX special case, which are PROBE REQUEST
>> >> > frames. Other frames are aligned by inserting L2 header pad. One
>> >> > solution for this would be just submit urb with NULL sg (same as
>> >> > Lorenzo's patches do, but still allocating buffers via buf->sg),
>> >> > but I think, you have right, we should provide 4 bytes aligned buffers
>> >> > by default as other DMA hardware may require that. I'm attaching yet
>> >> > another patch to test, which fix up alignment for PROBE REQUEST frames.
>> >> This approach looks completely wrong to me. MMIO based hardware does not
>> >> need 4-byte aligned buffers at all, other USB controllers do not need
>> >> this either.
>> >> As Lorenzo already pointed out, re-aligning the buffer is *very*
>> >> expensive, so we should not do this in the driver just to work around
>> >> quirks in a particular USB host driver.
>> >
>> > I decided to this patch because I thought some other USB & MMIO DMA
>> > platforms might also require this alignment. But it was never triggered
>> > in MMIO because on those mt76 is used in AP mode, hence no PROBE
>> > REQUEST frames (and scan can be passive on STA mode).
>> mt76 is regularly used and tested in STA and Mesh mode as well.
>> No DMA alignment related issues there.
>
> The question is why we need to do 2 bytes header pad ? Is this because
> ieee802.11 header length for mt76 hardware has to be multiple of 4 or
> it has to be aligned to 4 bytes? It would be strange if length has
> to be fixed to 4 bytes and alignment not. However this could be
> needed on some platforms and not on others.
Not sure why it was added in the hardware, but the MAC assumes that the
header is padded to a multiple of 4 bytes in the buffer after it has
been copied to device memory via DMA or USB.
Because of that, it has nothing to do with the alignment of the source
buffer, so the two should not be mixed up.
>> >> The way I see it, we have two choices.
>> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
>> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
>> >
>> > I agree, if this is only needed for dwc2. Though I would investigate
>> > if this is not a bug on other platforms as well.
>> >From what I can see, using Lorenzo's patches seems to be the better
>> solution, since they avoid these corner cases in dwc2 (and maybe other
>> drivers as well). I will apply them and then we'll see if we need to do
>> any further improvements later on.
>
> They work on rpi dwc2, but they do not address root of the problem.
> There is clearly something wrong how mt76usb handle SG, what is not
> fixed. And adding disable_usb_sg module parameter for hcd's supporting
> SG should be red flag.
I think we're simply dealing with multiple issues here, only some of
which are fixed by Lorenzo's patches.
I'm pretty sure it's still wrong for mt76 to try to align its buffers,
since the Linux USB API supports non-aligned transfer buffers and it
should be up to the controller driver to deal with that.
dwc2 tries to do that, but that has limitations which I already pointed
out and which are properly dealt with by Lorenzo's patches.
Any other issues with sg should be investigated separately, they are
probably unrelated to this one.
- Felix
On Tue, 19 Feb 2019, Stanislaw Gruszka wrote:
> It would be interesting why urb->num_sgs = 0 & urb->sg cause
> the troubles. This is how usb_sg_init() submit urbs for sg_tablesize = 0
> controllers. So either are there are some requirement on urb->sg
> mapped via dma_map_page() (which mt76usb does not meet) not needed
> for urb->transfer_buffer mapped via dma_map_single() or there
> is something wrong in dwc2 with sg and this driver will not
> work with urb_sg_init() as well. I don't have hardware to investigate
> this and don't want to bother you with more patches.
urb->sg != NULL and urb->num_sgs == 0 means that the transfer buffer
must fit into a single page, which is pointed to by the first element
of the scatterlist.
But now that I look at the code in usb_sg_init(), it seems odd that the
!use_sg case doesn't increment sg during each loop iteration. I don't
see how that could possibly work -- it looks like a bug!
Alan Stern
Am 19.02.19 um 11:59 schrieb Stanislaw Gruszka:
> On Mon, Feb 18, 2019 at 11:19:40PM +0100, Stefan Wahren wrote:
>> Hi,
>>
>>> Stanislaw Gruszka <[email protected]> hat am 18. Februar 2019 um 14:52 geschrieben:
>>>
>>>
>>> On Sat, Feb 16, 2019 at 08:17:07PM +0100, Stefan Wahren wrote:
>>>
>>> Sorry for being inprecise. I was talking about the branches not the exact tags. I tested 4.19.23 without luck.
> In github link you provided someone report mt76x0u working on 4.19 with
> dwc_otg with disabled FIQ irq, so perhaps this is due to bug in dwc2
> fixed in -next but not backported to 4.19.
>
We need to consider dwc_otg (out of tree) and dwc2 (mainline) as two
different drivers, which has been maintained independent over years.
I think it would be interesting to see how dwc2 behaves on a different
ARM platform. Currently we cannot say this issue is specific to dwc2 or RPi.
Stefan
On Tue, Feb 19, 2019 at 10:40:47AM -0500, Alan Stern wrote:
> On Tue, 19 Feb 2019, Stanislaw Gruszka wrote:
>
> > It would be interesting why urb->num_sgs = 0 & urb->sg cause
> > the troubles. This is how usb_sg_init() submit urbs for sg_tablesize = 0
> > controllers. So either are there are some requirement on urb->sg
> > mapped via dma_map_page() (which mt76usb does not meet) not needed
> > for urb->transfer_buffer mapped via dma_map_single() or there
> > is something wrong in dwc2 with sg and this driver will not
> > work with urb_sg_init() as well. I don't have hardware to investigate
> > this and don't want to bother you with more patches.
>
> urb->sg != NULL and urb->num_sgs == 0 means that the transfer buffer
> must fit into a single page, which is pointed to by the first element
> of the scatterlist.
I asked about that in other thread
https://lore.kernel.org/linux-wireless/[email protected]/
the answer was it is weird but valid. However I think do dma_map_page()
on buffer not fit in the single page is asking for troubles. I just posted
patch that should fix this for mt76usb.
> But now that I look at the code in usb_sg_init(), it seems odd that the
> !use_sg case doesn't increment sg during each loop iteration. I don't
> see how that could possibly work -- it looks like a bug!
Looks for me that this is done via for_each_sg(sg, sg, io->entries, i) loop.
Stanislaw
On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> >> >> The way I see it, we have two choices.
> >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> >> >
> >> > I agree, if this is only needed for dwc2. Though I would investigate
> >> > if this is not a bug on other platforms as well.
> >> >From what I can see, using Lorenzo's patches seems to be the better
> >> solution, since they avoid these corner cases in dwc2 (and maybe other
> >> drivers as well). I will apply them and then we'll see if we need to do
> >> any further improvements later on.
> >
> > They work on rpi dwc2, but they do not address root of the problem.
> > There is clearly something wrong how mt76usb handle SG, what is not
> > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > SG should be red flag.
> I think we're simply dealing with multiple issues here, only some of
> which are fixed by Lorenzo's patches.
> I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> since the Linux USB API supports non-aligned transfer buffers and it
> should be up to the controller driver to deal with that.
Agree.
> dwc2 tries to do that, but that has limitations which I already pointed
> out and which are properly dealt with by Lorenzo's patches.
I planed to just accept current solution, but I started to work on patch
that remove len, sglen arguments from mt76u_buf_alloc() and use
q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
related code is now tangled.
Would be ok to send this patch with proper changelog as fix for RPI
against wireless-drivers and cc:stable (assuming it works and really
fix things on RPI) and revert Lorenzo's patches in -next ?
Stanislaw
From 4f8d7d3f4031b0a97b3bb147cb7e52533886e7cc Mon Sep 17 00:00:00 2001
From: Stanislaw Gruszka <[email protected]>
Date: Wed, 20 Feb 2019 13:29:42 +0100
Subject: [PATCH] mt76usb: use urb transfer_buffer for one segment
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 2 +
.../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 4 +-
drivers/net/wireless/mediatek/mt76/usb.c | 75 +++++++++++--------
3 files changed, 46 insertions(+), 35 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 2e5bcb3fdff7..7e0680daeee6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -86,6 +86,8 @@ struct mt76_queue_buf {
struct mt76u_buf {
struct mt76_dev *dev;
struct urb *urb;
+ struct scatterlist *sg;
+ 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 da299b8a1334..75561910d630 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
@@ -90,7 +90,7 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
if (urb->status)
return -EIO;
- data = sg_virt(&urb->sg[0]);
+ data = sg_virt(&buf->sg[0]);
if (usb->mcu.rp)
mt76x02u_multiple_mcu_reads(dev, data + 4,
urb->actual_length - 8);
@@ -266,7 +266,7 @@ static int
__mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
const void *fw_data, int len, u32 dst_addr)
{
- u8 *data = sg_virt(&buf->urb->sg[0]);
+ u8 *data = sg_virt(&buf->sg[0]);
DECLARE_COMPLETION_ONSTACK(cmpl);
__le32 info;
u32 val;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index a1811c39415e..57bb16eaff06 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -277,7 +277,6 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
int nsgs, int len, int sglen)
{
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
- struct urb *urb = buf->urb;
int i;
spin_lock_bh(&q->rx_page_lock);
@@ -292,21 +291,21 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
page = virt_to_head_page(data);
offset = data - page_address(page);
- sg_set_page(&urb->sg[i], page, sglen, offset);
+ sg_set_page(&buf->sg[i], page, sglen, offset);
}
spin_unlock_bh(&q->rx_page_lock);
if (i < nsgs) {
int j;
- for (j = nsgs; j < urb->num_sgs; j++)
- skb_free_frag(sg_virt(&urb->sg[j]));
- urb->num_sgs = i;
+ for (j = nsgs; j < buf->num_sgs; j++)
+ skb_free_frag(sg_virt(&buf->sg[j]));
+ 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(buf->sg, buf->num_sgs);
return i ? : -ENOMEM;
}
@@ -318,13 +317,14 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
if (!buf->urb)
return -ENOMEM;
- buf->urb->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->urb->sg),
+ buf->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->sg),
gfp);
- if (!buf->urb->sg)
+ if (!buf->sg)
return -ENOMEM;
- sg_init_table(buf->urb->sg, nsgs);
+ sg_init_table(buf->sg, nsgs);
buf->dev = dev;
+ buf->num_sgs = nsgs;
return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
}
@@ -332,12 +332,11 @@ EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
void mt76u_buf_free(struct mt76u_buf *buf)
{
- struct urb *urb = buf->urb;
struct scatterlist *sg;
int i;
- for (i = 0; i < urb->num_sgs; i++) {
- sg = &urb->sg[i];
+ for (i = 0; i < buf->num_sgs; i++) {
+ sg = &buf->sg[i];
if (!sg)
continue;
@@ -347,9 +346,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 +360,22 @@ 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 (buf->num_sgs > 1) {
+ buf->urb->num_sgs = buf->num_sgs;
+ buf->urb->sg = buf->sg;
+ } else {
+ buf->urb->transfer_buffer = sg_virt(buf->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,7 +685,7 @@ 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;
@@ -680,13 +693,14 @@ mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
skb_walk_frags(skb, iter)
nsgs += 1 + skb_shinfo(iter)->nr_frags;
- memset(urb->sg, 0, sizeof(*urb->sg) * MT_SG_MAX_SIZE);
+ memset(buf->sg, 0, sizeof(*buf->sg) * MT_SG_MAX_SIZE);
nsgs = min_t(int, MT_SG_MAX_SIZE, nsgs);
- sg_init_marker(urb->sg, nsgs);
- urb->num_sgs = nsgs;
+ sg_init_marker(buf->sg, nsgs);
+ buf->num_sgs = nsgs;
+ buf->len = skb->len;
- return skb_to_sgvec_nomark(skb, urb->sg, 0, skb->len);
+ return skb_to_sgvec_nomark(skb, buf->sg, 0, skb->len);
}
static int
@@ -694,12 +708,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 +723,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;
@@ -776,8 +785,8 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
if (!buf->urb)
return -ENOMEM;
- buf->urb->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
- if (!buf->urb->sg)
+ buf->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
+ if (!buf->sg)
return -ENOMEM;
}
}
--
2.20.1
> On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > >> >> The way I see it, we have two choices.
> > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > >> >
> > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > >> > if this is not a bug on other platforms as well.
> > >> >From what I can see, using Lorenzo's patches seems to be the better
> > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > >> drivers as well). I will apply them and then we'll see if we need to do
> > >> any further improvements later on.
> > >
> > > They work on rpi dwc2, but they do not address root of the problem.
> > > There is clearly something wrong how mt76usb handle SG, what is not
> > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > SG should be red flag.
> > I think we're simply dealing with multiple issues here, only some of
> > which are fixed by Lorenzo's patches.
> > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > since the Linux USB API supports non-aligned transfer buffers and it
> > should be up to the controller driver to deal with that.
>
> Agree.
>
> > dwc2 tries to do that, but that has limitations which I already pointed
> > out and which are properly dealt with by Lorenzo's patches.
>
> I planed to just accept current solution, but I started to work on patch
> that remove len, sglen arguments from mt76u_buf_alloc() and use
> q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> related code is now tangled.
>
> Would be ok to send this patch with proper changelog as fix for RPI
> against wireless-drivers and cc:stable (assuming it works and really
> fix things on RPI) and revert Lorenzo's patches in -next ?
Hi Stanislaw,
what is the advantage of doing so? You have duplicated most of the fields that are
already in the urb data structure and you use transfer_buffer (no SG I/O).
Moreover I have ready a series that removes 99% of the dual allocation code and
maintain it in the control path (instead of the datapath one).
I need just to rebase it ontop of your series. I will post it soon.
Regards,
Lorenzo
>
> Stanislaw
>
> From 4f8d7d3f4031b0a97b3bb147cb7e52533886e7cc Mon Sep 17 00:00:00 2001
> From: Stanislaw Gruszka <[email protected]>
> Date: Wed, 20 Feb 2019 13:29:42 +0100
> Subject: [PATCH] mt76usb: use urb transfer_buffer for one segment
>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/mediatek/mt76/mt76.h | 2 +
> .../wireless/mediatek/mt76/mt76x02_usb_mcu.c | 4 +-
> drivers/net/wireless/mediatek/mt76/usb.c | 75 +++++++++++--------
> 3 files changed, 46 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 2e5bcb3fdff7..7e0680daeee6 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -86,6 +86,8 @@ struct mt76_queue_buf {
> struct mt76u_buf {
> struct mt76_dev *dev;
> struct urb *urb;
> + struct scatterlist *sg;
> + 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 da299b8a1334..75561910d630 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_mcu.c
> @@ -90,7 +90,7 @@ static int mt76x02u_mcu_wait_resp(struct mt76_dev *dev, u8 seq)
> if (urb->status)
> return -EIO;
>
> - data = sg_virt(&urb->sg[0]);
> + data = sg_virt(&buf->sg[0]);
> if (usb->mcu.rp)
> mt76x02u_multiple_mcu_reads(dev, data + 4,
> urb->actual_length - 8);
> @@ -266,7 +266,7 @@ static int
> __mt76x02u_mcu_fw_send_data(struct mt76x02_dev *dev, struct mt76u_buf *buf,
> const void *fw_data, int len, u32 dst_addr)
> {
> - u8 *data = sg_virt(&buf->urb->sg[0]);
> + u8 *data = sg_virt(&buf->sg[0]);
> DECLARE_COMPLETION_ONSTACK(cmpl);
> __le32 info;
> u32 val;
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> index a1811c39415e..57bb16eaff06 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -277,7 +277,6 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
> int nsgs, int len, int sglen)
> {
> struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
> - struct urb *urb = buf->urb;
> int i;
>
> spin_lock_bh(&q->rx_page_lock);
> @@ -292,21 +291,21 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76u_buf *buf,
>
> page = virt_to_head_page(data);
> offset = data - page_address(page);
> - sg_set_page(&urb->sg[i], page, sglen, offset);
> + sg_set_page(&buf->sg[i], page, sglen, offset);
> }
> spin_unlock_bh(&q->rx_page_lock);
>
> if (i < nsgs) {
> int j;
>
> - for (j = nsgs; j < urb->num_sgs; j++)
> - skb_free_frag(sg_virt(&urb->sg[j]));
> - urb->num_sgs = i;
> + for (j = nsgs; j < buf->num_sgs; j++)
> + skb_free_frag(sg_virt(&buf->sg[j]));
> + 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(buf->sg, buf->num_sgs);
>
> return i ? : -ENOMEM;
> }
> @@ -318,13 +317,14 @@ int mt76u_buf_alloc(struct mt76_dev *dev, struct mt76u_buf *buf,
> if (!buf->urb)
> return -ENOMEM;
>
> - buf->urb->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->urb->sg),
> + buf->sg = devm_kcalloc(dev->dev, nsgs, sizeof(*buf->sg),
> gfp);
> - if (!buf->urb->sg)
> + if (!buf->sg)
> return -ENOMEM;
>
> - sg_init_table(buf->urb->sg, nsgs);
> + sg_init_table(buf->sg, nsgs);
> buf->dev = dev;
> + buf->num_sgs = nsgs;
>
> return mt76u_fill_rx_sg(dev, buf, nsgs, len, sglen);
> }
> @@ -332,12 +332,11 @@ EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
>
> void mt76u_buf_free(struct mt76u_buf *buf)
> {
> - struct urb *urb = buf->urb;
> struct scatterlist *sg;
> int i;
>
> - for (i = 0; i < urb->num_sgs; i++) {
> - sg = &urb->sg[i];
> + for (i = 0; i < buf->num_sgs; i++) {
> + sg = &buf->sg[i];
> if (!sg)
> continue;
>
> @@ -347,9 +346,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 +360,22 @@ 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 (buf->num_sgs > 1) {
> + buf->urb->num_sgs = buf->num_sgs;
> + buf->urb->sg = buf->sg;
> + } else {
> + buf->urb->transfer_buffer = sg_virt(buf->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,7 +685,7 @@ 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;
> @@ -680,13 +693,14 @@ mt76u_tx_build_sg(struct sk_buff *skb, struct urb *urb)
> skb_walk_frags(skb, iter)
> nsgs += 1 + skb_shinfo(iter)->nr_frags;
>
> - memset(urb->sg, 0, sizeof(*urb->sg) * MT_SG_MAX_SIZE);
> + memset(buf->sg, 0, sizeof(*buf->sg) * MT_SG_MAX_SIZE);
>
> nsgs = min_t(int, MT_SG_MAX_SIZE, nsgs);
> - sg_init_marker(urb->sg, nsgs);
> - urb->num_sgs = nsgs;
> + sg_init_marker(buf->sg, nsgs);
> + buf->num_sgs = nsgs;
> + buf->len = skb->len;
>
> - return skb_to_sgvec_nomark(skb, urb->sg, 0, skb->len);
> + return skb_to_sgvec_nomark(skb, buf->sg, 0, skb->len);
> }
>
> static int
> @@ -694,12 +708,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 +723,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;
> @@ -776,8 +785,8 @@ static int mt76u_alloc_tx(struct mt76_dev *dev)
> if (!buf->urb)
> return -ENOMEM;
>
> - buf->urb->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
> - if (!buf->urb->sg)
> + buf->sg = devm_kzalloc(dev->dev, size, GFP_KERNEL);
> + if (!buf->sg)
> return -ENOMEM;
> }
> }
> --
> 2.20.1
>
On Wed, 20 Feb 2019, Stanislaw Gruszka wrote:
> On Tue, Feb 19, 2019 at 10:40:47AM -0500, Alan Stern wrote:
> > On Tue, 19 Feb 2019, Stanislaw Gruszka wrote:
> >
> > > It would be interesting why urb->num_sgs = 0 & urb->sg cause
> > > the troubles. This is how usb_sg_init() submit urbs for sg_tablesize = 0
> > > controllers. So either are there are some requirement on urb->sg
> > > mapped via dma_map_page() (which mt76usb does not meet) not needed
> > > for urb->transfer_buffer mapped via dma_map_single() or there
> > > is something wrong in dwc2 with sg and this driver will not
> > > work with urb_sg_init() as well. I don't have hardware to investigate
> > > this and don't want to bother you with more patches.
> >
> > urb->sg != NULL and urb->num_sgs == 0 means that the transfer buffer
> > must fit into a single page, which is pointed to by the first element
> > of the scatterlist.
>
> I asked about that in other thread
> https://lore.kernel.org/linux-wireless/[email protected]/
>
> the answer was it is weird but valid. However I think do dma_map_page()
> on buffer not fit in the single page is asking for troubles. I just posted
> patch that should fix this for mt76usb.
>
> > But now that I look at the code in usb_sg_init(), it seems odd that the
> > !use_sg case doesn't increment sg during each loop iteration. I don't
> > see how that could possibly work -- it looks like a bug!
>
> Looks for me that this is done via for_each_sg(sg, sg, io->entries, i) loop.
Ah, of course. Thanks for straightening me out; it's surprising what
blind spots one's brain can develop.
Alan Stern
On Wed, Feb 20, 2019 at 02:22:08PM +0100, Lorenzo Bianconi wrote:
> > On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > > >> >> The way I see it, we have two choices.
> > > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > > >> >
> > > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > > >> > if this is not a bug on other platforms as well.
> > > >> >From what I can see, using Lorenzo's patches seems to be the better
> > > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > > >> drivers as well). I will apply them and then we'll see if we need to do
> > > >> any further improvements later on.
> > > >
> > > > They work on rpi dwc2, but they do not address root of the problem.
> > > > There is clearly something wrong how mt76usb handle SG, what is not
> > > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > > SG should be red flag.
> > > I think we're simply dealing with multiple issues here, only some of
> > > which are fixed by Lorenzo's patches.
> > > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > > since the Linux USB API supports non-aligned transfer buffers and it
> > > should be up to the controller driver to deal with that.
> >
> > Agree.
> >
> > > dwc2 tries to do that, but that has limitations which I already pointed
> > > out and which are properly dealt with by Lorenzo's patches.
> >
> > I planed to just accept current solution, but I started to work on patch
> > that remove len, sglen arguments from mt76u_buf_alloc() and use
> > q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> > related code is now tangled.
> >
> > Would be ok to send this patch with proper changelog as fix for RPI
> > against wireless-drivers and cc:stable (assuming it works and really
> > fix things on RPI) and revert Lorenzo's patches in -next ?
>
> Hi Stanislaw,
>
> what is the advantage of doing so?
To have small fix proper for -stable to fix the problem in 4.20 and 4.19,
and have simpler code.
> You have duplicated most of the fields that are
> already in the urb data structure and you use transfer_buffer (no SG I/O).
URB has plenty of fields, I duplicated 2. If size of mt76u_buf is a concern
this can be optimized by packing num_sgs, len, done into fields variable.
> Moreover I have ready a series that removes 99% of the dual allocation code and
> maintain it in the control path (instead of the datapath one).
> I need just to rebase it ontop of your series. I will post it soon.
So I would ask what is the point to adding bunch of code and remove it in very
next patch?
Stanislaw
> On Wed, Feb 20, 2019 at 02:22:08PM +0100, Lorenzo Bianconi wrote:
> > > On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > > > >> >> The way I see it, we have two choices.
> > > > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > > > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > > > >> >
> > > > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > > > >> > if this is not a bug on other platforms as well.
> > > > >> >From what I can see, using Lorenzo's patches seems to be the better
> > > > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > > > >> drivers as well). I will apply them and then we'll see if we need to do
> > > > >> any further improvements later on.
> > > > >
> > > > > They work on rpi dwc2, but they do not address root of the problem.
> > > > > There is clearly something wrong how mt76usb handle SG, what is not
> > > > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > > > SG should be red flag.
> > > > I think we're simply dealing with multiple issues here, only some of
> > > > which are fixed by Lorenzo's patches.
> > > > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > > > since the Linux USB API supports non-aligned transfer buffers and it
> > > > should be up to the controller driver to deal with that.
> > >
> > > Agree.
> > >
> > > > dwc2 tries to do that, but that has limitations which I already pointed
> > > > out and which are properly dealt with by Lorenzo's patches.
> > >
> > > I planed to just accept current solution, but I started to work on patch
> > > that remove len, sglen arguments from mt76u_buf_alloc() and use
> > > q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> > > related code is now tangled.
> > >
> > > Would be ok to send this patch with proper changelog as fix for RPI
> > > against wireless-drivers and cc:stable (assuming it works and really
> > > fix things on RPI) and revert Lorenzo's patches in -next ?
> >
> > Hi Stanislaw,
> >
> > what is the advantage of doing so?
> To have small fix proper for -stable to fix the problem in 4.20 and 4.19,
> and have simpler code.
merging the series I sent we will have a pretty simple approach, just a
single routine that allocates the rx buffers in the control path according to
the operating mode.
>
> > You have duplicated most of the fields that are
> > already in the urb data structure and you use transfer_buffer (no SG I/O).
> URB has plenty of fields, I duplicated 2. If size of mt76u_buf is a concern
> this can be optimized by packing num_sgs, len, done into fields variable.
>
> > Moreover I have ready a series that removes 99% of the dual allocation code and
> > maintain it in the control path (instead of the datapath one).
> > I need just to rebase it ontop of your series. I will post it soon.
> So I would ask what is the point to adding bunch of code and remove it in very
> next patch?
In the first series I fixed the issue, in this one I improved the code, I have
no added any new feature
Regards,
Lorenzo
>
> Stanislaw
On Wed, Feb 20, 2019 at 05:22:18PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Feb 20, 2019 at 02:22:08PM +0100, Lorenzo Bianconi wrote:
> > > > On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > > > > >> >> The way I see it, we have two choices.
> > > > > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > > > > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > > > > >> >
> > > > > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > > > > >> > if this is not a bug on other platforms as well.
> > > > > >> >From what I can see, using Lorenzo's patches seems to be the better
> > > > > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > > > > >> drivers as well). I will apply them and then we'll see if we need to do
> > > > > >> any further improvements later on.
> > > > > >
> > > > > > They work on rpi dwc2, but they do not address root of the problem.
> > > > > > There is clearly something wrong how mt76usb handle SG, what is not
> > > > > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > > > > SG should be red flag.
> > > > > I think we're simply dealing with multiple issues here, only some of
> > > > > which are fixed by Lorenzo's patches.
> > > > > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > > > > since the Linux USB API supports non-aligned transfer buffers and it
> > > > > should be up to the controller driver to deal with that.
> > > >
> > > > Agree.
> > > >
> > > > > dwc2 tries to do that, but that has limitations which I already pointed
> > > > > out and which are properly dealt with by Lorenzo's patches.
> > > >
> > > > I planed to just accept current solution, but I started to work on patch
> > > > that remove len, sglen arguments from mt76u_buf_alloc() and use
> > > > q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> > > > related code is now tangled.
> > > >
> > > > Would be ok to send this patch with proper changelog as fix for RPI
> > > > against wireless-drivers and cc:stable (assuming it works and really
> > > > fix things on RPI) and revert Lorenzo's patches in -next ?
> > >
> > > Hi Stanislaw,
> > >
> > > what is the advantage of doing so?
> > To have small fix proper for -stable to fix the problem in 4.20 and 4.19,
> > and have simpler code.
>
> merging the series I sent we will have a pretty simple approach, just a
> single routine that allocates the rx buffers in the control path according to
> the operating mode.
So will you do the backport of your patches and post them to -stable ?
Do you think greg-kh will accept those ?
Stanislaw
> On Wed, Feb 20, 2019 at 05:22:18PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Feb 20, 2019 at 02:22:08PM +0100, Lorenzo Bianconi wrote:
> > > > > On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > > > > > >> >> The way I see it, we have two choices.
> > > > > > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > > > > > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > > > > > >> >
> > > > > > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > > > > > >> > if this is not a bug on other platforms as well.
> > > > > > >> >From what I can see, using Lorenzo's patches seems to be the better
> > > > > > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > > > > > >> drivers as well). I will apply them and then we'll see if we need to do
> > > > > > >> any further improvements later on.
> > > > > > >
> > > > > > > They work on rpi dwc2, but they do not address root of the problem.
> > > > > > > There is clearly something wrong how mt76usb handle SG, what is not
> > > > > > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > > > > > SG should be red flag.
> > > > > > I think we're simply dealing with multiple issues here, only some of
> > > > > > which are fixed by Lorenzo's patches.
> > > > > > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > > > > > since the Linux USB API supports non-aligned transfer buffers and it
> > > > > > should be up to the controller driver to deal with that.
> > > > >
> > > > > Agree.
> > > > >
> > > > > > dwc2 tries to do that, but that has limitations which I already pointed
> > > > > > out and which are properly dealt with by Lorenzo's patches.
> > > > >
> > > > > I planed to just accept current solution, but I started to work on patch
> > > > > that remove len, sglen arguments from mt76u_buf_alloc() and use
> > > > > q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> > > > > related code is now tangled.
> > > > >
> > > > > Would be ok to send this patch with proper changelog as fix for RPI
> > > > > against wireless-drivers and cc:stable (assuming it works and really
> > > > > fix things on RPI) and revert Lorenzo's patches in -next ?
> > > >
> > > > Hi Stanislaw,
> > > >
> > > > what is the advantage of doing so?
> > > To have small fix proper for -stable to fix the problem in 4.20 and 4.19,
> > > and have simpler code.
> >
> > merging the series I sent we will have a pretty simple approach, just a
> > single routine that allocates the rx buffers in the control path according to
> > the operating mode.
>
> So will you do the backport of your patches and post them to -stable ?
> Do you think greg-kh will accept those ?
I will take care of it
Regards,
Lorenzo
>
> Stanislaw
Hi,
> Stanislaw Gruszka <[email protected]> hat am 20. Februar 2019 um 17:32 geschrieben:
>
>
> On Wed, Feb 20, 2019 at 05:22:18PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Feb 20, 2019 at 02:22:08PM +0100, Lorenzo Bianconi wrote:
> > > > > On Tue, Feb 19, 2019 at 01:19:26PM +0100, Felix Fietkau wrote:
> > > > > > >> >> The way I see it, we have two choices.
> > > > > > >> >> 1. Fix dwc2 to do its alignment quirk for the urb->sg != NULL case
> > > > > > >> >> 2. Rely on urb->transfer_buffer and keep urb->sg NULL
> > > > > > >> >
> > > > > > >> > I agree, if this is only needed for dwc2. Though I would investigate
> > > > > > >> > if this is not a bug on other platforms as well.
> > > > > > >> >From what I can see, using Lorenzo's patches seems to be the better
> > > > > > >> solution, since they avoid these corner cases in dwc2 (and maybe other
> > > > > > >> drivers as well). I will apply them and then we'll see if we need to do
> > > > > > >> any further improvements later on.
> > > > > > >
> > > > > > > They work on rpi dwc2, but they do not address root of the problem.
> > > > > > > There is clearly something wrong how mt76usb handle SG, what is not
> > > > > > > fixed. And adding disable_usb_sg module parameter for hcd's supporting
> > > > > > > SG should be red flag.
> > > > > > I think we're simply dealing with multiple issues here, only some of
> > > > > > which are fixed by Lorenzo's patches.
> > > > > > I'm pretty sure it's still wrong for mt76 to try to align its buffers,
> > > > > > since the Linux USB API supports non-aligned transfer buffers and it
> > > > > > should be up to the controller driver to deal with that.
> > > > >
> > > > > Agree.
> > > > >
> > > > > > dwc2 tries to do that, but that has limitations which I already pointed
> > > > > > out and which are properly dealt with by Lorenzo's patches.
> > > > >
> > > > > I planed to just accept current solution, but I started to work on patch
> > > > > that remove len, sglen arguments from mt76u_buf_alloc() and use
> > > > > q->buf_size and SKB_WITH_OVERHEAD(q->buf_size) directly and realized how
> > > > > related code is now tangled.
> > > > >
> > > > > Would be ok to send this patch with proper changelog as fix for RPI
> > > > > against wireless-drivers and cc:stable (assuming it works and really
> > > > > fix things on RPI) and revert Lorenzo's patches in -next ?
> > > >
> > > > Hi Stanislaw,
> > > >
> > > > what is the advantage of doing so?
> > > To have small fix proper for -stable to fix the problem in 4.20 and 4.19,
> > > and have simpler code.
> >
> > merging the series I sent we will have a pretty simple approach, just a
> > single routine that allocates the rx buffers in the control path according to
> > the operating mode.
>
FWIW i tested current linux-next and it also works with arm64/defconfig on Raspberry Pi 3B.
Thanks
>
> Stanislaw