[net-next] net: phy: broadcom: Register dummy IRQ handler
Commit Message
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(-)
Comments
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>
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
+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.
@@ -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");
@@ -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 */
@@ -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;
}