| Message ID | 20220222095348.2926536-1-pbrobinson@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: bcmgenet: Return not supported if we don't have a WoL IRQ | expand |
Hello Peter, On 2/22/22 10:53, Peter Robinson wrote: > The ethtool WoL enable function wasn't checking if the device > has the optional WoL IRQ and hence on platforms such as the > Raspberry Pi 4 which had working ethernet prior to the last > fix regressed with the last fix, so also check if we have a > WoL IRQ there and return ENOTSUPP if not. > > Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > --- The patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards,
On 2/22/2022 1:53 AM, Peter Robinson wrote: > The ethtool WoL enable function wasn't checking if the device > has the optional WoL IRQ and hence on platforms such as the > Raspberry Pi 4 which had working ethernet prior to the last > fix regressed with the last fix, so also check if we have a > WoL IRQ there and return ENOTSUPP if not. > > Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ > 1 file changed, 4 insertions(+) > > We're seeing this crash on the Raspberry Pi 4 series of devices on > Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. Are you positive these two things are related to one another? The transmit queue timeout means that the TX DMA interrupt is not firing up what is the relationship with the absence/presence of the Wake-on-LAN interrupt line? At any rate: Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> On 2/22/2022 1:53 AM, Peter Robinson wrote: > > The ethtool WoL enable function wasn't checking if the device > > has the optional WoL IRQ and hence on platforms such as the > > Raspberry Pi 4 which had working ethernet prior to the last > > fix regressed with the last fix, so also check if we have a > > WoL IRQ there and return ENOTSUPP if not. > > > > Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > > Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > > Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > > Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > > --- > > drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > We're seeing this crash on the Raspberry Pi 4 series of devices on > > Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. > > Are you positive these two things are related to one another? The > transmit queue timeout means that the TX DMA interrupt is not firing up > what is the relationship with the absence/presence of the Wake-on-LAN > interrupt line? The first test I did was revert 9deb48b53e7f and the problem went away, then poked at a few bits and the patch also fixes it without having to revert the other fix. I don't know the HW well enough to know more. It seems there's other fixes/improvements that could be done around WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. This fix at least makes it work again in 5.17, I think improvements can be looked at later by something that actually knows their way around the driver and IP. Peter [1] https://elixir.bootlin.com/linux/v5.17-rc5/source/arch/arm/boot/dts/bcm2711.dtsi#L541 > At any rate: > > Acked-by: Florian Fainelli <f.fainelli@gmail.com> > -- > Florian
On 2/22/2022 12:07 PM, Peter Robinson wrote: >> On 2/22/2022 1:53 AM, Peter Robinson wrote: >>> The ethtool WoL enable function wasn't checking if the device >>> has the optional WoL IRQ and hence on platforms such as the >>> Raspberry Pi 4 which had working ethernet prior to the last >>> fix regressed with the last fix, so also check if we have a >>> WoL IRQ there and return ENOTSUPP if not. >>> >>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") >>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") >>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> >>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> >>> --- >>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> We're seeing this crash on the Raspberry Pi 4 series of devices on >>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. >> >> Are you positive these two things are related to one another? The >> transmit queue timeout means that the TX DMA interrupt is not firing up >> what is the relationship with the absence/presence of the Wake-on-LAN >> interrupt line? > > The first test I did was revert 9deb48b53e7f and the problem went > away, then poked at a few bits and the patch also fixes it without > having to revert the other fix. I don't know the HW well enough to > know more. > > It seems there's other fixes/improvements that could be done around > WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't > support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. There is no question we can report information more accurately and your patch fixes that. > > This fix at least makes it work again in 5.17, I think improvements > can be looked at later by something that actually knows their way > around the driver and IP. I happen to be that something, or rather consider myself a someone. But the DTS is perfectly well written and the Wake-on-LAN interrupt is optional, the driver assumes as per the binding documents that the Wake-on-LAN is the 3rd interrupt, when available. What I was hoping to get at is the output of /proc/interrupts for the good and the bad case so we can find out if by accident we end-up not using the appropriate interrupt number for the TX path. Not that I can see how that would happen, but since we have had some interesting issues being reported before when mixing upstream and downstream DTBs, I just don't fancy debugging that again: https://www.spinics.net/lists/arm-kernel/msg947308.html
On Tue, 22 Feb 2022 09:53:48 +0000 Peter Robinson wrote: > + /* We need a WoL IRQ to enable support, not all HW has one setup */ > + if (priv->wol_irq <= 0) > + return -ENOTSUPP; EOPNOTSUPP, ignore the existing code in this function and trust checkpatch's warning. ENOTSUPP should be avoided.
On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 2/22/2022 12:07 PM, Peter Robinson wrote: > >> On 2/22/2022 1:53 AM, Peter Robinson wrote: > >>> The ethtool WoL enable function wasn't checking if the device > >>> has the optional WoL IRQ and hence on platforms such as the > >>> Raspberry Pi 4 which had working ethernet prior to the last > >>> fix regressed with the last fix, so also check if we have a > >>> WoL IRQ there and return ENOTSUPP if not. > >>> > >>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > >>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > >>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > >>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > >>> --- > >>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> We're seeing this crash on the Raspberry Pi 4 series of devices on > >>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. > >> > >> Are you positive these two things are related to one another? The > >> transmit queue timeout means that the TX DMA interrupt is not firing up > >> what is the relationship with the absence/presence of the Wake-on-LAN > >> interrupt line? > > > > The first test I did was revert 9deb48b53e7f and the problem went > > away, then poked at a few bits and the patch also fixes it without > > having to revert the other fix. I don't know the HW well enough to > > know more. > > > > It seems there's other fixes/improvements that could be done around > > WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't > > support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. > > There is no question we can report information more accurately and your > patch fixes that. > > > > > This fix at least makes it work again in 5.17, I think improvements > > can be looked at later by something that actually knows their way > > around the driver and IP. > > I happen to be that something, or rather consider myself a someone. But > the DTS is perfectly well written and the Wake-on-LAN interrupt is > optional, the driver assumes as per the binding documents that the > Wake-on-LAN is the 3rd interrupt, when available. > > What I was hoping to get at is the output of /proc/interrupts for the > good and the bad case so we can find out if by accident we end-up not > using the appropriate interrupt number for the TX path. Not that I can > see how that would happen, but since we have had some interesting issues > being reported before when mixing upstream and downstream DTBs, I just > don't fancy debugging that again: The top two are pre/post plugging an ethernet cable with the patched kernel, the last two are the broken kernel. There doesn't seem to be a massive difference in interrupts but you likely know more of what you're looking for. Patched: [root@rpi400 ~]# cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 9: 0 0 0 0 GICv2 25 Level vgic 11: 16607 8639 5018 5793 GICv2 30 Level arch_timer 12: 0 0 0 0 GICv2 27 Level kvm guest vtimer 18: 0 0 0 0 GICv2 107 Level fe004000.txp 19: 1202 0 0 0 GICv2 65 Level fe00b880.mailbox 22: 4105 0 0 0 GICv2 125 Level ttyS0 23: 0 0 0 0 GICv2 129 Level vc4 hvs 24: 0 0 0 0 GICv2 105 Level fe980000.usb, fe980000.usb, dwc2_hsotg:usb3 26: 0 0 0 0 GICv2 112 Level DMA IRQ 28: 0 0 0 0 GICv2 114 Level DMA IRQ 35: 0 0 0 0 GICv2 141 Level vc4 crtc 36: 0 0 0 0 GICv2 142 Level vc4 crtc, vc4 crtc 37: 0 0 0 0 GICv2 133 Level vc4 crtc 38: 0 0 0 0 GICv2 138 Level vc4 crtc 41: 10661 0 0 0 GICv2 158 Level mmc0, mmc1 42: 0 0 0 0 GICv2 48 Level arm-pmu 43: 0 0 0 0 GICv2 49 Level arm-pmu 44: 0 0 0 0 GICv2 50 Level arm-pmu 45: 0 0 0 0 GICv2 51 Level arm-pmu 48: 3763 0 0 0 GICv2 189 Level eth0 49: 0 0 0 0 GICv2 190 Level eth0 57: 0 0 0 0 GICv2 175 Level PCIe PME, aerdrv 58: 69 0 0 0 BRCM STB PCIe MSI 524288 Edge xhci_hcd 59: 0 0 0 0 interrupt-controller@7ef00100 4 Edge vc4 hdmi hpd connected 60: 0 0 0 0 interrupt-controller@7ef00100 5 Edge vc4 hdmi hpd disconnected 61: 0 0 0 0 interrupt-controller@7ef00100 1 Edge vc4 hdmi cec rx 62: 0 0 0 0 interrupt-controller@7ef00100 0 Edge vc4 hdmi cec tx 63: 0 0 0 0 interrupt-controller@7ef00100 10 Edge vc4 hdmi hpd connected 64: 0 0 0 0 interrupt-controller@7ef00100 11 Edge vc4 hdmi hpd disconnected 65: 0 0 0 0 interrupt-controller@7ef00100 7 Edge vc4 hdmi cec rx 66: 0 0 0 0 interrupt-controller@7ef00100 8 Edge vc4 hdmi cec tx IPI0: 2984 9834 4892 4645 Rescheduling interrupts IPI1: 874 696 669 667 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 Timer broadcast interrupts IPI5: 121 86 103 79 IRQ work interrupts IPI6: 0 0 0 0 CPU wake-up interrupts Err: 0 [root@rpi400 ~]# [ 790.405408] bcmgenet fd580000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx [ 790.413783] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 9: 0 0 0 0 GICv2 25 Level vgic 11: 16769 9013 5132 5943 GICv2 30 Level arch_timer 12: 0 0 0 0 GICv2 27 Level kvm guest vtimer 18: 0 0 0 0 GICv2 107 Level fe004000.txp 19: 1263 0 0 0 GICv2 65 Level fe00b880.mailbox 22: 4566 0 0 0 GICv2 125 Level ttyS0 23: 0 0 0 0 GICv2 129 Level vc4 hvs 24: 0 0 0 0 GICv2 105 Level fe980000.usb, fe980000.usb, dwc2_hsotg:usb3 26: 0 0 0 0 GICv2 112 Level DMA IRQ 28: 0 0 0 0 GICv2 114 Level DMA IRQ 35: 0 0 0 0 GICv2 141 Level vc4 crtc 36: 0 0 0 0 GICv2 142 Level vc4 crtc, vc4 crtc 37: 0 0 0 0 GICv2 133 Level vc4 crtc 38: 0 0 0 0 GICv2 138 Level vc4 crtc 41: 10751 0 0 0 GICv2 158 Level mmc0, mmc1 42: 0 0 0 0 GICv2 48 Level arm-pmu 43: 0 0 0 0 GICv2 49 Level arm-pmu 44: 0 0 0 0 GICv2 50 Level arm-pmu 45: 0 0 0 0 GICv2 51 Level arm-pmu 48: 3838 0 0 0 GICv2 189 Level eth0 49: 33 0 0 0 GICv2 190 Level eth0 57: 0 0 0 0 GICv2 175 Level PCIe PME, aerdrv 58: 69 0 0 0 BRCM STB PCIe MSI 524288 Edge xhci_hcd 59: 0 0 0 0 interrupt-controller@7ef00100 4 Edge vc4 hdmi hpd connected 60: 0 0 0 0 interrupt-controller@7ef00100 5 Edge vc4 hdmi hpd disconnected 61: 0 0 0 0 interrupt-controller@7ef00100 1 Edge vc4 hdmi cec rx 62: 0 0 0 0 interrupt-controller@7ef00100 0 Edge vc4 hdmi cec tx 63: 0 0 0 0 interrupt-controller@7ef00100 10 Edge vc4 hdmi hpd connected 64: 0 0 0 0 interrupt-controller@7ef00100 11 Edge vc4 hdmi hpd disconnected 65: 0 0 0 0 interrupt-controller@7ef00100 7 Edge vc4 hdmi cec rx 66: 0 0 0 0 interrupt-controller@7ef00100 8 Edge vc4 hdmi cec tx IPI0: 3112 10093 5006 4796 Rescheduling interrupts IPI1: 891 729 679 671 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 Timer broadcast interrupts IPI5: 129 92 110 86 IRQ work interrupts IPI6: 0 0 0 0 CPU wake-up interrupts Err: 0 Broken: [root@rpi400 ~]# cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 9: 0 0 0 0 GICv2 25 Level vgic 11: 7651 5879 21793 19375 GICv2 30 Level arch_timer 12: 0 0 0 0 GICv2 27 Level kvm guest vtimer 18: 0 0 0 0 GICv2 107 Level fe004000.txp 19: 1719 0 0 0 GICv2 65 Level fe00b880.mailbox 22: 3118 0 0 0 GICv2 125 Level ttyS0 23: 0 0 0 0 GICv2 129 Level vc4 hvs 24: 0 0 0 0 GICv2 105 Level fe980000.usb, fe980000.usb, dwc2_hsotg:usb2 26: 0 0 0 0 GICv2 112 Level DMA IRQ 28: 0 0 0 0 GICv2 114 Level DMA IRQ 35: 0 0 0 0 GICv2 141 Level vc4 crtc 36: 0 0 0 0 GICv2 142 Level vc4 crtc, vc4 crtc 37: 0 0 0 0 GICv2 133 Level vc4 crtc 38: 0 0 0 0 GICv2 138 Level vc4 crtc 41: 11012 0 0 0 GICv2 158 Level mmc1, mmc0 42: 0 0 0 0 GICv2 48 Level arm-pmu 43: 0 0 0 0 GICv2 49 Level arm-pmu 44: 0 0 0 0 GICv2 50 Level arm-pmu 45: 0 0 0 0 GICv2 51 Level arm-pmu 48: 6518 0 0 0 GICv2 189 Level eth0 49: 0 0 0 0 GICv2 190 Level eth0 57: 0 0 0 0 GICv2 175 Level PCIe PME, aerdrv 58: 0 0 0 0 interrupt-controller@7ef00100 4 Edge vc4 hdmi hpd connected 59: 0 0 0 0 interrupt-controller@7ef00100 5 Edge vc4 hdmi hpd disconnected 60: 69 0 0 0 BRCM STB PCIe MSI 524288 Edge xhci_hcd 61: 0 0 0 0 interrupt-controller@7ef00100 1 Edge vc4 hdmi cec rx 62: 0 0 0 0 interrupt-controller@7ef00100 0 Edge vc4 hdmi cec tx 63: 0 0 0 0 interrupt-controller@7ef00100 10 Edge vc4 hdmi hpd connected 64: 0 0 0 0 interrupt-controller@7ef00100 11 Edge vc4 hdmi hpd disconnected 65: 0 0 0 0 interrupt-controller@7ef00100 7 Edge vc4 hdmi cec rx 66: 0 0 0 0 interrupt-controller@7ef00100 8 Edge vc4 hdmi cec tx IPI0: 3752 6274 11796 5915 Rescheduling interrupts IPI1: 840 590 537 764 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 Timer broadcast interrupts IPI5: 217 105 88 100 IRQ work interrupts IPI6: 0 0 0 0 CPU wake-up interrupts Err: 0 [root@rpi400 ~]# [ 1361.290396] bcmgenet fd580000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx [ 1361.298771] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready [ 1364.009923] ------------[ cut here ]------------ [ 1364.014648] NETDEV WATCHDOG: eth0 (bcmgenet): transmit queue 2 timed out [ 1364.021543] WARNING: CPU: 2 PID: 0 at net/sched/sch_generic.c:529 dev_watchdog+0x234/0x240 [ 1364.029955] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables nfnetlink btsdio bluetooth ecdh_generic brcmfmac cpufreq_dt brcmutil cfg80211 broadcom raspberrypi_cpufreq bcm_phy_lib snd_soc_hdmi_codec rfkill genet iproc_rng200 bcm2711_thermal mdio_bcm_unimac nvmem_rmem leds_gpio vfat fat fuse zram mmc_block vc4 snd_soc_core dwc2 snd_compress ac97_bus snd_pcm_dmaengine snd_pcm raspberrypi_hwmon udc_core crct10dif_ce gpio_raspberrypi_exp clk_bcm2711_dvp bcm2835_wdt bcm2835_dma snd_timer snd sdhci_iproc sdhci_pltfm soundcore sdhci pcie_brcmstb phy_generic drm_cma_helper i2c_dev aes_neon_bs [ 1364.097026] CPU: 2 PID: 0 Comm: swapper/2 Not tainted 5.17.0-0.rc3.89.fc36.aarch64 #1 [ 1364.104975] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2022.04-rc2 04/01/2022 [ 1364.113800] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 1364.120866] pc : dev_watchdog+0x234/0x240 [ 1364.124937] lr : dev_watchdog+0x234/0x240 [ 1364.129006] sp : ffff8000080aba40 [ 1364.132363] x29: ffff8000080aba40 x28: ffffa70da31c7000 x27: ffff8000080abb20 [ 1364.139614] x26: ffffa70da2c40000 x25: 0000000000000000 x24: ffffa70da31cec08 [ 1364.146864] x23: 0000000000000100 x22: ffffa70da31c7000 x21: ffff2c5242c30000 [ 1364.154112] x20: 0000000000000002 x19: ffff2c5242c304c8 x18: ffffffffffffffff [ 1364.161359] x17: ffff854558af2000 x16: ffff800008014000 x15: 0000000000000006 [ 1364.168606] x14: 0000000000000000 x13: 74756f2064656d69 x12: ffffa70da32bd5f0 [ 1364.175852] x11: 712074696d736e61 x10: ffffa70da32bd5f0 x9 : ffffa70da0e0c81c [ 1364.183098] x8 : 00000000ffffdfff x7 : 000000000000003f x6 : 0000000000000000 [ 1364.190345] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000001000 [ 1364.197592] x2 : 0000000000001000 x1 : 0000000000000005 x0 : 000000000000003c [ 1364.204839] Call trace: [ 1364.207315] dev_watchdog+0x234/0x240 [ 1364.211035] call_timer_fn+0x3c/0x15c [ 1364.214757] __run_timers.part.0+0x288/0x310 [ 1364.219093] run_timer_softirq+0x48/0x80 [ 1364.223076] __do_softirq+0x128/0x360 [ 1364.226791] __irq_exit_rcu+0x138/0x140 [ 1364.230688] irq_exit_rcu+0x1c/0x30 [ 1364.234229] el1_interrupt+0x38/0x54 [ 1364.237857] el1h_64_irq_handler+0x18/0x24 [ 1364.242013] el1h_64_irq+0x7c/0x80 [ 1364.245462] arch_cpu_idle+0x18/0x2c [ 1364.249089] default_idle_call+0x4c/0x140 [ 1364.253157] cpuidle_idle_call+0x14c/0x1a0 [ 1364.257318] do_idle+0xb0/0x100 [ 1364.260506] cpu_startup_entry+0x34/0x8c [ 1364.264488] secondary_start_kernel+0xe4/0x110 [ 1364.269001] __secondary_switched+0x94/0x98 [ 1364.273248] ---[ end trace 0000000000000000 ]--- [root@rpi400 ~]# cat /proc/interrupts CPU0 CPU1 CPU2 CPU3 9: 0 0 0 0 GICv2 25 Level vgic 11: 7878 5980 21922 19501 GICv2 30 Level arch_timer 12: 0 0 0 0 GICv2 27 Level kvm guest vtimer 18: 0 0 0 0 GICv2 107 Level fe004000.txp 19: 1761 0 0 0 GICv2 65 Level fe00b880.mailbox 22: 3588 0 0 0 GICv2 125 Level ttyS0 23: 0 0 0 0 GICv2 129 Level vc4 hvs 24: 0 0 0 0 GICv2 105 Level fe980000.usb, fe980000.usb, dwc2_hsotg:usb2 26: 0 0 0 0 GICv2 112 Level DMA IRQ 28: 0 0 0 0 GICv2 114 Level DMA IRQ 35: 0 0 0 0 GICv2 141 Level vc4 crtc 36: 0 0 0 0 GICv2 142 Level vc4 crtc, vc4 crtc 37: 0 0 0 0 GICv2 133 Level vc4 crtc 38: 0 0 0 0 GICv2 138 Level vc4 crtc 41: 11087 0 0 0 GICv2 158 Level mmc1, mmc0 42: 0 0 0 0 GICv2 48 Level arm-pmu 43: 0 0 0 0 GICv2 49 Level arm-pmu 44: 0 0 0 0 GICv2 50 Level arm-pmu 45: 0 0 0 0 GICv2 51 Level arm-pmu 48: 6551 0 0 0 GICv2 189 Level eth0 49: 0 0 0 0 GICv2 190 Level eth0 57: 0 0 0 0 GICv2 175 Level PCIe PME, aerdrv 58: 0 0 0 0 interrupt-controller@7ef00100 4 Edge vc4 hdmi hpd connected 59: 0 0 0 0 interrupt-controller@7ef00100 5 Edge vc4 hdmi hpd disconnected 60: 69 0 0 0 BRCM STB PCIe MSI 524288 Edge xhci_hcd 61: 0 0 0 0 interrupt-controller@7ef00100 1 Edge vc4 hdmi cec rx 62: 0 0 0 0 interrupt-controller@7ef00100 0 Edge vc4 hdmi cec tx 63: 0 0 0 0 interrupt-controller@7ef00100 10 Edge vc4 hdmi hpd connected 64: 0 0 0 0 interrupt-controller@7ef00100 11 Edge vc4 hdmi hpd disconnected 65: 0 0 0 0 interrupt-controller@7ef00100 7 Edge vc4 hdmi cec rx 66: 0 0 0 0 interrupt-controller@7ef00100 8 Edge vc4 hdmi cec tx IPI0: 3802 6344 11929 5993 Rescheduling interrupts IPI1: 852 595 542 770 Function call interrupts IPI2: 0 0 0 0 CPU stop interrupts IPI3: 0 0 0 0 CPU stop (for crash dump) interrupts IPI4: 0 0 0 0 Timer broadcast interrupts IPI5: 226 107 96 103 IRQ work interrupts IPI6: 0 0 0 0 CPU wake-up interrupts Err: 0
On 2/23/2022 3:40 AM, Peter Robinson wrote: > On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 2/22/2022 12:07 PM, Peter Robinson wrote: >>>> On 2/22/2022 1:53 AM, Peter Robinson wrote: >>>>> The ethtool WoL enable function wasn't checking if the device >>>>> has the optional WoL IRQ and hence on platforms such as the >>>>> Raspberry Pi 4 which had working ethernet prior to the last >>>>> fix regressed with the last fix, so also check if we have a >>>>> WoL IRQ there and return ENOTSUPP if not. >>>>> >>>>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") >>>>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") >>>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> >>>>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> >>>>> --- >>>>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ >>>>> 1 file changed, 4 insertions(+) >>>>> >>>>> We're seeing this crash on the Raspberry Pi 4 series of devices on >>>>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. >>>> >>>> Are you positive these two things are related to one another? The >>>> transmit queue timeout means that the TX DMA interrupt is not firing up >>>> what is the relationship with the absence/presence of the Wake-on-LAN >>>> interrupt line? >>> >>> The first test I did was revert 9deb48b53e7f and the problem went >>> away, then poked at a few bits and the patch also fixes it without >>> having to revert the other fix. I don't know the HW well enough to >>> know more. >>> >>> It seems there's other fixes/improvements that could be done around >>> WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't >>> support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. >> >> There is no question we can report information more accurately and your >> patch fixes that. >> >>> >>> This fix at least makes it work again in 5.17, I think improvements >>> can be looked at later by something that actually knows their way >>> around the driver and IP. >> >> I happen to be that something, or rather consider myself a someone. But >> the DTS is perfectly well written and the Wake-on-LAN interrupt is >> optional, the driver assumes as per the binding documents that the >> Wake-on-LAN is the 3rd interrupt, when available. >> >> What I was hoping to get at is the output of /proc/interrupts for the >> good and the bad case so we can find out if by accident we end-up not >> using the appropriate interrupt number for the TX path. Not that I can >> see how that would happen, but since we have had some interesting issues >> being reported before when mixing upstream and downstream DTBs, I just >> don't fancy debugging that again: > > The top two are pre/post plugging an ethernet cable with the patched > kernel, the last two are the broken kernel. There doesn't seem to be a > massive difference in interrupts but you likely know more of what > you're looking for. There is not a difference in the hardware interrupt numbers being claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and 158 + 32). In the broken case we can see that the second interrupt line (interrupt 190), which is the one that services the non-default TX queues does not fire up at all whereas it does in the patched case. The transmit queue timeout makes sense given that transmit queue 2 (which is not the default one, default is 0) has its interrupt serviced by the second interrupt line (190). We can see it not firing up, hence the timeout. What I *think* might be happening here is the following: - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative error code we do not install the interrupt handler for the WoL interrupt since it is not valid - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is able to resolve that irq number to a valid interrupt descriptor - eventually we just mess up the interrupt descriptor for interrupt 49 and it stops working Now since this appears to be an ACPI-enabled system, we may be hitting this part of the code in platform_get_irq_optional(): r = platform_get_resource(dev, IORESOURCE_IRQ, num); if (has_acpi_companion(&dev->dev)) { if (r && r->flags & IORESOURCE_DISABLED) { ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), num, r); if (ret) goto out; } } and then I am not clear what interrupt this translates into here, or whether it is possible to get a valid interrupt descriptor here. The patch is fine in itself, but I would really prefer that we get to the bottom of this rather than have a superficial understanding of the nature of the problem. Thanks for providing these dumps.
On Wed, Feb 23, 2022 at 5:35 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 2/23/2022 3:40 AM, Peter Robinson wrote: > > On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > >> > >> > >> > >> On 2/22/2022 12:07 PM, Peter Robinson wrote: > >>>> On 2/22/2022 1:53 AM, Peter Robinson wrote: > >>>>> The ethtool WoL enable function wasn't checking if the device > >>>>> has the optional WoL IRQ and hence on platforms such as the > >>>>> Raspberry Pi 4 which had working ethernet prior to the last > >>>>> fix regressed with the last fix, so also check if we have a > >>>>> WoL IRQ there and return ENOTSUPP if not. > >>>>> > >>>>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > >>>>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > >>>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > >>>>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > >>>>> --- > >>>>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ > >>>>> 1 file changed, 4 insertions(+) > >>>>> > >>>>> We're seeing this crash on the Raspberry Pi 4 series of devices on > >>>>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. > >>>> > >>>> Are you positive these two things are related to one another? The > >>>> transmit queue timeout means that the TX DMA interrupt is not firing up > >>>> what is the relationship with the absence/presence of the Wake-on-LAN > >>>> interrupt line? > >>> > >>> The first test I did was revert 9deb48b53e7f and the problem went > >>> away, then poked at a few bits and the patch also fixes it without > >>> having to revert the other fix. I don't know the HW well enough to > >>> know more. > >>> > >>> It seems there's other fixes/improvements that could be done around > >>> WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't > >>> support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. > >> > >> There is no question we can report information more accurately and your > >> patch fixes that. > >> > >>> > >>> This fix at least makes it work again in 5.17, I think improvements > >>> can be looked at later by something that actually knows their way > >>> around the driver and IP. > >> > >> I happen to be that something, or rather consider myself a someone. But > >> the DTS is perfectly well written and the Wake-on-LAN interrupt is > >> optional, the driver assumes as per the binding documents that the > >> Wake-on-LAN is the 3rd interrupt, when available. > >> > >> What I was hoping to get at is the output of /proc/interrupts for the > >> good and the bad case so we can find out if by accident we end-up not > >> using the appropriate interrupt number for the TX path. Not that I can > >> see how that would happen, but since we have had some interesting issues > >> being reported before when mixing upstream and downstream DTBs, I just > >> don't fancy debugging that again: > > > > The top two are pre/post plugging an ethernet cable with the patched > > kernel, the last two are the broken kernel. There doesn't seem to be a > > massive difference in interrupts but you likely know more of what > > you're looking for. > > There is not a difference in the hardware interrupt numbers being > claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and > 158 + 32). In the broken case we can see that the second interrupt line > (interrupt 190), which is the one that services the non-default TX > queues does not fire up at all whereas it does in the patched case. > > The transmit queue timeout makes sense given that transmit queue 2 > (which is not the default one, default is 0) has its interrupt serviced > by the second interrupt line (190). We can see it not firing up, hence > the timeout. > > What I *think* might be happening here is the following: > > - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative > error code we do not install the interrupt handler for the WoL interrupt > since it is not valid > > - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we > call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is > able to resolve that irq number to a valid interrupt descriptor > > - eventually we just mess up the interrupt descriptor for interrupt 49 > and it stops working > > Now since this appears to be an ACPI-enabled system, we may be hitting > this part of the code in platform_get_irq_optional(): This system is not booting ACPI, it's booting UEFI+DT, the system is Fedora so overall the OS supports ACPI or device tree, but the error is from a system booted with device tree. > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > if (has_acpi_companion(&dev->dev)) { > if (r && r->flags & IORESOURCE_DISABLED) { > ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), > num, r); > if (ret) > goto out; > } > } > > and then I am not clear what interrupt this translates into here, or > whether it is possible to get a valid interrupt descriptor here. > > The patch is fine in itself, but I would really prefer that we get to > the bottom of this rather than have a superficial understanding of the > nature of the problem. > > Thanks for providing these dumps. > -- > Florian
> > The top two are pre/post plugging an ethernet cable with the patched > > kernel, the last two are the broken kernel. There doesn't seem to be a > > massive difference in interrupts but you likely know more of what > > you're looking for. > > There is not a difference in the hardware interrupt numbers being > claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and > 158 + 32). In the broken case we can see that the second interrupt line > (interrupt 190), which is the one that services the non-default TX > queues does not fire up at all whereas it does in the patched case. > > The transmit queue timeout makes sense given that transmit queue 2 > (which is not the default one, default is 0) has its interrupt serviced > by the second interrupt line (190). We can see it not firing up, hence > the timeout. > > What I *think* might be happening here is the following: > > - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative > error code we do not install the interrupt handler for the WoL interrupt > since it is not valid > > - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we > call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is > able to resolve that irq number to a valid interrupt descriptor > > - eventually we just mess up the interrupt descriptor for interrupt 49 > and it stops working > > Now since this appears to be an ACPI-enabled system, we may be hitting > this part of the code in platform_get_irq_optional(): > > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > if (has_acpi_companion(&dev->dev)) { > if (r && r->flags & IORESOURCE_DISABLED) { > ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), > num, r); > if (ret) > goto out; > } > } > > and then I am not clear what interrupt this translates into here, or > whether it is possible to get a valid interrupt descriptor here. > > The patch is fine in itself, but I would really prefer that we get to > the bottom of this rather than have a superficial understanding of the > nature of the problem. I have no problems working with you to improve the driver, the problem I have is this is currently a regression in 5.17 so I would like to see something land, whether it's reverting the other patch, landing thing one or another straight forward fix and then maybe revisit as whole in 5.18. > Thanks for providing these dumps. > -- > Florian
On 2/23/2022 9:45 AM, Peter Robinson wrote: >>> The top two are pre/post plugging an ethernet cable with the patched >>> kernel, the last two are the broken kernel. There doesn't seem to be a >>> massive difference in interrupts but you likely know more of what >>> you're looking for. >> >> There is not a difference in the hardware interrupt numbers being >> claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and >> 158 + 32). In the broken case we can see that the second interrupt line >> (interrupt 190), which is the one that services the non-default TX >> queues does not fire up at all whereas it does in the patched case. >> >> The transmit queue timeout makes sense given that transmit queue 2 >> (which is not the default one, default is 0) has its interrupt serviced >> by the second interrupt line (190). We can see it not firing up, hence >> the timeout. >> >> What I *think* might be happening here is the following: >> >> - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative >> error code we do not install the interrupt handler for the WoL interrupt >> since it is not valid >> >> - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we >> call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is >> able to resolve that irq number to a valid interrupt descriptor >> >> - eventually we just mess up the interrupt descriptor for interrupt 49 >> and it stops working >> >> Now since this appears to be an ACPI-enabled system, we may be hitting >> this part of the code in platform_get_irq_optional(): >> >> r = platform_get_resource(dev, IORESOURCE_IRQ, num); >> if (has_acpi_companion(&dev->dev)) { >> if (r && r->flags & IORESOURCE_DISABLED) { >> ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), >> num, r); >> if (ret) >> goto out; >> } >> } >> >> and then I am not clear what interrupt this translates into here, or >> whether it is possible to get a valid interrupt descriptor here. >> >> The patch is fine in itself, but I would really prefer that we get to >> the bottom of this rather than have a superficial understanding of the >> nature of the problem. > > I have no problems working with you to improve the driver, the problem > I have is this is currently a regression in 5.17 so I would like to > see something land, whether it's reverting the other patch, landing > thing one or another straight forward fix and then maybe revisit as > whole in 5.18. Understood and I won't require you or me to complete this investigating before fixing the regression, this is just so we understand where it stemmed from and possibly fix the IRQ layer if need be. Given what I just wrote, do you think you can sprinkle debug prints throughout the kernel to figure out whether enable_irq_wake() somehow messes up the interrupt descriptor of interrupt and test that theory? We can do that offline if you want.
On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: > > I have no problems working with you to improve the driver, the problem > > I have is this is currently a regression in 5.17 so I would like to > > see something land, whether it's reverting the other patch, landing > > thing one or another straight forward fix and then maybe revisit as > > whole in 5.18. > > Understood and I won't require you or me to complete this investigating > before fixing the regression, this is just so we understand where it > stemmed from and possibly fix the IRQ layer if need be. Given what I > just wrote, do you think you can sprinkle debug prints throughout the > kernel to figure out whether enable_irq_wake() somehow messes up the > interrupt descriptor of interrupt and test that theory? We can do that > offline if you want. Let me mark v2 as Deferred for now, then. I'm not really sure if that's what's intended but we have 3 weeks or so until 5.17 is cut so we can afford a few days of investigating. I'm likely missing the point but sounds like the IRQ subsystem treats IRQ numbers as unsigned so if we pass a negative value "fun" is sort of expected. Isn't the problem that device somehow comes with wakeup capable being set already? Isn't it better to make sure device is not wake capable if there's no WoL irq instead of adding second check? diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index cfe09117fe6c..7dea44803beb 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev) /* Request the WOL interrupt and advertise suspend if available */ priv->wol_irq_disabled = true; - if (priv->wol_irq > 0) { + if (priv->wol_irq > 0) err = devm_request_irq(&pdev->dev, priv->wol_irq, bcmgenet_wol_isr, 0, dev->name, priv); - if (!err) - device_set_wakeup_capable(&pdev->dev, 1); - } + else + err = -ENOENT; + device_set_wakeup_capable(&pdev->dev, !err); /* Set the needed headroom to account for any possible * features enabling/disabling at runtime
On 2/23/2022 2:48 PM, Jakub Kicinski wrote: > On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>> I have no problems working with you to improve the driver, the problem >>> I have is this is currently a regression in 5.17 so I would like to >>> see something land, whether it's reverting the other patch, landing >>> thing one or another straight forward fix and then maybe revisit as >>> whole in 5.18. >> >> Understood and I won't require you or me to complete this investigating >> before fixing the regression, this is just so we understand where it >> stemmed from and possibly fix the IRQ layer if need be. Given what I >> just wrote, do you think you can sprinkle debug prints throughout the >> kernel to figure out whether enable_irq_wake() somehow messes up the >> interrupt descriptor of interrupt and test that theory? We can do that >> offline if you want. > > Let me mark v2 as Deferred for now, then. I'm not really sure if that's > what's intended but we have 3 weeks or so until 5.17 is cut so we can > afford a few days of investigating. > > I'm likely missing the point but sounds like the IRQ subsystem treats > IRQ numbers as unsigned so if we pass a negative value "fun" is sort > of expected. Isn't the problem that device somehow comes with wakeup > capable being set already? Isn't it better to make sure device is not > wake capable if there's no WoL irq instead of adding second check? > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index cfe09117fe6c..7dea44803beb 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev) > > /* Request the WOL interrupt and advertise suspend if available */ > priv->wol_irq_disabled = true; > - if (priv->wol_irq > 0) { > + if (priv->wol_irq > 0) > err = devm_request_irq(&pdev->dev, priv->wol_irq, > bcmgenet_wol_isr, 0, dev->name, priv); > - if (!err) > - device_set_wakeup_capable(&pdev->dev, 1); > - } > + else > + err = -ENOENT; > + device_set_wakeup_capable(&pdev->dev, !err); Yes, this looks more elegant and is certainly more correct. However there must have been something else going on with Peter's provided information. We clearly did not have an interrupt registered for the Wake-on-LAN interrupt line as witnessed by the outputs of /proc/interrupts, however if we managed to go past the device_can_wakeup() check in bcmgenet_set_wol(), then we must have had devm_request_irq() return success on an invalid interrupt number or worse, botch the interrupt number in priv->irq1 to the point where the handler got re-installed maybe and we only end-up calling bcmgenet_wol_isr but no longer bcmgenet_isr1.. Hummm.
On Wed, 23 Feb 2022 14:58:13 -0800 Florian Fainelli wrote: > Yes, this looks more elegant and is certainly more correct. > > However there must have been something else going on with Peter's > provided information. > > We clearly did not have an interrupt registered for the Wake-on-LAN > interrupt line as witnessed by the outputs of /proc/interrupts, however > if we managed to go past the device_can_wakeup() check in > bcmgenet_set_wol(), then we must have had devm_request_irq() return > success on an invalid interrupt number My thinking was we never called devm_request_irq(). > or worse, botch the interrupt number in priv->irq1 to the point where > the handler got re-installed maybe and we only end-up calling > bcmgenet_wol_isr but no longer bcmgenet_isr1.. Hummm. You're right, my thinking was maybe some IRQ code casts the IRQ number to u8, but irq_to_desc() contains no such silliness and should be exercised on every path :S
> On 2/23/2022 9:45 AM, Peter Robinson wrote: > >>> The top two are pre/post plugging an ethernet cable with the patched > >>> kernel, the last two are the broken kernel. There doesn't seem to be a > >>> massive difference in interrupts but you likely know more of what > >>> you're looking for. > >> > >> There is not a difference in the hardware interrupt numbers being > >> claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and > >> 158 + 32). In the broken case we can see that the second interrupt line > >> (interrupt 190), which is the one that services the non-default TX > >> queues does not fire up at all whereas it does in the patched case. > >> > >> The transmit queue timeout makes sense given that transmit queue 2 > >> (which is not the default one, default is 0) has its interrupt serviced > >> by the second interrupt line (190). We can see it not firing up, hence > >> the timeout. > >> > >> What I *think* might be happening here is the following: > >> > >> - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative > >> error code we do not install the interrupt handler for the WoL interrupt > >> since it is not valid > >> > >> - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we > >> call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is > >> able to resolve that irq number to a valid interrupt descriptor > >> > >> - eventually we just mess up the interrupt descriptor for interrupt 49 > >> and it stops working > >> > >> Now since this appears to be an ACPI-enabled system, we may be hitting > >> this part of the code in platform_get_irq_optional(): > >> > >> r = platform_get_resource(dev, IORESOURCE_IRQ, num); > >> if (has_acpi_companion(&dev->dev)) { > >> if (r && r->flags & IORESOURCE_DISABLED) { > >> ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), > >> num, r); > >> if (ret) > >> goto out; > >> } > >> } > >> > >> and then I am not clear what interrupt this translates into here, or > >> whether it is possible to get a valid interrupt descriptor here. > >> > >> The patch is fine in itself, but I would really prefer that we get to > >> the bottom of this rather than have a superficial understanding of the > >> nature of the problem. > > > > I have no problems working with you to improve the driver, the problem > > I have is this is currently a regression in 5.17 so I would like to > > see something land, whether it's reverting the other patch, landing > > thing one or another straight forward fix and then maybe revisit as > > whole in 5.18. > > Understood and I won't require you or me to complete this investigating > before fixing the regression, this is just so we understand where it > stemmed from and possibly fix the IRQ layer if need be. Given what I > just wrote, do you think you can sprinkle debug prints throughout the > kernel to figure out whether enable_irq_wake() somehow messes up the > interrupt descriptor of interrupt and test that theory? We can do that > offline if you want. Yes, I can do that, may be quicker if you send me a rough patch of the debugs you like as that'll be likely less round trips, happy to deal offline. P
Hi, On 2/23/22 11:35, Florian Fainelli wrote: > > > On 2/23/2022 3:40 AM, Peter Robinson wrote: >> On Tue, Feb 22, 2022 at 8:15 PM Florian Fainelli >> <f.fainelli@gmail.com> wrote: >>> >>> >>> >>> On 2/22/2022 12:07 PM, Peter Robinson wrote: >>>>> On 2/22/2022 1:53 AM, Peter Robinson wrote: >>>>>> The ethtool WoL enable function wasn't checking if the device >>>>>> has the optional WoL IRQ and hence on platforms such as the >>>>>> Raspberry Pi 4 which had working ethernet prior to the last >>>>>> fix regressed with the last fix, so also check if we have a >>>>>> WoL IRQ there and return ENOTSUPP if not. >>>>>> >>>>>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") >>>>>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") >>>>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> >>>>>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> >>>>>> --- >>>>>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ >>>>>> 1 file changed, 4 insertions(+) >>>>>> >>>>>> We're seeing this crash on the Raspberry Pi 4 series of devices on >>>>>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet >>>>>> doesn't work. >>>>> >>>>> Are you positive these two things are related to one another? The >>>>> transmit queue timeout means that the TX DMA interrupt is not >>>>> firing up >>>>> what is the relationship with the absence/presence of the Wake-on-LAN >>>>> interrupt line? >>>> >>>> The first test I did was revert 9deb48b53e7f and the problem went >>>> away, then poked at a few bits and the patch also fixes it without >>>> having to revert the other fix. I don't know the HW well enough to >>>> know more. >>>> >>>> It seems there's other fixes/improvements that could be done around >>>> WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't >>>> support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. >>> >>> There is no question we can report information more accurately and your >>> patch fixes that. >>> >>>> >>>> This fix at least makes it work again in 5.17, I think improvements >>>> can be looked at later by something that actually knows their way >>>> around the driver and IP. >>> >>> I happen to be that something, or rather consider myself a someone. But >>> the DTS is perfectly well written and the Wake-on-LAN interrupt is >>> optional, the driver assumes as per the binding documents that the >>> Wake-on-LAN is the 3rd interrupt, when available. >>> >>> What I was hoping to get at is the output of /proc/interrupts for the >>> good and the bad case so we can find out if by accident we end-up not >>> using the appropriate interrupt number for the TX path. Not that I can >>> see how that would happen, but since we have had some interesting issues >>> being reported before when mixing upstream and downstream DTBs, I just >>> don't fancy debugging that again: >> >> The top two are pre/post plugging an ethernet cable with the patched >> kernel, the last two are the broken kernel. There doesn't seem to be a >> massive difference in interrupts but you likely know more of what >> you're looking for. > > There is not a difference in the hardware interrupt numbers being > claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and > 158 + 32). In the broken case we can see that the second interrupt line > (interrupt 190), which is the one that services the non-default TX > queues does not fire up at all whereas it does in the patched case. > > The transmit queue timeout makes sense given that transmit queue 2 > (which is not the default one, default is 0) has its interrupt serviced > by the second interrupt line (190). We can see it not firing up, hence > the timeout. > > What I *think* might be happening here is the following: > > - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative > error code we do not install the interrupt handler for the WoL interrupt > since it is not valid > > - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we > call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is > able to resolve that irq number to a valid interrupt descriptor That should not be possible, see below. > > - eventually we just mess up the interrupt descriptor for interrupt 49 > and it stops working > > Now since this appears to be an ACPI-enabled system, we may be hitting > this part of the code in platform_get_irq_optional(): > > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > if (has_acpi_companion(&dev->dev)) { > if (r && r->flags & IORESOURCE_DISABLED) { > ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), > num, r); > if (ret) > goto out; > } > } As Peter points out, he is using uboot/DT. I on the other hand am not having any issues with fedora on the edk2/ACPI rpi4 with 5.17rc's. Although, I found this series interesting because I didn't (still don't, although I have a couple theories) see why the same bug shouldn't be affecting ACPI. Also, I don't actually understand how Peter's patch fixes the problem. That is because, device_set_wakeup_capable() isn't setting can_wakeup, thus the machine should immediately be returning from bcmgetnet_set_wol() when it checks device_can_wakeup(). Meaning it shouldn't ever execute the wol_irq <= 0 check being added by this patch. On the working/ACPI machine that is true, and it actually results in an unusual ethtool error. So, understanding how that gets set (and maybe adding an wakeup_capable(,false), like a couple other drivers) is the right path here? It should be 0, but I can't prove that to myself right now. Which brings me to my second point about ethtool. The return from bcmgenet_get_wol() is incorrect on these platforms, and that is why bcmgetnet_set_wol() is even being called. I have a patch I will post, to fix it, but its basically adding a device_can_wakeup() check to _get_wol() and returning wol->supported = 0; wol->wolopts=0; Finally, more on the thinking out loud theory, it came to my attention that some of the fedora kernels were being built with gcc11 (my rpi test kernels for sure) and some with gcc12? Is the failing kernel built with gcc12? > > and then I am not clear what interrupt this translates into here, or > whether it is possible to get a valid interrupt descriptor here. > > The patch is fine in itself, but I would really prefer that we get to > the bottom of this rather than have a superficial understanding of the > nature of the problem. > > Thanks for providing these dumps.
> >>>>> On 2/22/2022 1:53 AM, Peter Robinson wrote: > >>>>>> The ethtool WoL enable function wasn't checking if the device > >>>>>> has the optional WoL IRQ and hence on platforms such as the > >>>>>> Raspberry Pi 4 which had working ethernet prior to the last > >>>>>> fix regressed with the last fix, so also check if we have a > >>>>>> WoL IRQ there and return ENOTSUPP if not. > >>>>>> > >>>>>> Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") > >>>>>> Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") > >>>>>> Signed-off-by: Peter Robinson <pbrobinson@gmail.com> > >>>>>> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> > >>>>>> --- > >>>>>> drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ > >>>>>> 1 file changed, 4 insertions(+) > >>>>>> > >>>>>> We're seeing this crash on the Raspberry Pi 4 series of devices on > >>>>>> Fedora on 5.17-rc with the top Fixes patch and wired ethernet > >>>>>> doesn't work. > >>>>> > >>>>> Are you positive these two things are related to one another? The > >>>>> transmit queue timeout means that the TX DMA interrupt is not > >>>>> firing up > >>>>> what is the relationship with the absence/presence of the Wake-on-LAN > >>>>> interrupt line? > >>>> > >>>> The first test I did was revert 9deb48b53e7f and the problem went > >>>> away, then poked at a few bits and the patch also fixes it without > >>>> having to revert the other fix. I don't know the HW well enough to > >>>> know more. > >>>> > >>>> It seems there's other fixes/improvements that could be done around > >>>> WOL in the driver, the bcm2711 SoC at least in the upstream DT doesn't > >>>> support/implement a WOL IRQ, yet the RPi4 reports it supports WOL. > >>> > >>> There is no question we can report information more accurately and your > >>> patch fixes that. > >>> > >>>> > >>>> This fix at least makes it work again in 5.17, I think improvements > >>>> can be looked at later by something that actually knows their way > >>>> around the driver and IP. > >>> > >>> I happen to be that something, or rather consider myself a someone. But > >>> the DTS is perfectly well written and the Wake-on-LAN interrupt is > >>> optional, the driver assumes as per the binding documents that the > >>> Wake-on-LAN is the 3rd interrupt, when available. > >>> > >>> What I was hoping to get at is the output of /proc/interrupts for the > >>> good and the bad case so we can find out if by accident we end-up not > >>> using the appropriate interrupt number for the TX path. Not that I can > >>> see how that would happen, but since we have had some interesting issues > >>> being reported before when mixing upstream and downstream DTBs, I just > >>> don't fancy debugging that again: > >> > >> The top two are pre/post plugging an ethernet cable with the patched > >> kernel, the last two are the broken kernel. There doesn't seem to be a > >> massive difference in interrupts but you likely know more of what > >> you're looking for. > > > > There is not a difference in the hardware interrupt numbers being > > claimed by GENET which are both GIC interrupts 189 and 190 (157 + 32 and > > 158 + 32). In the broken case we can see that the second interrupt line > > (interrupt 190), which is the one that services the non-default TX > > queues does not fire up at all whereas it does in the patched case. > > > > The transmit queue timeout makes sense given that transmit queue 2 > > (which is not the default one, default is 0) has its interrupt serviced > > by the second interrupt line (190). We can see it not firing up, hence > > the timeout. > > > > What I *think* might be happening here is the following: > > > > - priv->wol_irq = platform_get_irq_optional(pdev, 2) returns a negative > > error code we do not install the interrupt handler for the WoL interrupt > > since it is not valid > > > > - bcmgenet_set_wol() is called, we do not check priv->wol_irq, so we > > call enable_irq_wake(priv->wol_irq) and somehow irq_set_irq_wake() is > > able to resolve that irq number to a valid interrupt descriptor > > That should not be possible, see below. > > > > > - eventually we just mess up the interrupt descriptor for interrupt 49 > > and it stops working > > > > Now since this appears to be an ACPI-enabled system, we may be hitting > > this part of the code in platform_get_irq_optional(): > > > > r = platform_get_resource(dev, IORESOURCE_IRQ, num); > > if (has_acpi_companion(&dev->dev)) { > > if (r && r->flags & IORESOURCE_DISABLED) { > > ret = acpi_irq_get(ACPI_HANDLE(&dev->dev), > > num, r); > > if (ret) > > goto out; > > } > > } > > As Peter points out, he is using uboot/DT. I on the other hand am not > having any issues with fedora on the edk2/ACPI rpi4 with 5.17rc's. > > Although, I found this series interesting because I didn't (still don't, > although I have a couple theories) see why the same bug shouldn't be > affecting ACPI. > > Also, I don't actually understand how Peter's patch fixes the problem. > That is because, device_set_wakeup_capable() isn't setting can_wakeup, > thus the machine should immediately be returning from > bcmgetnet_set_wol() when it checks device_can_wakeup(). Meaning it > shouldn't ever execute the wol_irq <= 0 check being added by this patch. > > On the working/ACPI machine that is true, and it actually results in an > unusual ethtool error. So, understanding how that gets set (and maybe > adding an wakeup_capable(,false), like a couple other drivers) is the > right path here? It should be 0, but I can't prove that to myself right now. > > Which brings me to my second point about ethtool. The return from > bcmgenet_get_wol() is incorrect on these platforms, and that is why > bcmgetnet_set_wol() is even being called. I have a patch I will post, to > fix it, but its basically adding a device_can_wakeup() check to > _get_wol() and returning wol->supported = 0; wol->wolopts=0; > > Finally, more on the thinking out loud theory, it came to my attention > that some of the fedora kernels were being built with gcc11 (my rpi test > kernels for sure) and some with gcc12? Is the failing kernel built with > gcc12? Yes, all the 5.17-rcX kernels in Fedora are currently built with gcc12.
On Wed, 23 Feb 2022 14:48:18 -0800 Jakub Kicinski wrote: > > Understood and I won't require you or me to complete this investigating > > before fixing the regression, this is just so we understand where it > > stemmed from and possibly fix the IRQ layer if need be. Given what I > > just wrote, do you think you can sprinkle debug prints throughout the > > kernel to figure out whether enable_irq_wake() somehow messes up the > > interrupt descriptor of interrupt and test that theory? We can do that > > offline if you want. > > Let me mark v2 as Deferred for now, then. I'm not really sure if that's > what's intended but we have 3 weeks or so until 5.17 is cut so we can > afford a few days of investigating. I think the "few days of investigating" have now run out :( How should we proceed?
On 3/2/2022 10:02 AM, Jakub Kicinski wrote: > On Wed, 23 Feb 2022 14:48:18 -0800 Jakub Kicinski wrote: >>> Understood and I won't require you or me to complete this investigating >>> before fixing the regression, this is just so we understand where it >>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>> just wrote, do you think you can sprinkle debug prints throughout the >>> kernel to figure out whether enable_irq_wake() somehow messes up the >>> interrupt descriptor of interrupt and test that theory? We can do that >>> offline if you want. >> >> Let me mark v2 as Deferred for now, then. I'm not really sure if that's >> what's intended but we have 3 weeks or so until 5.17 is cut so we can >> afford a few days of investigating. > > I think the "few days of investigating" have now run out :( > How should we proceed? I have not had a chance to provide Peter with the debug patch I wanted him to apply, but your patch seemed better in that regard because if we don't have a Wake-on-LAN interrupt line, we should not mark the device as wake-up capable to begin with. Peter, could you try Jakub's patch and confirm that it works equally well as yours? https://lore.kernel.org/netdev/20220223144818.2f9ce725@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/
Hi, On 2/23/22 16:48, Jakub Kicinski wrote: > On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>> I have no problems working with you to improve the driver, the problem >>> I have is this is currently a regression in 5.17 so I would like to >>> see something land, whether it's reverting the other patch, landing >>> thing one or another straight forward fix and then maybe revisit as >>> whole in 5.18. >> >> Understood and I won't require you or me to complete this investigating >> before fixing the regression, this is just so we understand where it >> stemmed from and possibly fix the IRQ layer if need be. Given what I >> just wrote, do you think you can sprinkle debug prints throughout the >> kernel to figure out whether enable_irq_wake() somehow messes up the >> interrupt descriptor of interrupt and test that theory? We can do that >> offline if you want. > > Let me mark v2 as Deferred for now, then. I'm not really sure if that's > what's intended but we have 3 weeks or so until 5.17 is cut so we can > afford a few days of investigating. > > I'm likely missing the point but sounds like the IRQ subsystem treats > IRQ numbers as unsigned so if we pass a negative value "fun" is sort > of expected. Isn't the problem that device somehow comes with wakeup > capable being set already? Isn't it better to make sure device is not > wake capable if there's no WoL irq instead of adding second check? > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index cfe09117fe6c..7dea44803beb 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev) > > /* Request the WOL interrupt and advertise suspend if available */ > priv->wol_irq_disabled = true; > - if (priv->wol_irq > 0) { > + if (priv->wol_irq > 0) > err = devm_request_irq(&pdev->dev, priv->wol_irq, > bcmgenet_wol_isr, 0, dev->name, priv); > - if (!err) > - device_set_wakeup_capable(&pdev->dev, 1); > - } > + else > + err = -ENOENT; > + device_set_wakeup_capable(&pdev->dev, !err); > > /* Set the needed headroom to account for any possible > * features enabling/disabling at runtime > I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b config that is close as I can achieve using gcc11 vs 12 and the one built with gcc12 fails pretty consistently while the gcc11 works. So, I assumed that applying this patch would fix it, but it doesn't. I may have messed something up, but I'm trying to figure out what is actually different in the two modules between gcc11 and 12.
Hello Jeremy, On 3/3/22 21:00, Jeremy Linton wrote: > Hi, > > On 2/23/22 16:48, Jakub Kicinski wrote: >> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>> I have no problems working with you to improve the driver, the problem >>>> I have is this is currently a regression in 5.17 so I would like to >>>> see something land, whether it's reverting the other patch, landing >>>> thing one or another straight forward fix and then maybe revisit as >>>> whole in 5.18. >>> >>> Understood and I won't require you or me to complete this investigating >>> before fixing the regression, this is just so we understand where it >>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>> just wrote, do you think you can sprinkle debug prints throughout the >>> kernel to figure out whether enable_irq_wake() somehow messes up the >>> interrupt descriptor of interrupt and test that theory? We can do that >>> offline if you want. >> >> Let me mark v2 as Deferred for now, then. I'm not really sure if that's >> what's intended but we have 3 weeks or so until 5.17 is cut so we can >> afford a few days of investigating. >> >> I'm likely missing the point but sounds like the IRQ subsystem treats >> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >> of expected. Isn't the problem that device somehow comes with wakeup >> capable being set already? Isn't it better to make sure device is not >> wake capable if there's no WoL irq instead of adding second check? >> >> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> index cfe09117fe6c..7dea44803beb 100644 >> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev) >> >> /* Request the WOL interrupt and advertise suspend if available */ >> priv->wol_irq_disabled = true; >> - if (priv->wol_irq > 0) { >> + if (priv->wol_irq > 0) >> err = devm_request_irq(&pdev->dev, priv->wol_irq, >> bcmgenet_wol_isr, 0, dev->name, priv); >> - if (!err) >> - device_set_wakeup_capable(&pdev->dev, 1); >> - } >> + else >> + err = -ENOENT; >> + device_set_wakeup_capable(&pdev->dev, !err); >> >> /* Set the needed headroom to account for any possible >> * features enabling/disabling at runtime >> > > > I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b > config that is close as I can achieve using gcc11 vs 12 and the one > built with gcc12 fails pretty consistently while the gcc11 works. > Did Peter's patch instead of this one help ?
Hi, On 3/3/22 14:04, Javier Martinez Canillas wrote: > Hello Jeremy, > > On 3/3/22 21:00, Jeremy Linton wrote: >> Hi, >> >> On 2/23/22 16:48, Jakub Kicinski wrote: >>> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>>> I have no problems working with you to improve the driver, the problem >>>>> I have is this is currently a regression in 5.17 so I would like to >>>>> see something land, whether it's reverting the other patch, landing >>>>> thing one or another straight forward fix and then maybe revisit as >>>>> whole in 5.18. >>>> >>>> Understood and I won't require you or me to complete this investigating >>>> before fixing the regression, this is just so we understand where it >>>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>>> just wrote, do you think you can sprinkle debug prints throughout the >>>> kernel to figure out whether enable_irq_wake() somehow messes up the >>>> interrupt descriptor of interrupt and test that theory? We can do that >>>> offline if you want. >>> >>> Let me mark v2 as Deferred for now, then. I'm not really sure if that's >>> what's intended but we have 3 weeks or so until 5.17 is cut so we can >>> afford a few days of investigating. >>> >>> I'm likely missing the point but sounds like the IRQ subsystem treats >>> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >>> of expected. Isn't the problem that device somehow comes with wakeup >>> capable being set already? Isn't it better to make sure device is not >>> wake capable if there's no WoL irq instead of adding second check? >>> >>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> index cfe09117fe6c..7dea44803beb 100644 >>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct platform_device *pdev) >>> >>> /* Request the WOL interrupt and advertise suspend if available */ >>> priv->wol_irq_disabled = true; >>> - if (priv->wol_irq > 0) { >>> + if (priv->wol_irq > 0) >>> err = devm_request_irq(&pdev->dev, priv->wol_irq, >>> bcmgenet_wol_isr, 0, dev->name, priv); >>> - if (!err) >>> - device_set_wakeup_capable(&pdev->dev, 1); >>> - } >>> + else >>> + err = -ENOENT; >>> + device_set_wakeup_capable(&pdev->dev, !err); >>> >>> /* Set the needed headroom to account for any possible >>> * features enabling/disabling at runtime >>> >> >> >> I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b >> config that is close as I can achieve using gcc11 vs 12 and the one >> built with gcc12 fails pretty consistently while the gcc11 works. >> > > Did Peter's patch instead of this one help ? > No, it seems to be the same problem. The second irq is registered, but never seems to fire. There are a couple odd compiler warnings about infinite recursion in memcpy()/etc I was looking at, but nothing really pops out. Its like the adapter never gets the command submissions (although link/up/down appear to be working/etc).
On 3/4/2022 9:33 AM, Jeremy Linton wrote: > Hi, > > On 3/3/22 14:04, Javier Martinez Canillas wrote: >> Hello Jeremy, >> >> On 3/3/22 21:00, Jeremy Linton wrote: >>> Hi, >>> >>> On 2/23/22 16:48, Jakub Kicinski wrote: >>>> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>>>> I have no problems working with you to improve the driver, the >>>>>> problem >>>>>> I have is this is currently a regression in 5.17 so I would like to >>>>>> see something land, whether it's reverting the other patch, landing >>>>>> thing one or another straight forward fix and then maybe revisit as >>>>>> whole in 5.18. >>>>> >>>>> Understood and I won't require you or me to complete this >>>>> investigating >>>>> before fixing the regression, this is just so we understand where it >>>>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>>>> just wrote, do you think you can sprinkle debug prints throughout the >>>>> kernel to figure out whether enable_irq_wake() somehow messes up the >>>>> interrupt descriptor of interrupt and test that theory? We can do that >>>>> offline if you want. >>>> >>>> Let me mark v2 as Deferred for now, then. I'm not really sure if that's >>>> what's intended but we have 3 weeks or so until 5.17 is cut so we can >>>> afford a few days of investigating. >>>> >>>> I'm likely missing the point but sounds like the IRQ subsystem treats >>>> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >>>> of expected. Isn't the problem that device somehow comes with wakeup >>>> capable being set already? Isn't it better to make sure device is not >>>> wake capable if there's no WoL irq instead of adding second check? >>>> >>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>> index cfe09117fe6c..7dea44803beb 100644 >>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct >>>> platform_device *pdev) >>>> /* Request the WOL interrupt and advertise suspend if >>>> available */ >>>> priv->wol_irq_disabled = true; >>>> - if (priv->wol_irq > 0) { >>>> + if (priv->wol_irq > 0) >>>> err = devm_request_irq(&pdev->dev, priv->wol_irq, >>>> bcmgenet_wol_isr, 0, dev->name, priv); >>>> - if (!err) >>>> - device_set_wakeup_capable(&pdev->dev, 1); >>>> - } >>>> + else >>>> + err = -ENOENT; >>>> + device_set_wakeup_capable(&pdev->dev, !err); >>>> /* Set the needed headroom to account for any possible >>>> * features enabling/disabling at runtime >>>> >>> >>> >>> I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b >>> config that is close as I can achieve using gcc11 vs 12 and the one >>> built with gcc12 fails pretty consistently while the gcc11 works. >>> >> >> Did Peter's patch instead of this one help ? >> > > No, it seems to be the same problem. The second irq is registered, but > never seems to fire. There are a couple odd compiler warnings about > infinite recursion in memcpy()/etc I was looking at, but nothing really > pops out. Its like the adapter never gets the command submissions > (although link/up/down appear to be working/etc). There are two "main" interrupt lines which are required and an optional third interrupt line which is the side band Wake-on-LAN interrupt from the second level interrupt controller that aggregates all wake-up sources. The first interrupt line collects the the default RX/TX queue interrupts (ring 16) as well as the MAC link up/down and other interrupts that we are not using. The second interrupt line is only for the TX queues (rings 0 through 3) transmit done completion signaling. Because the driver is multi-queue aware and enabled, the network stack will chose any of those 5 queues before transmitting packets based upon a hash, so if you want to reliably prove/disprove that the second interrupt line is non-functional, you would need to force a given type of packet(s) to use that queue specifically. There is an example on how to do that here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/multiqueue.rst#n47 With that said, please try the following debug patch so we can get more understanding of how we managed to prevent the second interrupt line from getting its interrupt handler serviced. Thanks
Hi, Sorry about the delay, i'm flipping between a couple different things here. On 3/4/22 14:12, Florian Fainelli wrote: > > > On 3/4/2022 9:33 AM, Jeremy Linton wrote: >> Hi, >> >> On 3/3/22 14:04, Javier Martinez Canillas wrote: >>> Hello Jeremy, >>> >>> On 3/3/22 21:00, Jeremy Linton wrote: >>>> Hi, >>>> >>>> On 2/23/22 16:48, Jakub Kicinski wrote: >>>>> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>>>>> I have no problems working with you to improve the driver, the >>>>>>> problem >>>>>>> I have is this is currently a regression in 5.17 so I would like to >>>>>>> see something land, whether it's reverting the other patch, landing >>>>>>> thing one or another straight forward fix and then maybe revisit as >>>>>>> whole in 5.18. >>>>>> >>>>>> Understood and I won't require you or me to complete this >>>>>> investigating >>>>>> before fixing the regression, this is just so we understand where it >>>>>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>>>>> just wrote, do you think you can sprinkle debug prints throughout the >>>>>> kernel to figure out whether enable_irq_wake() somehow messes up the >>>>>> interrupt descriptor of interrupt and test that theory? We can do >>>>>> that >>>>>> offline if you want. >>>>> >>>>> Let me mark v2 as Deferred for now, then. I'm not really sure if >>>>> that's >>>>> what's intended but we have 3 weeks or so until 5.17 is cut so we can >>>>> afford a few days of investigating. >>>>> >>>>> I'm likely missing the point but sounds like the IRQ subsystem treats >>>>> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >>>>> of expected. Isn't the problem that device somehow comes with wakeup >>>>> capable being set already? Isn't it better to make sure device is not >>>>> wake capable if there's no WoL irq instead of adding second check? >>>>> >>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>> index cfe09117fe6c..7dea44803beb 100644 >>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct >>>>> platform_device *pdev) >>>>> /* Request the WOL interrupt and advertise suspend if >>>>> available */ >>>>> priv->wol_irq_disabled = true; >>>>> - if (priv->wol_irq > 0) { >>>>> + if (priv->wol_irq > 0) >>>>> err = devm_request_irq(&pdev->dev, priv->wol_irq, >>>>> bcmgenet_wol_isr, 0, dev->name, priv); >>>>> - if (!err) >>>>> - device_set_wakeup_capable(&pdev->dev, 1); >>>>> - } >>>>> + else >>>>> + err = -ENOENT; >>>>> + device_set_wakeup_capable(&pdev->dev, !err); >>>>> /* Set the needed headroom to account for any possible >>>>> * features enabling/disabling at runtime >>>>> >>>> >>>> >>>> I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have a/b >>>> config that is close as I can achieve using gcc11 vs 12 and the one >>>> built with gcc12 fails pretty consistently while the gcc11 works. >>>> >>> >>> Did Peter's patch instead of this one help ? >>> >> >> No, it seems to be the same problem. The second irq is registered, but >> never seems to fire. There are a couple odd compiler warnings about >> infinite recursion in memcpy()/etc I was looking at, but nothing >> really pops out. Its like the adapter never gets the command >> submissions (although link/up/down appear to be working/etc). > > There are two "main" interrupt lines which are required and an optional > third interrupt line which is the side band Wake-on-LAN interrupt from > the second level interrupt controller that aggregates all wake-up sources. > > The first interrupt line collects the the default RX/TX queue interrupts > (ring 16) as well as the MAC link up/down and other interrupts that we > are not using. The second interrupt line is only for the TX queues > (rings 0 through 3) transmit done completion signaling. Because the > driver is multi-queue aware and enabled, the network stack will chose > any of those 5 queues before transmitting packets based upon a hash, so > if you want to reliably prove/disprove that the second interrupt line is > non-functional, you would need to force a given type of packet(s) to use > that queue specifically. There is an example on how to do that here: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/multiqueue.rst#n47 > > > With that said, please try the following debug patch so we can get more > understanding of how we managed to prevent the second interrupt line > from getting its interrupt handler serviced. Thanks Right, I applied your patch to rc7, and it prints the following (trimming uninterresting bits) [ 7.044681] bcmgenet BCM6E4E:00: IRQ0: 28 (29), IRQ1: -6 (28), Wol IRQ: 29 (4294967290) [ 7.064731] bcmgenet BCM6E4E:00: GENET 5.0 EPHY: 0x0000 [ 8.533639] bcmgenet BCM6E4E:00 enabcm6e4ei0: renamed from eth0 [ 56.803894] bcmgenet BCM6E4E:00: configuring instance for external RGMII (RX delay) [ 56.896851] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Down [ 60.045071] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Up - 1Gbps/Full - flow control off [ 60.055872] IPv6: ADDRCONF(NETDEV_CHANGE): enabcm6e4ei0: link becomes ready [ 62.283525] ------------[ cut here ]------------ [ 62.290811] NETDEV WATCHDOG: enabcm6e4ei0 (bcmgenet): transmit queue 2 timed out [ 62.301080] WARNING: CPU: 3 PID: 0 at net/sched/sch_generic.c:529 dev_watchdog+0x234/0x240 [ 62.312220] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat ns [ 62.370353] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.17.0-rc7G12+ #58 [ 62.380052] Hardware name: Raspberry Pi Foundation Raspberry Pi 4 Model B/Raspberry Pi 4 Model B, BIOS EDK2-DEV 02/08/2022 [ 62.394151] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 62.404211] pc : dev_watchdog+0x234/0x240 [ 62.411304] lr : dev_watchdog+0x234/0x240 [ 62.418371] sp : ffff8000080b3a40 [ 62.424715] x29: ffff8000080b3a40 x28: ffffdd4ed64c7000 x27: ffff8000080b3b20 [ 62.434899] x26: ffffdd4ed5f40000 x25: 0000000000000000 x24: ffffdd4ed64cec08 [ 62.445095] x23: 0000000000000100 x22: ffffdd4ed64c7000 x21: ffff1bd254e58000 [ 62.455259] x20: 0000000000000002 x19: ffff1bd254e584c8 x18: ffffffffffffffff [ 62.465439] x17: 64656d6974203220 x16: 0000000000000001 x15: 6d736e617274203a [ 62.475615] x14: 2974656e65676d63 x13: ffffdd4ed51700d8 x12: ffffdd4ed65bd5f0 [ 62.485787] x11: 00000000ffffffff x10: ffffdd4ed65bd5f0 x9 : ffffdd4ed420c0fc [ 62.495978] x8 : 00000000ffffdfff x7 : ffffdd4ed65bd5f0 x6 : 0000000000000001 [ 62.506173] x5 : 0000000000000000 x4 : ffff1bd2fb7b1408 x3 : ffff1bd2fb7bddb0 [ 62.516334] x2 : ffff1bd2fb7b1408 x1 : ffff3e842586e000 x0 : 0000000000000044 [ 62.526520] Call trace: [ 62.531969] dev_watchdog+0x234/0x240 [ 62.538671] call_timer_fn+0x3c/0x15c [ 62.545331] __run_timers.part.0+0x288/0x310 [ 62.552579] run_timer_softirq+0x48/0x80 [ 62.559466] __do_softirq+0x128/0x360 [ 62.566055] __irq_exit_rcu+0x138/0x140 [ 62.572823] irq_exit_rcu+0x1c/0x30 [ 62.580799] el1_interrupt+0x38/0x54 [ 62.580817] el1h_64_irq_handler+0x18/0x24 [ 62.580822] el1h_64_irq+0x7c/0x80 [ 62.580827] arch_cpu_idle+0x18/0x2c [ 62.580832] default_idle_call+0x4c/0x140 [ 62.580836] cpuidle_idle_call+0x14c/0x1a0 [ 62.580844] do_idle+0xb0/0x100 [ 62.580849] cpu_startup_entry+0x30/0x8c [ 62.580854] secondary_start_kernel+0xe4/0x110 [ 62.580862] __secondary_switched+0x94/0x98 [ 62.580871] ---[ end trace 0000000000000000 ]--- It should be noted that the irq0/1/2 numbers are a bit messed up in the patch, but the general idea should be visible here. The full log is here https://pastebin.centos.org/view/22c2aede The WOL paths aren't required to trigger this, which is why I questioned the other patch.
On 3/7/22 10:27 AM, Jeremy Linton wrote: > Hi, > > Sorry about the delay, i'm flipping between a couple different things here. > > On 3/4/22 14:12, Florian Fainelli wrote: >> >> >> On 3/4/2022 9:33 AM, Jeremy Linton wrote: >>> Hi, >>> >>> On 3/3/22 14:04, Javier Martinez Canillas wrote: >>>> Hello Jeremy, >>>> >>>> On 3/3/22 21:00, Jeremy Linton wrote: >>>>> Hi, >>>>> >>>>> On 2/23/22 16:48, Jakub Kicinski wrote: >>>>>> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>>>>>> I have no problems working with you to improve the driver, the >>>>>>>> problem >>>>>>>> I have is this is currently a regression in 5.17 so I would like to >>>>>>>> see something land, whether it's reverting the other patch, landing >>>>>>>> thing one or another straight forward fix and then maybe revisit as >>>>>>>> whole in 5.18. >>>>>>> >>>>>>> Understood and I won't require you or me to complete this >>>>>>> investigating >>>>>>> before fixing the regression, this is just so we understand where it >>>>>>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>>>>>> just wrote, do you think you can sprinkle debug prints throughout >>>>>>> the >>>>>>> kernel to figure out whether enable_irq_wake() somehow messes up the >>>>>>> interrupt descriptor of interrupt and test that theory? We can do >>>>>>> that >>>>>>> offline if you want. >>>>>> >>>>>> Let me mark v2 as Deferred for now, then. I'm not really sure if >>>>>> that's >>>>>> what's intended but we have 3 weeks or so until 5.17 is cut so we can >>>>>> afford a few days of investigating. >>>>>> >>>>>> I'm likely missing the point but sounds like the IRQ subsystem treats >>>>>> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >>>>>> of expected. Isn't the problem that device somehow comes with wakeup >>>>>> capable being set already? Isn't it better to make sure device is not >>>>>> wake capable if there's no WoL irq instead of adding second check? >>>>>> >>>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>> index cfe09117fe6c..7dea44803beb 100644 >>>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct >>>>>> platform_device *pdev) >>>>>> /* Request the WOL interrupt and advertise suspend if >>>>>> available */ >>>>>> priv->wol_irq_disabled = true; >>>>>> - if (priv->wol_irq > 0) { >>>>>> + if (priv->wol_irq > 0) >>>>>> err = devm_request_irq(&pdev->dev, priv->wol_irq, >>>>>> bcmgenet_wol_isr, 0, dev->name, priv); >>>>>> - if (!err) >>>>>> - device_set_wakeup_capable(&pdev->dev, 1); >>>>>> - } >>>>>> + else >>>>>> + err = -ENOENT; >>>>>> + device_set_wakeup_capable(&pdev->dev, !err); >>>>>> /* Set the needed headroom to account for any possible >>>>>> * features enabling/disabling at runtime >>>>>> >>>>> >>>>> >>>>> I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have >>>>> a/b >>>>> config that is close as I can achieve using gcc11 vs 12 and the one >>>>> built with gcc12 fails pretty consistently while the gcc11 works. >>>>> >>>> >>>> Did Peter's patch instead of this one help ? >>>> >>> >>> No, it seems to be the same problem. The second irq is registered, >>> but never seems to fire. There are a couple odd compiler warnings >>> about infinite recursion in memcpy()/etc I was looking at, but >>> nothing really pops out. Its like the adapter never gets the command >>> submissions (although link/up/down appear to be working/etc). >> >> There are two "main" interrupt lines which are required and an >> optional third interrupt line which is the side band Wake-on-LAN >> interrupt from the second level interrupt controller that aggregates >> all wake-up sources. >> >> The first interrupt line collects the the default RX/TX queue >> interrupts (ring 16) as well as the MAC link up/down and other >> interrupts that we are not using. The second interrupt line is only >> for the TX queues (rings 0 through 3) transmit done completion >> signaling. Because the driver is multi-queue aware and enabled, the >> network stack will chose any of those 5 queues before transmitting >> packets based upon a hash, so if you want to reliably prove/disprove >> that the second interrupt line is non-functional, you would need to >> force a given type of packet(s) to use that queue specifically. There >> is an example on how to do that here: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/multiqueue.rst#n47 >> >> >> With that said, please try the following debug patch so we can get >> more understanding of how we managed to prevent the second interrupt >> line from getting its interrupt handler serviced. Thanks > > Right, I applied your patch to rc7, and it prints the following > (trimming uninterresting bits) > > > > [ 7.044681] bcmgenet BCM6E4E:00: IRQ0: 28 (29), IRQ1: -6 (28), Wol > IRQ: 29 (4294967290) OK, my debug patch was a bit messed up in that it should have been: + dev_info(&pdev->dev, "IRQ0: %d (%u), IRQ1: %d (%u), Wol IRQ: %d (%u)\n", + priv->irq0, priv->irq0, priv->irq1, + priv->irq1, priv->wol_irq, priv->wol_irq); still, we have the information we want, that is, both IRQ0 and IRQ1 are valid with the values 28, 49, however wol_irq is -ENXIO as expected. So I really do not think that the Wake-on-LAN interrupt has anything to do with getting the transmit queue timeout. I have seen reports that it look like switching the checksum offload might be responsible for these timeouts: https://github.com/raspberrypi/linux/issues/3850 https://github.com/raspberrypi/linux/issues/3850#issuecomment-698206124 If that is the case, would you try the following patch in addition to the previous one: diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 1ff0e9a0998e..5ee92b7f70e4 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -4034,8 +4034,7 @@ static int bcmgenet_probe(struct platform_device *pdev) priv->msg_enable = netif_msg_init(-1, GENET_MSG_DEFAULT); /* Set default features */ - dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM | - NETIF_F_RXCSUM; + dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_RXCSUM; dev->hw_features |= dev->features; dev->vlan_features |= dev->features; > [ 7.064731] bcmgenet BCM6E4E:00: GENET 5.0 EPHY: 0x0000 > [ 8.533639] bcmgenet BCM6E4E:00 enabcm6e4ei0: renamed from eth0 > [ 56.803894] bcmgenet BCM6E4E:00: configuring instance for external > RGMII (RX delay) > [ 56.896851] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Down > [ 60.045071] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Up - 1Gbps/Full > - flow control off > [ 60.055872] IPv6: ADDRCONF(NETDEV_CHANGE): enabcm6e4ei0: link becomes > ready > [ 62.283525] ------------[ cut here ]------------ > [ 62.290811] NETDEV WATCHDOG: enabcm6e4ei0 (bcmgenet): transmit queue > 2 timed out > [ 62.301080] WARNING: CPU: 3 PID: 0 at net/sched/sch_generic.c:529 > dev_watchdog+0x234/0x240 > [ 62.312220] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 > nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct > nft_chain_nat nf_nat ns > [ 62.370353] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.17.0-rc7G12+ #58 > [ 62.380052] Hardware name: Raspberry Pi Foundation Raspberry Pi 4 > Model B/Raspberry Pi 4 Model B, BIOS EDK2-DEV 02/08/2022 > [ 62.394151] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS > BTYPE=--) > [ 62.404211] pc : dev_watchdog+0x234/0x240 > [ 62.411304] lr : dev_watchdog+0x234/0x240 > [ 62.418371] sp : ffff8000080b3a40 > [ 62.424715] x29: ffff8000080b3a40 x28: ffffdd4ed64c7000 x27: > ffff8000080b3b20 > [ 62.434899] x26: ffffdd4ed5f40000 x25: 0000000000000000 x24: > ffffdd4ed64cec08 > [ 62.445095] x23: 0000000000000100 x22: ffffdd4ed64c7000 x21: > ffff1bd254e58000 > [ 62.455259] x20: 0000000000000002 x19: ffff1bd254e584c8 x18: > ffffffffffffffff > [ 62.465439] x17: 64656d6974203220 x16: 0000000000000001 x15: > 6d736e617274203a > [ 62.475615] x14: 2974656e65676d63 x13: ffffdd4ed51700d8 x12: > ffffdd4ed65bd5f0 > [ 62.485787] x11: 00000000ffffffff x10: ffffdd4ed65bd5f0 x9 : > ffffdd4ed420c0fc > [ 62.495978] x8 : 00000000ffffdfff x7 : ffffdd4ed65bd5f0 x6 : > 0000000000000001 > [ 62.506173] x5 : 0000000000000000 x4 : ffff1bd2fb7b1408 x3 : > ffff1bd2fb7bddb0 > [ 62.516334] x2 : ffff1bd2fb7b1408 x1 : ffff3e842586e000 x0 : > 0000000000000044 > [ 62.526520] Call trace: > [ 62.531969] dev_watchdog+0x234/0x240 > [ 62.538671] call_timer_fn+0x3c/0x15c > [ 62.545331] __run_timers.part.0+0x288/0x310 > [ 62.552579] run_timer_softirq+0x48/0x80 > [ 62.559466] __do_softirq+0x128/0x360 > [ 62.566055] __irq_exit_rcu+0x138/0x140 > [ 62.572823] irq_exit_rcu+0x1c/0x30 > [ 62.580799] el1_interrupt+0x38/0x54 > [ 62.580817] el1h_64_irq_handler+0x18/0x24 > [ 62.580822] el1h_64_irq+0x7c/0x80 > [ 62.580827] arch_cpu_idle+0x18/0x2c > [ 62.580832] default_idle_call+0x4c/0x140 > [ 62.580836] cpuidle_idle_call+0x14c/0x1a0 > [ 62.580844] do_idle+0xb0/0x100 > [ 62.580849] cpu_startup_entry+0x30/0x8c > [ 62.580854] secondary_start_kernel+0xe4/0x110 > [ 62.580862] __secondary_switched+0x94/0x98 > [ 62.580871] ---[ end trace 0000000000000000 ]--- > > It should be noted that the irq0/1/2 numbers are a bit messed up in the > patch, but the general idea should be visible here. > > The full log is here https://pastebin.centos.org/view/22c2aede > > The WOL paths aren't required to trigger this, which is why I questioned > the other patch. >
Hi, On 3/7/22 12:44, Florian Fainelli wrote: > On 3/7/22 10:27 AM, Jeremy Linton wrote: >> Hi, >> >> Sorry about the delay, i'm flipping between a couple different things here. >> >> On 3/4/22 14:12, Florian Fainelli wrote: >>> >>> >>> On 3/4/2022 9:33 AM, Jeremy Linton wrote: >>>> Hi, >>>> >>>> On 3/3/22 14:04, Javier Martinez Canillas wrote: >>>>> Hello Jeremy, >>>>> >>>>> On 3/3/22 21:00, Jeremy Linton wrote: >>>>>> Hi, >>>>>> >>>>>> On 2/23/22 16:48, Jakub Kicinski wrote: >>>>>>> On Wed, 23 Feb 2022 09:54:26 -0800 Florian Fainelli wrote: >>>>>>>>> I have no problems working with you to improve the driver, the >>>>>>>>> problem >>>>>>>>> I have is this is currently a regression in 5.17 so I would like to >>>>>>>>> see something land, whether it's reverting the other patch, landing >>>>>>>>> thing one or another straight forward fix and then maybe revisit as >>>>>>>>> whole in 5.18. >>>>>>>> >>>>>>>> Understood and I won't require you or me to complete this >>>>>>>> investigating >>>>>>>> before fixing the regression, this is just so we understand where it >>>>>>>> stemmed from and possibly fix the IRQ layer if need be. Given what I >>>>>>>> just wrote, do you think you can sprinkle debug prints throughout >>>>>>>> the >>>>>>>> kernel to figure out whether enable_irq_wake() somehow messes up the >>>>>>>> interrupt descriptor of interrupt and test that theory? We can do >>>>>>>> that >>>>>>>> offline if you want. >>>>>>> >>>>>>> Let me mark v2 as Deferred for now, then. I'm not really sure if >>>>>>> that's >>>>>>> what's intended but we have 3 weeks or so until 5.17 is cut so we can >>>>>>> afford a few days of investigating. >>>>>>> >>>>>>> I'm likely missing the point but sounds like the IRQ subsystem treats >>>>>>> IRQ numbers as unsigned so if we pass a negative value "fun" is sort >>>>>>> of expected. Isn't the problem that device somehow comes with wakeup >>>>>>> capable being set already? Isn't it better to make sure device is not >>>>>>> wake capable if there's no WoL irq instead of adding second check? >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>>> b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>>> index cfe09117fe6c..7dea44803beb 100644 >>>>>>> --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>>> +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c >>>>>>> @@ -4020,12 +4020,12 @@ static int bcmgenet_probe(struct >>>>>>> platform_device *pdev) >>>>>>> /* Request the WOL interrupt and advertise suspend if >>>>>>> available */ >>>>>>> priv->wol_irq_disabled = true; >>>>>>> - if (priv->wol_irq > 0) { >>>>>>> + if (priv->wol_irq > 0) >>>>>>> err = devm_request_irq(&pdev->dev, priv->wol_irq, >>>>>>> bcmgenet_wol_isr, 0, dev->name, priv); >>>>>>> - if (!err) >>>>>>> - device_set_wakeup_capable(&pdev->dev, 1); >>>>>>> - } >>>>>>> + else >>>>>>> + err = -ENOENT; >>>>>>> + device_set_wakeup_capable(&pdev->dev, !err); >>>>>>> /* Set the needed headroom to account for any possible >>>>>>> * features enabling/disabling at runtime >>>>>>> >>>>>> >>>>>> >>>>>> I duplicated the problem on rpi4/ACPI by moving to gcc12, so I have >>>>>> a/b >>>>>> config that is close as I can achieve using gcc11 vs 12 and the one >>>>>> built with gcc12 fails pretty consistently while the gcc11 works. >>>>>> >>>>> >>>>> Did Peter's patch instead of this one help ? >>>>> >>>> >>>> No, it seems to be the same problem. The second irq is registered, >>>> but never seems to fire. There are a couple odd compiler warnings >>>> about infinite recursion in memcpy()/etc I was looking at, but >>>> nothing really pops out. Its like the adapter never gets the command >>>> submissions (although link/up/down appear to be working/etc). >>> >>> There are two "main" interrupt lines which are required and an >>> optional third interrupt line which is the side band Wake-on-LAN >>> interrupt from the second level interrupt controller that aggregates >>> all wake-up sources. >>> >>> The first interrupt line collects the the default RX/TX queue >>> interrupts (ring 16) as well as the MAC link up/down and other >>> interrupts that we are not using. The second interrupt line is only >>> for the TX queues (rings 0 through 3) transmit done completion >>> signaling. Because the driver is multi-queue aware and enabled, the >>> network stack will chose any of those 5 queues before transmitting >>> packets based upon a hash, so if you want to reliably prove/disprove >>> that the second interrupt line is non-functional, you would need to >>> force a given type of packet(s) to use that queue specifically. There >>> is an example on how to do that here: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/networking/multiqueue.rst#n47 >>> >>> >>> With that said, please try the following debug patch so we can get >>> more understanding of how we managed to prevent the second interrupt >>> line from getting its interrupt handler serviced. Thanks >> >> Right, I applied your patch to rc7, and it prints the following >> (trimming uninterresting bits) >> >> >> >> [ 7.044681] bcmgenet BCM6E4E:00: IRQ0: 28 (29), IRQ1: -6 (28), Wol >> IRQ: 29 (4294967290) > > OK, my debug patch was a bit messed up in that it should have been: > > + dev_info(&pdev->dev, "IRQ0: %d (%u), IRQ1: %d (%u), Wol IRQ: %d (%u)\n", > + priv->irq0, priv->irq0, priv->irq1, > + priv->irq1, priv->wol_irq, priv->wol_irq); > > still, we have the information we want, that is, both IRQ0 and IRQ1 are > valid with the values 28, 49, however wol_irq is -ENXIO as expected. > > So I really do not think that the Wake-on-LAN interrupt has anything to > do with getting the transmit queue timeout. I have seen reports that it > look like switching the checksum offload might be responsible for these > timeouts: > > https://github.com/raspberrypi/linux/issues/3850 > https://github.com/raspberrypi/linux/issues/3850#issuecomment-698206124 > > If that is the case, would you try the following patch in addition to > the previous one: > > diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > index 1ff0e9a0998e..5ee92b7f70e4 100644 > --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c > +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c > @@ -4034,8 +4034,7 @@ static int bcmgenet_probe(struct platform_device > *pdev) > priv->msg_enable = netif_msg_init(-1, GENET_MSG_DEFAULT); > > /* Set default features */ > - dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_HW_CSUM | > - NETIF_F_RXCSUM; > + dev->features |= NETIF_F_SG | NETIF_F_HIGHDMA | NETIF_F_RXCSUM; > dev->hw_features |= dev->features; > dev->vlan_features |= dev->features; Well I "fixed" it, by fully serializing the register writes with writel/readl instead of write/readl_relaxed. So, I'm looking for cases where the descriptor read/write in memory, isn't assured to be in order with accessing the device. > > > > > >> [ 7.064731] bcmgenet BCM6E4E:00: GENET 5.0 EPHY: 0x0000 >> [ 8.533639] bcmgenet BCM6E4E:00 enabcm6e4ei0: renamed from eth0 >> [ 56.803894] bcmgenet BCM6E4E:00: configuring instance for external >> RGMII (RX delay) >> [ 56.896851] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Down >> [ 60.045071] bcmgenet BCM6E4E:00 enabcm6e4ei0: Link is Up - 1Gbps/Full >> - flow control off >> [ 60.055872] IPv6: ADDRCONF(NETDEV_CHANGE): enabcm6e4ei0: link becomes >> ready >> [ 62.283525] ------------[ cut here ]------------ >> [ 62.290811] NETDEV WATCHDOG: enabcm6e4ei0 (bcmgenet): transmit queue >> 2 timed out >> [ 62.301080] WARNING: CPU: 3 PID: 0 at net/sched/sch_generic.c:529 >> dev_watchdog+0x234/0x240 >> [ 62.312220] Modules linked in: nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 >> nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct >> nft_chain_nat nf_nat ns >> [ 62.370353] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.17.0-rc7G12+ #58 >> [ 62.380052] Hardware name: Raspberry Pi Foundation Raspberry Pi 4 >> Model B/Raspberry Pi 4 Model B, BIOS EDK2-DEV 02/08/2022 >> [ 62.394151] pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS >> BTYPE=--) >> [ 62.404211] pc : dev_watchdog+0x234/0x240 >> [ 62.411304] lr : dev_watchdog+0x234/0x240 >> [ 62.418371] sp : ffff8000080b3a40 >> [ 62.424715] x29: ffff8000080b3a40 x28: ffffdd4ed64c7000 x27: >> ffff8000080b3b20 >> [ 62.434899] x26: ffffdd4ed5f40000 x25: 0000000000000000 x24: >> ffffdd4ed64cec08 >> [ 62.445095] x23: 0000000000000100 x22: ffffdd4ed64c7000 x21: >> ffff1bd254e58000 >> [ 62.455259] x20: 0000000000000002 x19: ffff1bd254e584c8 x18: >> ffffffffffffffff >> [ 62.465439] x17: 64656d6974203220 x16: 0000000000000001 x15: >> 6d736e617274203a >> [ 62.475615] x14: 2974656e65676d63 x13: ffffdd4ed51700d8 x12: >> ffffdd4ed65bd5f0 >> [ 62.485787] x11: 00000000ffffffff x10: ffffdd4ed65bd5f0 x9 : >> ffffdd4ed420c0fc >> [ 62.495978] x8 : 00000000ffffdfff x7 : ffffdd4ed65bd5f0 x6 : >> 0000000000000001 >> [ 62.506173] x5 : 0000000000000000 x4 : ffff1bd2fb7b1408 x3 : >> ffff1bd2fb7bddb0 >> [ 62.516334] x2 : ffff1bd2fb7b1408 x1 : ffff3e842586e000 x0 : >> 0000000000000044 >> [ 62.526520] Call trace: >> [ 62.531969] dev_watchdog+0x234/0x240 >> [ 62.538671] call_timer_fn+0x3c/0x15c >> [ 62.545331] __run_timers.part.0+0x288/0x310 >> [ 62.552579] run_timer_softirq+0x48/0x80 >> [ 62.559466] __do_softirq+0x128/0x360 >> [ 62.566055] __irq_exit_rcu+0x138/0x140 >> [ 62.572823] irq_exit_rcu+0x1c/0x30 >> [ 62.580799] el1_interrupt+0x38/0x54 >> [ 62.580817] el1h_64_irq_handler+0x18/0x24 >> [ 62.580822] el1h_64_irq+0x7c/0x80 >> [ 62.580827] arch_cpu_idle+0x18/0x2c >> [ 62.580832] default_idle_call+0x4c/0x140 >> [ 62.580836] cpuidle_idle_call+0x14c/0x1a0 >> [ 62.580844] do_idle+0xb0/0x100 >> [ 62.580849] cpu_startup_entry+0x30/0x8c >> [ 62.580854] secondary_start_kernel+0xe4/0x110 >> [ 62.580862] __secondary_switched+0x94/0x98 >> [ 62.580871] ---[ end trace 0000000000000000 ]--- >> >> It should be noted that the irq0/1/2 numbers are a bit messed up in the >> patch, but the general idea should be visible here. >> >> The full log is here https://pastebin.centos.org/view/22c2aede >> >> The WOL paths aren't required to trigger this, which is why I questioned >> the other patch. >> > >
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c index e31a5a397f11..325f01b916c8 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c @@ -60,6 +60,10 @@ int bcmgenet_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol) if (!device_can_wakeup(kdev)) return -ENOTSUPP; + /* We need a WoL IRQ to enable support, not all HW has one setup */ + if (priv->wol_irq <= 0) + return -ENOTSUPP; + if (wol->wolopts & ~(WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER)) return -EINVAL;
The ethtool WoL enable function wasn't checking if the device has the optional WoL IRQ and hence on platforms such as the Raspberry Pi 4 which had working ethernet prior to the last fix regressed with the last fix, so also check if we have a WoL IRQ there and return ENOTSUPP if not. Fixes: 9deb48b53e7f ("bcmgenet: add WOL IRQ check") Fixes: 8562056f267d ("net: bcmgenet: request Wake-on-LAN interrupt") Signed-off-by: Peter Robinson <pbrobinson@gmail.com> Suggested-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 4 ++++ 1 file changed, 4 insertions(+) We're seeing this crash on the Raspberry Pi 4 series of devices on Fedora on 5.17-rc with the top Fixes patch and wired ethernet doesn't work. [173353.733114] bcmgenet fd580000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx [173353.741855] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready dm[173356.452622] ------------[ cut here ]------------ [173356.457442] NETDEV WATCHDOG: eth0 (bcmgenet): transmit queue 4 timed out [173356.464418] WARNING: CPU: 3 PID: 0 at net/sched/sch_generic.c:529 dev_watchdog+0x234/0x240 [173356.472915] Modules linked in: tls nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_set nf_tables nfnetlink rtl8xxxu rtl8192cu rtl_usb rtl8192c_common rtlwifi btsdio mac80211 bluetooth cpufreq_dt libarc4 ecdh_generic brcmfmac brcmutil cfg80211 raspberrypi_cpufreq rfkill broadcom snd_soc_hdmi_codec bcm_phy_lib bcm2711_thermal genet iproc_rng200 mdio_bcm_unimac leds_gpio nvmem_rmem vfat fat fuse zram mmc_block vc4 snd_soc_core crct10dif_ce snd_compress gpio_raspberrypi_exp raspberrypi_hwmon dwc2 clk_bcm2711_dvp ac97_bus snd_pcm_dmaengine bcm2835_wdt snd_pcm udc_core snd_timer bcm2835_dma sdhci_iproc snd sdhci_pltfm pcie_brcmstb sdhci phy_generic soundcore drm_cma_helper i2c_dev aes_neon_bs [173356.546454] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 5.17.0-0.rc4.96.pbr1.fc36.aarch64 #1 [173356.554931] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2022.04-rc1 04/01/2022 [173356.563843] pstate: 80400005 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [173356.570997] pc : dev_watchdog+0x234/0x240 [173356.575157] lr : dev_watchdog+0x234/0x240 [173356.579314] sp : ffff8000080b3a40 [173356.582758] x29: ffff8000080b3a40 x28: ffffc6e8711b7000 x27: ffff8000080b3b20 [173356.590096] x26: ffffc6e870c30000 x25: 0000000000000000 x24: ffffc6e8711bec08 [173356.597432] x23: 0000000000000100 x22: ffffc6e8711b7000 x21: ffff38a4c2250000 [173356.604767] x20: 0000000000000004 x19: ffff38a4c22504c8 x18: ffffffffffffffff [173356.612102] x17: ffff71bd0ab21000 x16: ffff80000801c000 x15: 0000000000000006 [173356.619436] x14: 0000000000000000 x13: 205d323434373534 x12: ffffc6e8712ad5f0 [173356.626769] x11: 712074696d736e61 x10: ffffc6e8712ad5f0 x9 : ffffc6e86edfc78c [173356.634104] x8 : 00000000ffffdfff x7 : 000000000000003f x6 : 0000000000000000 [173356.641438] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000001000 [173356.648773] x2 : 0000000000001000 x1 : 0000000000000005 x0 : 000000000000003c [173356.656107] Call trace: [173356.658685] dev_watchdog+0x234/0x240 [173356.662493] call_timer_fn+0x3c/0x15c [173356.666304] __run_timers.part.0+0x288/0x310 [173356.670728] run_timer_softirq+0x48/0x80 [173356.674799] __do_softirq+0x128/0x360 e[173356.678601] __irq_exit_rcu+0x138/0x140 [173356.682661] irq_exit_rcu+0x1c/0x30 [173356.686291] el1_interrupt+0x38/0x54 [173356.690007] el1h_64_irq_handler+0x18/0x24 [173356.694251] el1h_64_irq+0x7c/0x80 [173356.697787] arch_cpu_idle+0x18/0x2c [173356.701502] default_idle_call+0x4c/0x140 [173356.705657] cpuidle_idle_call+0x14c/0x1a0 [173356.709905] do_idle+0xb0/0x100 [173356.713182] cpu_startup_entry+0x34/0x8c [173356.717252] secondary_start_kernel+0xe4/0x110 [173356.721852] __secondary_switched+0x94/0x98 [173356.726188] ---[ end trace 0000000000000000 ]---