[PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

[PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

[Florian Fainelli]

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

From: Florian Fainelli <florian.fainelli@broadcom.com>

In order to have our interrupt descriptor fully setup, and in particular
the action, ensure that we register a full fledged interrupt handler.
This is in particular necessary for the kernel to properly manage early
wake-up scenarios and arm the wake-up interrupt, otherwise there would
be risks of missing the interrupt and leaving it in a state where it
would not be handled correctly at all, including for wake-up purposes.

Fixes: 8baddaa9d4ba ("net: phy: broadcom: Add support for Wake-on-LAN")
Suggested-by: Doug Berger <doug.berger@broadcom.com>
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/phy/bcm-phy-lib.c | 6 ++++++
 drivers/net/phy/bcm-phy-lib.h | 2 ++
 drivers/net/phy/broadcom.c    | 9 ++++++++-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 27c57f6ab211..5603d0a9ce96 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -1028,6 +1028,12 @@ void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_get_wol);
 
+irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_wol_isr);
+
 MODULE_DESCRIPTION("Broadcom PHY Library");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index c6fed43ec913..2f30ce0cab0e 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -8,6 +8,7 @@
 
 #include <linux/brcmphy.h>
 #include <linux/phy.h>
+#include <linux/interrupt.h>
 
 struct ethtool_wolinfo;
 
@@ -115,5 +116,6 @@ static inline void bcm_ptp_stop(struct bcm_ptp_private *priv)
 
 int bcm_phy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol);
 void bcm_phy_get_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol);
+irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
 
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 418e6bc0e998..822c8b01dc53 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -927,7 +927,14 @@ static int bcm54xx_phy_probe(struct phy_device *phydev)
 
 	if (!IS_ERR(wakeup_gpio)) {
 		priv->wake_irq = gpiod_to_irq(wakeup_gpio);
-		ret = irq_set_irq_type(priv->wake_irq, IRQ_TYPE_LEVEL_LOW);
+
+		/* Dummy interrupt handler which is not enabled but is provided
+		 * in order for the interrupt descriptor to be fully set-up.
+		 */
+		ret = devm_request_irq(&phydev->mdio.dev, priv->wake_irq,
+				       bcm_phy_wol_isr,
+				       IRQF_TRIGGER_LOW | IRQF_NO_AUTOEN,
+				       dev_name(&phydev->mdio.dev), phydev);
 		if (ret)
 			return ret;
 	}
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

[Simon Horman]

On Tue, May 16, 2023 at 03:56:39PM -0700, Florian Fainelli wrote:
> From: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> In order to have our interrupt descriptor fully setup, and in particular
> the action, ensure that we register a full fledged interrupt handler.
> This is in particular necessary for the kernel to properly manage early
> wake-up scenarios and arm the wake-up interrupt, otherwise there would
> be risks of missing the interrupt and leaving it in a state where it
> would not be handled correctly at all, including for wake-up purposes.
> 
> Fixes: 8baddaa9d4ba ("net: phy: broadcom: Add support for Wake-on-LAN")
> Suggested-by: Doug Berger <doug.berger@broadcom.com>
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

[Andrew Lunn]

On Tue, May 16, 2023 at 03:56:39PM -0700, Florian Fainelli wrote:
> From: Florian Fainelli <florian.fainelli@broadcom.com>
> 
> In order to have our interrupt descriptor fully setup, and in particular
> the action, ensure that we register a full fledged interrupt handler.
> This is in particular necessary for the kernel to properly manage early
> wake-up scenarios and arm the wake-up interrupt, otherwise there would
> be risks of missing the interrupt and leaving it in a state where it
> would not be handled correctly at all, including for wake-up purposes.

Hi Florian

I've not seen any other driver do this. Maybe that is just my
ignorance.

Please could you Cc: the interrupt and power management
Maintainers. This seems like a generic problem, and should have a
generic solution.

In the setup which needs this, does the output from the PHY go to a
PMIC, not a SoC interrupt? And i assume the PMIC is not interrupt
capable?

	Andrew

Re: [PATCH net-next] net: phy: broadcom: Register dummy IRQ handler

[Florian Fainelli]

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

+tglx, Rafael,

On 5/17/23 05:32, Andrew Lunn wrote:
> On Tue, May 16, 2023 at 03:56:39PM -0700, Florian Fainelli wrote:
>> From: Florian Fainelli <florian.fainelli@broadcom.com>
>>
>> In order to have our interrupt descriptor fully setup, and in particular
>> the action, ensure that we register a full fledged interrupt handler.
>> This is in particular necessary for the kernel to properly manage early
>> wake-up scenarios and arm the wake-up interrupt, otherwise there would
>> be risks of missing the interrupt and leaving it in a state where it
>> would not be handled correctly at all, including for wake-up purposes.
> 
> Hi Florian
> 
> I've not seen any other driver do this. Maybe that is just my
> ignorance.

As a matter of fact I think this is how most, if not all drivers do it, 
they always have an interrupt service routine registered with the 
interrupt on which {disable,enable}_irq_wake() is called.

If you remember in my case we do not want to actually service the 
interrupt because as soon as we configure the PHY with a wake-up 
pattern, the PHY will assert the interrupt line, and if we configure an 
unicast/broadcast/multicast pattern we will be interrupted for every 
packet received in the network.

If we do not acknowledge the pattern match by doing a clear on read of 
the interrupt status register in the PHY, then obviously no new pattern 
matched events are generated. The interrupt is level low driven FWIW.

This was the reason why I went with this approach.

> 
> Please could you Cc: the interrupt and power management
> Maintainers. This seems like a generic problem, and should have a
> generic solution.

While I was working on this patch set initially, I had missed a call to 
irq_set_irq_type(.., IRQ_TYPE_LEVEL_LOW) and the interrupt was left in 
its default configuration of being falling edge triggered. The hardware 
interrupt generated by the PHY is level low driven. Since we are not 
using the interrupt for anything, it did not really matter that the flow 
handling would have been incorrect and it worked for the most part.

Except in one particular case which was when I entered an ACPI S5 / 
poweroff state, then woke-up the system using the Ethernet PHY, cold 
booted the kernel. The GPIO driver would have probed and acknowledged 
the interrupt because we want to report any GPIO-based wake-up from ACPI S5:

https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpio/gpio-brcmstb.c#L707

In that cold boot case the PHY driver would probe and acknowledge do the 
necessary clear on read and also charge the device for wake-up.

Later any attempt to wake-up from the PHY would not work however. This 
came down to the fact that in kernel/irq/pm.c::suspend_device_irq we had 
no action associated with the interrupt and therefore we would not be 
ensuring that the interrupt was marked as wake-up active within the 
interrupt controller provider driver (GPIO).

Maybe there is an opportunity for a patch here to issue a WARN_ON() when 
we find an interrupt we call {disable,enable}_irq_wake() against which 
does not have an action?

Anyway, I think that the registering a dummy handler is a more correct 
way that does not make assumptions about how the interrupt subsystem works.

> 
> In the setup which needs this, does the output from the PHY go to a
> PMIC, not a SoC interrupt? And i assume the PMIC is not interrupt
> capable?

The PHY is connected to an always-on GPIO controller which generates an 
interrupt output to an out of band interrupt controller that wakes-up 
the SoC.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]