[PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

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

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

> 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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jakub Kicinski]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jakub Kicinski]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jakub Kicinski]

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?

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jeremy Linton]

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.

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Javier Martinez Canillas]

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 ?

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jeremy Linton]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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



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

[-- Attachment #2: debug.diff --]
[-- Type: text/plain, Size: 3982 bytes --]

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index cfe09117fe6c..7c7ab2cb0ebf 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -3991,6 +3991,10 @@ static int bcmgenet_probe(struct platform_device *pdev)
 	}
 	priv->wol_irq = platform_get_irq_optional(pdev, 2);
 
+	dev_info(&pdev->dev, "IRQ0: %d (%u), IRQ1: %d (%u), Wol IRQ: %d (%u)\n",
+		 priv->irq0, priv->irq1, priv->wol_irq,
+		 priv->irq0, priv->irq1, priv->wol_irq);
+
 	priv->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(priv->base)) {
 		err = PTR_ERR(priv->base);
@@ -4021,10 +4025,13 @@ 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) {
+		dev_info(&pdev->dev, "Wol IRQ > 0 requesting WoL ISR\n");
 		err = devm_request_irq(&pdev->dev, priv->wol_irq,
 				       bcmgenet_wol_isr, 0, dev->name, priv);
-		if (!err)
+		if (!err) {
+			dev_info(&pdev->dev, "Marking device as wake-up capable\n");
 			device_set_wakeup_capable(&pdev->dev, 1);
+		}
 	}
 
 	/* Set the needed headroom to account for any possible
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
index e31a5a397f11..26efa4d175a7 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
@@ -57,11 +57,15 @@ int bcmgenet_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	struct bcmgenet_priv *priv = netdev_priv(dev);
 	struct device *kdev = &priv->pdev->dev;
 
-	if (!device_can_wakeup(kdev))
+	if (!device_can_wakeup(kdev)) {
+		dev_err(kdev, "Device cannot wake-up\n");
 		return -ENOTSUPP;
+	}
 
-	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER))
+	if (wol->wolopts & ~(WAKE_MAGIC | WAKE_MAGICSECURE | WAKE_FILTER)) {
+		dev_err(kdev, "Invalid wolopts\n");
 		return -EINVAL;
+	}
 
 	if (wol->wolopts & WAKE_MAGICSECURE)
 		memcpy(priv->sopass, wol->sopass, sizeof(priv->sopass));
@@ -69,6 +73,7 @@ int bcmgenet_set_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
 	/* Flag the device and relevant IRQ as wakeup capable */
 	if (wol->wolopts) {
 		device_set_wakeup_enable(kdev, 1);
+		dev_info(kdev, "Enabling device for wake-up\n");
 		/* Avoid unbalanced enable_irq_wake calls */
 		if (priv->wol_irq_disabled)
 			enable_irq_wake(priv->wol_irq);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f23ffd30385b..d1a436f29a3a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -841,11 +841,15 @@ static int set_irq_wake_real(unsigned int irq, unsigned int on)
 	struct irq_desc *desc = irq_to_desc(irq);
 	int ret = -ENXIO;
 
-	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE)
+	if (irq_desc_get_chip(desc)->flags &  IRQCHIP_SKIP_SET_WAKE) {
+		pr_info("%s: IRQ chip is marked with IRQCHIP_SKIP_SET_WAKE\n", __func__);
 		return 0;
+	}
 
-	if (desc->irq_data.chip->irq_set_wake)
+	if (desc->irq_data.chip->irq_set_wake) {
 		ret = desc->irq_data.chip->irq_set_wake(&desc->irq_data, on);
+		pr_info("%s: irq_set_wake returned: %d for IRQ: %d\n", __func__, irq);
+	}
 
 	return ret;
 }
@@ -875,8 +879,12 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
 	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
 	int ret = 0;
 
-	if (!desc)
+	pr_info("%s: called with IRQ: %d on: %d\n", __func__, irq, on);
+
+	if (!desc) {
+		pr_err("%s: invalid descriptor\n", __func__);
 		return -EINVAL;
+	}
 
 	/* Don't use NMIs as wake up interrupts please */
 	if (desc->istate & IRQS_NMI) {
@@ -909,6 +917,7 @@ int irq_set_irq_wake(unsigned int irq, unsigned int on)
 
 out_unlock:
 	irq_put_desc_busunlock(desc, flags);
+	pr_info("%s: returning %d for IRQ: %d\n", __func__, ret, irq);
 	return ret;
 }
 EXPORT_SYMBOL(irq_set_irq_wake);

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jeremy Linton]

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.

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Florian Fainelli]

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


-- 
Florian

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jeremy Linton]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

> 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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jeremy Linton]

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.

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Peter Robinson]

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

Re: [PATCH] net: bcmgenet: Return not supported if we don't have a WoL IRQ

[Jakub Kicinski]

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.