LinuxLists.cc - [PATCH net-next v2 00/19] net: phy: add support for shared interrupts (part 1)

2020-11-01 12:54:09

Subject: [PATCH net-next v2 00/19] net: phy: add support for shared interrupts (part 1)

From: Ioana Ciornei <[email protected]>

This patch set aims to actually add support for shared interrupts in
phylib and not only for multi-PHY devices. While we are at it,
streamline the interrupt handling in phylib.

For a bit of context, at the moment, there are multiple phy_driver ops
that deal with this subject:

- .config_intr() - Enable/disable the interrupt line.

- .ack_interrupt() - Should quiesce any interrupts that may have been
fired. It's also used by phylib in conjunction with .config_intr() to
clear any pending interrupts after the line was disabled, and before
it is going to be enabled.

- .did_interrupt() - Intended for multi-PHY devices with a shared IRQ
line and used by phylib to discern which PHY from the package was the
one that actually fired the interrupt.

- .handle_interrupt() - Completely overrides the default interrupt
handling logic from phylib. The PHY driver is responsible for checking
if any interrupt was fired by the respective PHY and choose
accordingly if it's the one that should trigger the link state machine.

From my point of view, the interrupt handling in phylib has become
somewhat confusing with all these callbacks that actually read the same
PHY register - the interrupt status. A more streamlined approach would
be to just move the responsibility to write an interrupt handler to the
driver (as any other device driver does) and make .handle_interrupt()
the only way to deal with interrupts.

Another advantage with this approach would be that phylib would gain
support for shared IRQs between different PHY (not just multi-PHY
devices), something which at the moment would require extending every
PHY driver anyway in order to implement their .did_interrupt() callback
and duplicate the same logic as in .ack_interrupt(). The disadvantage
of making .did_interrupt() mandatory would be that we are slightly
changing the semantics of the phylib API and that would increase
confusion instead of reducing it.

What I am proposing is the following:

- As a first step, make the .ack_interrupt() callback optional so that
we do not break any PHY driver amid the transition.

- Every PHY driver gains a .handle_interrupt() implementation that, for
the most part, would look like below:

irq_status = phy_read(phydev, INTR_STATUS);
if (irq_status < 0) {
phy_error(phydev);
return IRQ_NONE;
}

if (!(irq_status & irq_mask))
return IRQ_NONE;

phy_trigger_machine(phydev);

return IRQ_HANDLED;

- Remove each PHY driver's implementation of the .ack_interrupt() by
actually taking care of quiescing any pending interrupts before
enabling/after disabling the interrupt line.

- Finally, after all drivers have been ported, remove the
.ack_interrupt() and .did_interrupt() callbacks from phy_driver.

This patch set is part 1 and it addresses the changes needed in phylib
and 7 PHY drivers. The rest can be found on my Github branch here:
https://github.com/IoanaCiornei/linux/commits/phylib-shared-irq

I do not have access to most of these PHY's, therefore I Cc-ed the
latest contributors to the individual PHY drivers in order to have
access, hopefully, to more regression testing.

Changes in v2:
- Rework the .handle_interrupt() implementation for each driver so that
only the enabled interrupts are taken into account when
IRQ_NONE/IRQ_HANDLED it returned. The main idea is so that we avoid
falsely blaming a device for triggering an interrupt when this is not
the case.
The only devices for which I was unable to make this adjustment were
the BCM8706, BCM8727, BCMAC131 and BCM5241 since I do not have access
to their datasheets.
- I also updated the pseudo-code added in the cover-letter so that it's
more clear how a .handle_interrupt() callback should look like.

Ioana Ciornei (19):
net: phy: export phy_error and phy_trigger_machine
net: phy: add a shutdown procedure
net: phy: make .ack_interrupt() optional
net: phy: at803x: implement generic .handle_interrupt() callback
net: phy: at803x: remove the use of .ack_interrupt()
net: phy: mscc: use phy_trigger_machine() to notify link change
net: phy: mscc: implement generic .handle_interrupt() callback
net: phy: mscc: remove the use of .ack_interrupt()
net: phy: aquantia: implement generic .handle_interrupt() callback
net: phy: aquantia: remove the use of .ack_interrupt()
net: phy: broadcom: implement generic .handle_interrupt() callback
net: phy: broadcom: remove use of ack_interrupt()
net: phy: cicada: implement the generic .handle_interrupt() callback
net: phy: cicada: remove the use of .ack_interrupt()
net: phy: davicom: implement generic .handle_interrupt() calback
net: phy: davicom: remove the use of .ack_interrupt()
net: phy: add genphy_handle_interrupt_no_ack()
net: phy: realtek: implement generic .handle_interrupt() callback
net: phy: realtek: remove the use of .ack_interrupt()

drivers/net/phy/aquantia_main.c | 59 +++++++++----
drivers/net/phy/at803x.c | 50 +++++++++--
drivers/net/phy/bcm-cygnus.c | 2 +-
drivers/net/phy/bcm-phy-lib.c | 49 ++++++++++-
drivers/net/phy/bcm-phy-lib.h | 1 +
drivers/net/phy/bcm54140.c | 46 +++++++---
drivers/net/phy/bcm63xx.c | 20 +++--
drivers/net/phy/bcm87xx.c | 50 ++++++-----
drivers/net/phy/broadcom.c | 70 +++++++++++-----
drivers/net/phy/cicada.c | 35 +++++++-
drivers/net/phy/davicom.c | 63 ++++++++++----
drivers/net/phy/mscc/mscc_main.c | 70 ++++++++--------
drivers/net/phy/phy.c | 6 +-
drivers/net/phy/phy_device.c | 23 ++++-
drivers/net/phy/realtek.c | 140 +++++++++++++++++++++++++++----
include/linux/phy.h | 3 +
16 files changed, 529 insertions(+), 158 deletions(-)

Cc: Alexandru Ardelean <[email protected]>
Cc: Andre Edich <[email protected]>
Cc: Antoine Tenart <[email protected]>
Cc: Baruch Siach <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Dan Murphy <[email protected]>
Cc: Divya Koppera <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: Heiner Kallweit <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Kavya Sree Kotagiri <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Marco Felsch <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Mathias Kresin <[email protected]>
Cc: Maxim Kochetkov <[email protected]>
Cc: Michael Walle <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Nisar Sayed <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: Philippe Schenker <[email protected]>
Cc: Willy Liu <[email protected]>
Cc: Yuiko Oshino <[email protected]>

--
2.28.0

2020-11-01 12:54:14

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 02/19] net: phy: add a shutdown procedure

From: Ioana Ciornei <[email protected]>

In case of a board which uses a shared IRQ we can easily end up with an
IRQ storm after a forced reboot.

For example, a 'reboot -f' will trigger a call to the .shutdown()
callbacks of all devices. Because phylib does not implement that hook,
the PHY is not quiesced, thus it can very well leave its IRQ enabled.

At the next boot, if that IRQ line is found asserted by the first PHY
driver that uses it, but _before_ the driver that is _actually_ keeping
the shared IRQ asserted is probed, the IRQ is not going to be
acknowledged, thus it will keep being fired preventing the boot process
of the kernel to continue. This is even worse when the second PHY driver
is a module.

To fix this, implement the .shutdown() callback and disable the
interrupts if these are used.

Note that we are still susceptible to IRQ storms if the previous kernel
exited with a panic or if the bootloader left the shared IRQ active, but
there is absolutely nothing we can do about these cases.

Cc: Alexandru Ardelean <[email protected]>
Cc: Andre Edich <[email protected]>
Cc: Antoine Tenart <[email protected]>
Cc: Baruch Siach <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Dan Murphy <[email protected]>
Cc: Divya Koppera <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: Heiner Kallweit <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Kavya Sree Kotagiri <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Marco Felsch <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Mathias Kresin <[email protected]>
Cc: Maxim Kochetkov <[email protected]>
Cc: Michael Walle <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Nisar Sayed <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: Philippe Schenker <[email protected]>
Cc: Willy Liu <[email protected]>
Cc: Yuiko Oshino <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- none

drivers/net/phy/phy_device.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5dab6be6fc38..413a0a2c5d51 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2947,6 +2947,13 @@ static int phy_remove(struct device *dev)
return 0;
}

+static void phy_shutdown(struct device *dev)
+{
+ struct phy_device *phydev = to_phy_device(dev);
+
+ phy_disable_interrupts(phydev);
+}
+
/**
* phy_driver_register - register a phy_driver with the PHY layer
* @new_driver: new phy_driver to register
@@ -2970,6 +2977,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)
new_driver->mdiodrv.driver.bus = &mdio_bus_type;
new_driver->mdiodrv.driver.probe = phy_probe;
new_driver->mdiodrv.driver.remove = phy_remove;
+ new_driver->mdiodrv.driver.shutdown = phy_shutdown;
new_driver->mdiodrv.driver.owner = owner;
new_driver->mdiodrv.driver.probe_type = PROBE_FORCE_SYNCHRONOUS;

--
2.28.0

2020-11-01 12:54:30

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 04/19] net: phy: at803x: implement generic .handle_interrupt() callback

2020-11-01 12:54:53

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 08/19] net: phy: mscc: remove the use of .ack_interrupt()

2020-11-01 12:55:05

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 19/19] net: phy: realtek: remove the use of .ack_interrupt()

2020-11-01 12:55:39

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 17/19] net: phy: add genphy_handle_interrupt_no_ack()

From: Ioana Ciornei <[email protected]>

It seems there are cases where the interrupts are handled by another
entity (ie an IRQ controller embedded inside the PHY) and do not need
any other interraction from phylib. For this kind of PHYs, like the
RTL8366RB, add the genphy_handle_interrupt_no_ack() function which just
triggers the link state machine.

Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- none

drivers/net/phy/phy_device.c | 13 +++++++++++++
include/linux/phy.h | 1 +
2 files changed, 14 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f54f483d7fd6..e13a46c25437 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2463,6 +2463,19 @@ int genphy_soft_reset(struct phy_device *phydev)
}
EXPORT_SYMBOL(genphy_soft_reset);

+irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev)
+{
+ /* It seems there are cases where the interrupts are handled by another
+ * entity (ie an IRQ controller embedded inside the PHY) and do not
+ * need any other interraction from phylib. In this case, just trigger
+ * the state machine directly.
+ */
+ phy_trigger_machine(phydev);
+
+ return 0;
+}
+EXPORT_SYMBOL(genphy_handle_interrupt_no_ack);
+
/**
* genphy_read_abilities - read PHY abilities from Clause 22 registers
* @phydev: target phy_device struct
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 566b39f6cd64..4f158d6352ae 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1510,6 +1510,7 @@ int genphy_suspend(struct phy_device *phydev);
int genphy_resume(struct phy_device *phydev);
int genphy_loopback(struct phy_device *phydev, bool enable);
int genphy_soft_reset(struct phy_device *phydev);
+irqreturn_t genphy_handle_interrupt_no_ack(struct phy_device *phydev);

static inline int genphy_config_aneg(struct phy_device *phydev)
{
--
2.28.0

2020-11-01 12:56:03

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 16/19] net: phy: davicom: remove the use of .ack_interrupt()

2020-11-01 12:56:07

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 18/19] net: phy: realtek: implement generic .handle_interrupt() callback

2020-11-01 12:56:55

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 11/19] net: phy: broadcom: implement generic .handle_interrupt() callback

From: Ioana Ciornei <[email protected]>

In an attempt to actually support shared IRQs in phylib, we now move the
responsibility of triggering the phylib state machine or just returning
IRQ_NONE, based on the IRQ status register, to the PHY driver. Having
3 different IRQ handling callbacks (.handle_interrupt(),
.did_interrupt() and .ack_interrupt() ) is confusing so let the PHY
driver implement directly an IRQ handler like any other device driver.
Make this driver follow the new convention.

Cc: Michael Walle <[email protected]>
Cc: Florian Fainelli <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- Adjust .handle_interrupt() so that we only take into account the
enabled IRQs. The BCM8706, BCM8727, BCMAC131 and BCM5241 were the
only devices which I did not do this change since I do not have
access to the register map.

drivers/net/phy/bcm-cygnus.c | 1 +
drivers/net/phy/bcm-phy-lib.c | 31 ++++++++++++++++++++++++++++++
drivers/net/phy/bcm-phy-lib.h | 1 +
drivers/net/phy/bcm54140.c | 26 ++++++++++++++++++++-----
drivers/net/phy/bcm63xx.c | 2 ++
drivers/net/phy/bcm87xx.c | 32 +++++++++++++++++++++----------
drivers/net/phy/broadcom.c | 36 +++++++++++++++++++++++++++++++++++
7 files changed, 114 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm-cygnus.c b/drivers/net/phy/bcm-cygnus.c
index 9ccf28b0a04d..a236e0b8d04d 100644
--- a/drivers/net/phy/bcm-cygnus.c
+++ b/drivers/net/phy/bcm-cygnus.c
@@ -258,6 +258,7 @@ static struct phy_driver bcm_cygnus_phy_driver[] = {
.config_init = bcm_cygnus_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
.resume = bcm_cygnus_resume,
}, {
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index ef6825b30323..c232fcfe0e20 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -196,6 +196,37 @@ int bcm_phy_config_intr(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(bcm_phy_config_intr);

+irqreturn_t bcm_phy_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status, irq_mask;
+
+ irq_status = phy_read(phydev, MII_BCM54XX_ISR);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ /* If a bit from the Interrupt Mask register is set, the corresponding
+ * bit from the Interrupt Status register is masked. So read the IMR
+ * and then flip the bits to get the list of possible interrupt
+ * sources.
+ */
+ irq_mask = phy_read(phydev, MII_BCM54XX_IMR);
+ if (irq_mask < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+ irq_mask = ~irq_mask;
+
+ if (!(irq_status & irq_mask))
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+
+ return IRQ_HANDLED;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_handle_interrupt);
+
int bcm_phy_read_shadow(struct phy_device *phydev, u16 shadow)
{
phy_write(phydev, MII_BCM54XX_SHD, MII_BCM54XX_SHD_VAL(shadow));
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 237a8503c9b4..c3842f87c33b 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -63,6 +63,7 @@ int bcm_phy_modify_rdb(struct phy_device *phydev, u16 rdb, u16 mask,

int bcm_phy_ack_intr(struct phy_device *phydev);
int bcm_phy_config_intr(struct phy_device *phydev);
+irqreturn_t bcm_phy_handle_interrupt(struct phy_device *phydev);

int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down);

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 8998e68bb26b..36c899a88c5d 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -637,13 +637,29 @@ static int bcm54140_config_init(struct phy_device *phydev)
BCM54140_RDB_C_PWR_ISOLATE, 0);
}

-static int bcm54140_did_interrupt(struct phy_device *phydev)
+static irqreturn_t bcm54140_handle_interrupt(struct phy_device *phydev)
{
- int ret;
+ int irq_status, irq_mask;
+
+ irq_status = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ irq_mask = bcm_phy_read_rdb(phydev, BCM54140_RDB_IMR);
+ if (irq_mask < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+ irq_mask = ~irq_mask;
+
+ if (!(irq_status & irq_mask))
+ return IRQ_NONE;

- ret = bcm_phy_read_rdb(phydev, BCM54140_RDB_ISR);
+ phy_trigger_machine(phydev);

- return (ret < 0) ? 0 : ret;
+ return IRQ_HANDLED;
}

static int bcm54140_ack_intr(struct phy_device *phydev)
@@ -834,8 +850,8 @@ static struct phy_driver bcm54140_drivers[] = {
.flags = PHY_POLL_CABLE_TEST,
.features = PHY_GBIT_FEATURES,
.config_init = bcm54140_config_init,
- .did_interrupt = bcm54140_did_interrupt,
.ack_interrupt = bcm54140_ack_intr,
+ .handle_interrupt = bcm54140_handle_interrupt,
.config_intr = bcm54140_config_intr,
.probe = bcm54140_probe,
.suspend = genphy_suspend,
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index 459fb2069c7e..818c853b6638 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -69,6 +69,7 @@ static struct phy_driver bcm63xx_driver[] = {
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
/* same phy as above, with just a different OUI */
.phy_id = 0x002bdc00,
@@ -79,6 +80,7 @@ static struct phy_driver bcm63xx_driver[] = {
.config_init = bcm63xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
} };

module_phy_driver(bcm63xx_driver);
diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index df360e1c5069..f20cfb05ef04 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -153,10 +153,29 @@ static int bcm87xx_config_intr(struct phy_device *phydev)
return err;
}

-static int bcm87xx_did_interrupt(struct phy_device *phydev)
+static irqreturn_t bcm87xx_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status;
+
+ irq_status = phy_read(phydev, BCM87XX_LASI_STATUS);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (irq_status == 0)
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+
+ return IRQ_HANDLED;
+}
+
+static int bcm87xx_ack_interrupt(struct phy_device *phydev)
{
int reg;

+ /* Reading the LASI status clears it. */
reg = phy_read(phydev, BCM87XX_LASI_STATUS);

if (reg < 0) {
@@ -168,13 +187,6 @@ static int bcm87xx_did_interrupt(struct phy_device *phydev)
return (reg & 1) != 0;
}

-static int bcm87xx_ack_interrupt(struct phy_device *phydev)
-{
- /* Reading the LASI status clears it. */
- bcm87xx_did_interrupt(phydev);
- return 0;
-}
-
static int bcm8706_match_phy_device(struct phy_device *phydev)
{
return phydev->c45_ids.device_ids[4] == PHY_ID_BCM8706;
@@ -196,7 +208,7 @@ static struct phy_driver bcm87xx_driver[] = {
.read_status = bcm87xx_read_status,
.ack_interrupt = bcm87xx_ack_interrupt,
.config_intr = bcm87xx_config_intr,
- .did_interrupt = bcm87xx_did_interrupt,
+ .handle_interrupt = bcm87xx_handle_interrupt,
.match_phy_device = bcm8706_match_phy_device,
}, {
.phy_id = PHY_ID_BCM8727,
@@ -208,7 +220,7 @@ static struct phy_driver bcm87xx_driver[] = {
.read_status = bcm87xx_read_status,
.ack_interrupt = bcm87xx_ack_interrupt,
.config_intr = bcm87xx_config_intr,
- .did_interrupt = bcm87xx_did_interrupt,
+ .handle_interrupt = bcm87xx_handle_interrupt,
.match_phy_device = bcm8727_match_phy_device,
} };

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index cd271de9609b..8bcdb94ef2fc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -643,6 +643,24 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
return err;
}

+static irqreturn_t brcm_fet_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status;
+
+ irq_status = phy_read(phydev, MII_BRCM_FET_INTREG);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (irq_status == 0)
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+
+ return IRQ_HANDLED;
+}
+
struct bcm53xx_phy_priv {
u64 *stats;
};
@@ -683,6 +701,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM5421,
.phy_id_mask = 0xfffffff0,
@@ -691,6 +710,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM54210E,
.phy_id_mask = 0xfffffff0,
@@ -699,6 +719,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM5461,
.phy_id_mask = 0xfffffff0,
@@ -707,6 +728,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM54612E,
.phy_id_mask = 0xfffffff0,
@@ -715,6 +737,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM54616S,
.phy_id_mask = 0xfffffff0,
@@ -724,6 +747,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg = bcm54616s_config_aneg,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
.read_status = bcm54616s_read_status,
.probe = bcm54616s_probe,
}, {
@@ -734,6 +758,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
.resume = genphy_resume,
}, {
@@ -745,6 +770,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg = bcm5481_config_aneg,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM54810,
.phy_id_mask = 0xfffffff0,
@@ -754,6 +780,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg = bcm5481_config_aneg,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
.resume = bcm54xx_resume,
}, {
@@ -765,6 +792,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg = bcm5481_config_aneg,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
.resume = bcm54xx_resume,
}, {
@@ -776,6 +804,7 @@ static struct phy_driver broadcom_drivers[] = {
.read_status = bcm5482_read_status,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM50610,
.phy_id_mask = 0xfffffff0,
@@ -784,6 +813,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM50610M,
.phy_id_mask = 0xfffffff0,
@@ -792,6 +822,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM57780,
.phy_id_mask = 0xfffffff0,
@@ -800,6 +831,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCMAC131,
.phy_id_mask = 0xfffffff0,
@@ -808,6 +840,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = brcm_fet_config_init,
.ack_interrupt = brcm_fet_ack_interrupt,
.config_intr = brcm_fet_config_intr,
+ .handle_interrupt = brcm_fet_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM5241,
.phy_id_mask = 0xfffffff0,
@@ -816,6 +849,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = brcm_fet_config_init,
.ack_interrupt = brcm_fet_ack_interrupt,
.config_intr = brcm_fet_config_intr,
+ .handle_interrupt = brcm_fet_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM5395,
.phy_id_mask = 0xfffffff0,
@@ -839,6 +873,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
}, {
.phy_id = PHY_ID_BCM89610,
.phy_id_mask = 0xfffffff0,
@@ -847,6 +882,7 @@ static struct phy_driver broadcom_drivers[] = {
.config_init = bcm54xx_config_init,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .handle_interrupt = bcm_phy_handle_interrupt,
} };

module_phy_driver(broadcom_drivers);
--
2.28.0

2020-11-01 12:57:01

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 09/19] net: phy: aquantia: implement generic .handle_interrupt() callback

2020-11-01 12:57:04

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 05/19] net: phy: at803x: remove the use of .ack_interrupt()

2020-11-01 12:57:05

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 07/19] net: phy: mscc: implement generic .handle_interrupt() callback

2020-11-01 12:57:40

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 06/19] net: phy: mscc: use phy_trigger_machine() to notify link change

From: Ioana Ciornei <[email protected]>

According to the comment describing the phy_mac_interrupt() function, it
it intended to be used by MAC drivers which have noticed a link change
thus its use in the mscc PHY driver is improper and, most probably, was
added just because phy_trigger_machine() was not exported.
Now that we have acces to trigger the link state machine, use directly
the phy_trigger_machine() function to notify a link change detected by
the PHY driver.

Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- none

drivers/net/phy/mscc/mscc_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 6bc7406a1ce7..b705121c9d26 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1498,7 +1498,7 @@ static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev)
vsc8584_handle_macsec_interrupt(phydev);

if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG)
- phy_mac_interrupt(phydev);
+ phy_trigger_machine(phydev);

return IRQ_HANDLED;
}
--
2.28.0

2020-11-01 12:57:58

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 10/19] net: phy: aquantia: remove the use of .ack_interrupt()

2020-11-01 12:58:14

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 15/19] net: phy: davicom: implement generic .handle_interrupt() calback

2020-11-01 12:58:14

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 14/19] net: phy: cicada: remove the use of .ack_interrupt()

2020-11-01 12:58:21

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 13/19] net: phy: cicada: implement the generic .handle_interrupt() callback

2020-11-01 12:58:22

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 03/19] net: phy: make .ack_interrupt() optional

From: Ioana Ciornei <[email protected]>

As a first step into making phylib and all PHY drivers to actually
have support for shared IRQs, make the .ack_interrupt() callback
optional.

After all drivers have been moved to implement the generic
interrupt handle, the phy_drv_supports_irq() check will be
changed again to only require the .handle_interrupts() callback.

Cc: Alexandru Ardelean <[email protected]>
Cc: Andre Edich <[email protected]>
Cc: Antoine Tenart <[email protected]>
Cc: Baruch Siach <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Dan Murphy <[email protected]>
Cc: Divya Koppera <[email protected]>
Cc: Florian Fainelli <[email protected]>
Cc: Hauke Mehrtens <[email protected]>
Cc: Heiner Kallweit <[email protected]>
Cc: Jerome Brunet <[email protected]>
Cc: Kavya Sree Kotagiri <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Marco Felsch <[email protected]>
Cc: Marek Vasut <[email protected]>
Cc: Martin Blumenstingl <[email protected]>
Cc: Mathias Kresin <[email protected]>
Cc: Maxim Kochetkov <[email protected]>
Cc: Michael Walle <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Nisar Sayed <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: Philippe Schenker <[email protected]>
Cc: Willy Liu <[email protected]>
Cc: Yuiko Oshino <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- none

drivers/net/phy/phy_device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 413a0a2c5d51..f54f483d7fd6 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2815,7 +2815,7 @@ EXPORT_SYMBOL(phy_get_internal_delay);

static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
- return phydrv->config_intr && phydrv->ack_interrupt;
+ return phydrv->config_intr && (phydrv->ack_interrupt || phydrv->handle_interrupt);
}

/**
--
2.28.0

2020-11-01 12:58:38

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next v2 12/19] net: phy: broadcom: remove use of ack_interrupt()

From: Ioana Ciornei <[email protected]>

In preparation of removing the .ack_interrupt() callback, we must replace
its occurrences (aka phy_clear_interrupt), from the 2 places where it is
called from (phy_enable_interrupts and phy_disable_interrupts), with
equivalent functionality.

This means that clearing interrupts now becomes something that the PHY
driver is responsible of doing, before enabling interrupts and after
clearing them. Make this driver follow the new contract.

Cc: Michael Walle <[email protected]>
Cc: Florian Fainelli <[email protected]>
Signed-off-by: Ioana Ciornei <[email protected]>
---
Changes in v2:
- none

drivers/net/phy/bcm-cygnus.c | 1 -
drivers/net/phy/bcm-phy-lib.c | 18 ++++++++++++++----
drivers/net/phy/bcm54140.c | 20 +++++++++++++++-----
drivers/net/phy/bcm63xx.c | 18 +++++++++++++-----
drivers/net/phy/bcm87xx.c | 34 +++++++++++++---------------------
drivers/net/phy/broadcom.c | 34 +++++++++++++---------------------
6 files changed, 68 insertions(+), 57 deletions(-)

diff --git a/drivers/net/phy/bcm-cygnus.c b/drivers/net/phy/bcm-cygnus.c
index a236e0b8d04d..da8f7cb41b44 100644
--- a/drivers/net/phy/bcm-cygnus.c
+++ b/drivers/net/phy/bcm-cygnus.c
@@ -256,7 +256,6 @@ static struct phy_driver bcm_cygnus_phy_driver[] = {
.name = "Broadcom Cygnus PHY",
/* PHY_GBIT_FEATURES */
.config_init = bcm_cygnus_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index c232fcfe0e20..53282a6d5928 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -181,18 +181,28 @@ EXPORT_SYMBOL_GPL(bcm_phy_ack_intr);

int bcm_phy_config_intr(struct phy_device *phydev)
{
- int reg;
+ int reg, err;

reg = phy_read(phydev, MII_BCM54XX_ECR);
if (reg < 0)
return reg;

- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = bcm_phy_ack_intr(phydev);
+ if (err)
+ return err;
+
reg &= ~MII_BCM54XX_ECR_IM;
- else
+ err = phy_write(phydev, MII_BCM54XX_ECR, reg);
+ } else {
reg |= MII_BCM54XX_ECR_IM;
+ err = phy_write(phydev, MII_BCM54XX_ECR, reg);
+ if (err)
+ return err;

- return phy_write(phydev, MII_BCM54XX_ECR, reg);
+ err = bcm_phy_ack_intr(phydev);
+ }
+ return err;
}
EXPORT_SYMBOL_GPL(bcm_phy_config_intr);

diff --git a/drivers/net/phy/bcm54140.c b/drivers/net/phy/bcm54140.c
index 36c899a88c5d..d8f3024860dc 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -681,7 +681,7 @@ static int bcm54140_config_intr(struct phy_device *phydev)
BCM54140_RDB_TOP_IMR_PORT0, BCM54140_RDB_TOP_IMR_PORT1,
BCM54140_RDB_TOP_IMR_PORT2, BCM54140_RDB_TOP_IMR_PORT3,
};
- int reg;
+ int reg, err;

if (priv->port >= ARRAY_SIZE(port_to_imr_bit))
return -EINVAL;
@@ -690,12 +690,23 @@ static int bcm54140_config_intr(struct phy_device *phydev)
if (reg < 0)
return reg;

- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = bcm54140_ack_intr(phydev);
+ if (err)
+ return err;
+
reg &= ~port_to_imr_bit[priv->port];
- else
+ err = bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+ } else {
reg |= port_to_imr_bit[priv->port];
+ err = bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+ if (err)
+ return err;
+
+ err = bcm54140_ack_intr(phydev);
+ }

- return bcm54140_base_write_rdb(phydev, BCM54140_RDB_TOP_IMR, reg);
+ return err;
}

static int bcm54140_get_downshift(struct phy_device *phydev, u8 *data)
@@ -850,7 +861,6 @@ static struct phy_driver bcm54140_drivers[] = {
.flags = PHY_POLL_CABLE_TEST,
.features = PHY_GBIT_FEATURES,
.config_init = bcm54140_config_init,
- .ack_interrupt = bcm54140_ack_intr,
.handle_interrupt = bcm54140_handle_interrupt,
.config_intr = bcm54140_config_intr,
.probe = bcm54140_probe,
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index 818c853b6638..0eb33be824f1 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -25,12 +25,22 @@ static int bcm63xx_config_intr(struct phy_device *phydev)
if (reg < 0)
return reg;

- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = bcm_phy_ack_intr(phydev);
+ if (err)
+ return err;
+
reg &= ~MII_BCM63XX_IR_GMASK;
- else
+ err = phy_write(phydev, MII_BCM63XX_IR, reg);
+ } else {
reg |= MII_BCM63XX_IR_GMASK;
+ err = phy_write(phydev, MII_BCM63XX_IR, reg);
+ if (err)
+ return err;
+
+ err = bcm_phy_ack_intr(phydev);
+ }

- err = phy_write(phydev, MII_BCM63XX_IR, reg);
return err;
}

@@ -67,7 +77,6 @@ static struct phy_driver bcm63xx_driver[] = {
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -78,7 +87,6 @@ static struct phy_driver bcm63xx_driver[] = {
/* PHY_BASIC_FEATURES */
.flags = PHY_IS_INTERNAL,
.config_init = bcm63xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm63xx_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
} };
diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index f20cfb05ef04..4ac8fd190e9d 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -144,12 +144,22 @@ static int bcm87xx_config_intr(struct phy_device *phydev)
if (reg < 0)
return reg;

- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = phy_read(phydev, BCM87XX_LASI_STATUS);
+ if (err)
+ return err;
+
reg |= 1;
- else
+ err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
+ } else {
reg &= ~1;
+ err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
+ if (err)
+ return err;
+
+ err = phy_read(phydev, BCM87XX_LASI_STATUS);
+ }

- err = phy_write(phydev, BCM87XX_LASI_CONTROL, reg);
return err;
}

@@ -171,22 +181,6 @@ static irqreturn_t bcm87xx_handle_interrupt(struct phy_device *phydev)
return IRQ_HANDLED;
}

-static int bcm87xx_ack_interrupt(struct phy_device *phydev)
-{
- int reg;
-
- /* Reading the LASI status clears it. */
- reg = phy_read(phydev, BCM87XX_LASI_STATUS);
-
- if (reg < 0) {
- phydev_err(phydev,
- "Error: Read of BCM87XX_LASI_STATUS failed: %d\n",
- reg);
- return 0;
- }
- return (reg & 1) != 0;
-}
-
static int bcm8706_match_phy_device(struct phy_device *phydev)
{
return phydev->c45_ids.device_ids[4] == PHY_ID_BCM8706;
@@ -206,7 +200,6 @@ static struct phy_driver bcm87xx_driver[] = {
.config_init = bcm87xx_config_init,
.config_aneg = bcm87xx_config_aneg,
.read_status = bcm87xx_read_status,
- .ack_interrupt = bcm87xx_ack_interrupt,
.config_intr = bcm87xx_config_intr,
.handle_interrupt = bcm87xx_handle_interrupt,
.match_phy_device = bcm8706_match_phy_device,
@@ -218,7 +211,6 @@ static struct phy_driver bcm87xx_driver[] = {
.config_init = bcm87xx_config_init,
.config_aneg = bcm87xx_config_aneg,
.read_status = bcm87xx_read_status,
- .ack_interrupt = bcm87xx_ack_interrupt,
.config_intr = bcm87xx_config_intr,
.handle_interrupt = bcm87xx_handle_interrupt,
.match_phy_device = bcm8727_match_phy_device,
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 8bcdb94ef2fc..8a4ec3222168 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -634,12 +634,22 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
if (reg < 0)
return reg;

- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ err = brcm_fet_ack_interrupt(phydev);
+ if (err)
+ return err;
+
reg &= ~MII_BRCM_FET_IR_MASK;
- else
+ err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
+ } else {
reg |= MII_BRCM_FET_IR_MASK;
+ err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
+ if (err)
+ return err;
+
+ err = brcm_fet_ack_interrupt(phydev);
+ }

- err = phy_write(phydev, MII_BRCM_FET_INTREG, reg);
return err;
}

@@ -699,7 +709,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5411",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -708,7 +717,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5421",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -717,7 +725,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM54210E",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -726,7 +733,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5461",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -735,7 +741,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM54612E",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -745,7 +750,6 @@ static struct phy_driver broadcom_drivers[] = {
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
.config_aneg = bcm54616s_config_aneg,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
.read_status = bcm54616s_read_status,
@@ -756,7 +760,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5464",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
@@ -768,7 +771,6 @@ static struct phy_driver broadcom_drivers[] = {
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
.config_aneg = bcm5481_config_aneg,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -778,7 +780,6 @@ static struct phy_driver broadcom_drivers[] = {
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
.config_aneg = bcm5481_config_aneg,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
@@ -790,7 +791,6 @@ static struct phy_driver broadcom_drivers[] = {
/* PHY_GBIT_FEATURES */
.config_init = bcm54811_config_init,
.config_aneg = bcm5481_config_aneg,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
.suspend = genphy_suspend,
@@ -802,7 +802,6 @@ static struct phy_driver broadcom_drivers[] = {
/* PHY_GBIT_FEATURES */
.config_init = bcm5482_config_init,
.read_status = bcm5482_read_status,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -811,7 +810,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM50610",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -820,7 +818,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM50610M",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -829,7 +826,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM57780",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -838,7 +834,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCMAC131",
/* PHY_BASIC_FEATURES */
.config_init = brcm_fet_config_init,
- .ack_interrupt = brcm_fet_ack_interrupt,
.config_intr = brcm_fet_config_intr,
.handle_interrupt = brcm_fet_handle_interrupt,
}, {
@@ -847,7 +842,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5241",
/* PHY_BASIC_FEATURES */
.config_init = brcm_fet_config_init,
- .ack_interrupt = brcm_fet_ack_interrupt,
.config_intr = brcm_fet_config_intr,
.handle_interrupt = brcm_fet_handle_interrupt,
}, {
@@ -871,7 +865,6 @@ static struct phy_driver broadcom_drivers[] = {
.get_stats = bcm53xx_phy_get_stats,
.probe = bcm53xx_phy_probe,
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
}, {
@@ -880,7 +873,6 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM89610",
/* PHY_GBIT_FEATURES */
.config_init = bcm54xx_config_init,
- .ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
.handle_interrupt = bcm_phy_handle_interrupt,
} };
--
2.28.0

2020-11-03 16:49:18

by Andrew Lunn

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/19] net: phy: add support for shared interrupts (part 1)

On Sun, Nov 01, 2020 at 02:50:55PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> This patch set aims to actually add support for shared interrupts in
> phylib and not only for multi-PHY devices. While we are at it,
> streamline the interrupt handling in phylib.

Hi Ioana

I did some quick testing with the Marvell PHY. This is one of the
drivers you have not changed. But it at least tests some of the core
changes.

Tested-by: Andrew Lunn <[email protected]>

I will try to review the actual changes soon.

Andrew

2020-11-03 22:54:56

by Michael Walle

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/19] net: phy: add support for shared interrupts (part 1)

Am 2020-11-01 13:50, schrieb Ioana Ciornei:
> From: Ioana Ciornei <[email protected]>
>
> This patch set aims to actually add support for shared interrupts in
> phylib and not only for multi-PHY devices. While we are at it,
> streamline the interrupt handling in phylib.

For the Broadcom BCM54140:
Tested-by: Michael Walle <[email protected]>

-michael

2020-11-04 20:36:33

by Oleksij Rempel

[permalink] [raw]

Subject: Re: [PATCH net-next v2 05/19] net: phy: at803x: remove the use of .ack_interrupt()

On Sun, Nov 01, 2020 at 02:51:00PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> In preparation of removing the .ack_interrupt() callback, we must replace
> its occurrences (aka phy_clear_interrupt), from the 2 places where it is
> called from (phy_enable_interrupts and phy_disable_interrupts), with
> equivalent functionality.
>
> This means that clearing interrupts now becomes something that the PHY
> driver is responsible of doing, before enabling interrupts and after
> clearing them. Make this driver follow the new contract.
>
> Cc: Oleksij Rempel <[email protected]>
> Cc: Michael Walle <[email protected]>
> Signed-off-by: Ioana Ciornei <[email protected]>

Tested-by: Oleksij Rempel <[email protected]>

> ---
> Changes in v2:
> - none
>
> drivers/net/phy/at803x.c | 19 ++++++++++++-------
> 1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index c7f91934cf82..d0b36fd6c265 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -614,6 +614,11 @@ static int at803x_config_intr(struct phy_device *phydev)
> value = phy_read(phydev, AT803X_INTR_ENABLE);
>
> if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + /* Clear any pending interrupts */
> + err = at803x_ack_interrupt(phydev);
> + if (err)
> + return err;
> +
> value |= AT803X_INTR_ENABLE_AUTONEG_ERR;
> value |= AT803X_INTR_ENABLE_SPEED_CHANGED;
> value |= AT803X_INTR_ENABLE_DUPLEX_CHANGED;
> @@ -621,9 +626,14 @@ static int at803x_config_intr(struct phy_device *phydev)
> value |= AT803X_INTR_ENABLE_LINK_SUCCESS;
>
> err = phy_write(phydev, AT803X_INTR_ENABLE, value);
> - }
> - else
> + } else {
> err = phy_write(phydev, AT803X_INTR_ENABLE, 0);
> + if (err)
> + return err;
> +
> + /* Clear any pending interrupts */
> + err = at803x_ack_interrupt(phydev);
> + }
>
> return err;
> }
> @@ -1088,7 +1098,6 @@ static struct phy_driver at803x_driver[] = {
> .resume = at803x_resume,
> /* PHY_GBIT_FEATURES */
> .read_status = at803x_read_status,
> - .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> .get_tunable = at803x_get_tunable,
> @@ -1109,7 +1118,6 @@ static struct phy_driver at803x_driver[] = {
> .suspend = at803x_suspend,
> .resume = at803x_resume,
> /* PHY_BASIC_FEATURES */
> - .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> }, {
> @@ -1128,7 +1136,6 @@ static struct phy_driver at803x_driver[] = {
> /* PHY_GBIT_FEATURES */
> .read_status = at803x_read_status,
> .aneg_done = at803x_aneg_done,
> - .ack_interrupt = &at803x_ack_interrupt,
> .config_intr = &at803x_config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> .get_tunable = at803x_get_tunable,
> @@ -1149,7 +1156,6 @@ static struct phy_driver at803x_driver[] = {
> .suspend = at803x_suspend,
> .resume = at803x_resume,
> /* PHY_BASIC_FEATURES */
> - .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> .cable_test_start = at803x_cable_test_start,
> @@ -1162,7 +1168,6 @@ static struct phy_driver at803x_driver[] = {
> .resume = at803x_resume,
> .flags = PHY_POLL_CABLE_TEST,
> /* PHY_BASIC_FEATURES */
> - .ack_interrupt = &at803x_ack_interrupt,
> .config_intr = &at803x_config_intr,
> .handle_interrupt = at803x_handle_interrupt,
> .cable_test_start = at803x_cable_test_start,
> --
> 2.28.0
>
>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2020-11-05 00:11:38

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH net-next v2 08/19] net: phy: mscc: remove the use of .ack_interrupt()

On Sun, Nov 01, 2020 at 02:51:03PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> In preparation of removing the .ack_interrupt() callback, we must replace
> its occurrences (aka phy_clear_interrupt), from the 2 places where it is
> called from (phy_enable_interrupts and phy_disable_interrupts), with
> equivalent functionality.
>
> This means that clearing interrupts now becomes something that the PHY
> driver is responsible of doing, before enabling interrupts and after
> clearing them. Make this driver follow the new contract.
>
> Cc: Antoine Tenart <[email protected]>
> Signed-off-by: Ioana Ciornei <[email protected]>
> ---

Tested-by: Vladimir Oltean <[email protected]> # VSC8514

2020-11-05 00:12:14

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH net-next v2 07/19] net: phy: mscc: implement generic .handle_interrupt() callback

On Sun, Nov 01, 2020 at 02:51:02PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> In an attempt to actually support shared IRQs in phylib, we now move the
> responsibility of triggering the phylib state machine or just returning
> IRQ_NONE, based on the IRQ status register, to the PHY driver. Having
> 3 different IRQ handling callbacks (.handle_interrupt(),
> .did_interrupt() and .ack_interrupt() ) is confusing so let the PHY
> driver implement directly an IRQ handler like any other device driver.
> Make this driver follow the new convention.
>
> Also, remove the .did_interrupt() callback since it's not anymore used.
>
> Signed-off-by: Ioana Ciornei <[email protected]>
> ---

Tested-by: Vladimir Oltean <[email protected]> # VSC8514

2020-11-05 00:13:00

by Vladimir Oltean

[permalink] [raw]

Subject: Re: [PATCH net-next v2 06/19] net: phy: mscc: use phy_trigger_machine() to notify link change

On Sun, Nov 01, 2020 at 02:51:01PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> According to the comment describing the phy_mac_interrupt() function, it
> it intended to be used by MAC drivers which have noticed a link change
> thus its use in the mscc PHY driver is improper and, most probably, was
> added just because phy_trigger_machine() was not exported.
> Now that we have acces to trigger the link state machine, use directly
> the phy_trigger_machine() function to notify a link change detected by
> the PHY driver.
>
> Signed-off-by: Ioana Ciornei <[email protected]>
> ---

Tested-by: Vladimir Oltean <[email protected]>

2020-11-05 00:31:02

by Oleksij Rempel

[permalink] [raw]

Subject: Re: [PATCH net-next v2 04/19] net: phy: at803x: implement generic .handle_interrupt() callback

On Sun, Nov 01, 2020 at 02:50:59PM +0200, Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> In an attempt to actually support shared IRQs in phylib, we now move the
> responsibility of triggering the phylib state machine or just returning
> IRQ_NONE, based on the IRQ status register, to the PHY driver. Having
> 3 different IRQ handling callbacks (.handle_interrupt(),
> .did_interrupt() and .ack_interrupt() ) is confusing so let the PHY
> driver implement directly an IRQ handler like any other device driver.
> Make this driver follow the new convention.
>
> Cc: Oleksij Rempel <[email protected]>
> Cc: Michael Walle <[email protected]>
> Signed-off-by: Ioana Ciornei <[email protected]>

Tested-by: Oleksij Rempel <[email protected]>

> ---
> Changes in v2:
> - adjust .handle_interrupt() so that we only take into account the
> enabled IRQs.
>
> drivers/net/phy/at803x.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index ed601a7e46a0..c7f91934cf82 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -628,6 +628,32 @@ static int at803x_config_intr(struct phy_device *phydev)
> return err;
> }
>
> +static irqreturn_t at803x_handle_interrupt(struct phy_device *phydev)
> +{
> + int irq_status, int_enabled;
> +
> + irq_status = phy_read(phydev, AT803X_INTR_STATUS);
> + if (irq_status < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + /* Read the current enabled interrupts */
> + int_enabled = phy_read(phydev, AT803X_INTR_ENABLE);
> + if (int_enabled < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + /* See if this was one of our enabled interrupts */
> + if (!(irq_status & int_enabled))
> + return IRQ_NONE;
> +
> + phy_trigger_machine(phydev);
> +
> + return IRQ_HANDLED;
> +}
> +
> static void at803x_link_change_notify(struct phy_device *phydev)
> {
> /*
> @@ -1064,6 +1090,7 @@ static struct phy_driver at803x_driver[] = {
> .read_status = at803x_read_status,
> .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> + .handle_interrupt = at803x_handle_interrupt,
> .get_tunable = at803x_get_tunable,
> .set_tunable = at803x_set_tunable,
> .cable_test_start = at803x_cable_test_start,
> @@ -1084,6 +1111,7 @@ static struct phy_driver at803x_driver[] = {
> /* PHY_BASIC_FEATURES */
> .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> + .handle_interrupt = at803x_handle_interrupt,
> }, {
> /* Qualcomm Atheros AR8031/AR8033 */
> PHY_ID_MATCH_EXACT(ATH8031_PHY_ID),
> @@ -1102,6 +1130,7 @@ static struct phy_driver at803x_driver[] = {
> .aneg_done = at803x_aneg_done,
> .ack_interrupt = &at803x_ack_interrupt,
> .config_intr = &at803x_config_intr,
> + .handle_interrupt = at803x_handle_interrupt,
> .get_tunable = at803x_get_tunable,
> .set_tunable = at803x_set_tunable,
> .cable_test_start = at803x_cable_test_start,
> @@ -1122,6 +1151,7 @@ static struct phy_driver at803x_driver[] = {
> /* PHY_BASIC_FEATURES */
> .ack_interrupt = at803x_ack_interrupt,
> .config_intr = at803x_config_intr,
> + .handle_interrupt = at803x_handle_interrupt,
> .cable_test_start = at803x_cable_test_start,
> .cable_test_get_status = at803x_cable_test_get_status,
> }, {
> @@ -1134,6 +1164,7 @@ static struct phy_driver at803x_driver[] = {
> /* PHY_BASIC_FEATURES */
> .ack_interrupt = &at803x_ack_interrupt,
> .config_intr = &at803x_config_intr,
> + .handle_interrupt = at803x_handle_interrupt,
> .cable_test_start = at803x_cable_test_start,
> .cable_test_get_status = at803x_cable_test_get_status,
> .read_status = at803x_read_status,
> --
> 2.28.0
>
>

--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2020-11-06 00:41:19

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH net-next v2 00/19] net: phy: add support for shared interrupts (part 1)

On Sun, 1 Nov 2020 14:50:55 +0200 Ioana Ciornei wrote:
> From: Ioana Ciornei <[email protected]>
>
> This patch set aims to actually add support for shared interrupts in
> phylib and not only for multi-PHY devices. While we are at it,
> streamline the interrupt handling in phylib.

Applied, thank you!

2023-08-03 18:41:22

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/19] net: phy: add a shutdown procedure

Hello,

this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
contained in v5.11-rc1.

It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
report it now as I just upgraded that machine from Debian 11 (with
kernel 5.10.x) to Debian 12 (with kernel 6.1.x).

Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
fixes the problem for me.

On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
> In case of a board which uses a shared IRQ we can easily end up with an
> IRQ storm after a forced reboot.
>
> For example, a 'reboot -f' will trigger a call to the .shutdown()
> callbacks of all devices. Because phylib does not implement that hook,
> the PHY is not quiesced, thus it can very well leave its IRQ enabled.
>
> At the next boot, if that IRQ line is found asserted by the first PHY
> driver that uses it, but _before_ the driver that is _actually_ keeping
> the shared IRQ asserted is probed, the IRQ is not going to be
> acknowledged, thus it will keep being fired preventing the boot process
> of the kernel to continue. This is even worse when the second PHY driver
> is a module.
>
> To fix this, implement the .shutdown() callback and disable the
> interrupts if these are used.

I don't know how this should interact with wake-on-lan, but I would
expect that there is a way to fix this without reintroducing the problem
fixed by this change. However I cannot say if this needs fixing in the
generic phy code or the phy driver. Any hints?

> Note that we are still susceptible to IRQ storms if the previous kernel
> exited with a panic or if the bootloader left the shared IRQ active, but
> there is absolutely nothing we can do about these cases.

I'd say the bootloader could handle that, knowing that for some machines
changing the bootloader isn't an option.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (2.09 kB)
signature.asc (499.00 B)
Download all attachments

2023-08-03 19:19:18

by Russell King (Oracle)

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/19] net: phy: add a shutdown procedure

On Thu, Aug 03, 2023 at 11:24:04AM -0700, Florian Fainelli wrote:
> On 8/3/23 11:16, Uwe Kleine-K?nig wrote:
> > Hello,
> >
> > this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
> > contained in v5.11-rc1.
> >
> > It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
> > armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
> > report it now as I just upgraded that machine from Debian 11 (with
> > kernel 5.10.x) to Debian 12 (with kernel 6.1.x).
> >
> > Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
> > fixes the problem for me.
> >
> > On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
> > > In case of a board which uses a shared IRQ we can easily end up with an
> > > IRQ storm after a forced reboot.
> > >
> > > For example, a 'reboot -f' will trigger a call to the .shutdown()
> > > callbacks of all devices. Because phylib does not implement that hook,
> > > the PHY is not quiesced, thus it can very well leave its IRQ enabled.
> > >
> > > At the next boot, if that IRQ line is found asserted by the first PHY
> > > driver that uses it, but _before_ the driver that is _actually_ keeping
> > > the shared IRQ asserted is probed, the IRQ is not going to be
> > > acknowledged, thus it will keep being fired preventing the boot process
> > > of the kernel to continue. This is even worse when the second PHY driver
> > > is a module.
> > >
> > > To fix this, implement the .shutdown() callback and disable the
> > > interrupts if these are used.
> >
> > I don't know how this should interact with wake-on-lan, but I would
> > expect that there is a way to fix this without reintroducing the problem
> > fixed by this change. However I cannot say if this needs fixing in the
> > generic phy code or the phy driver. Any hints?
>
> It depends upon what the PHY drivers and underlying hardware are capable and
> willing to do. Some PHY drivers will shutdown the TX path completely since
> you do not need that part to receive Wake-on-LAN packets and pass them up to
> the PHY and/or MAC Wake-on-LAN matching logic. This would invite us to let
> individual PHY drivers make a decision as to what they want to do in a
> .shutdown() routine that would then need to be added to each and every
> driver that wants to do something special. In the absence of said routine,
> you could default to calling phy_disable_interrupts() unless the PHY has WoL
> enabled?
>
> phydev::wol_enabled reflects whether the PHY and/or the MAC has Wake-on-LAN
> enabled which could you could key off to "nullify" what the shutdown does.

If the shutdown method is being called, doesn't that imply that we're
going to be hibernating rather than suspend-to-RAM - because suspend-to
RAM doesn't involve powering down the system, whereas hibernating does.

I think the problem is that when placing the system into hibernate mode
with WoL enabled, there are PHYs which use their interrupt pin to signal
that a WoL event has happened. The problem comes with the aforementioned
commit that the core PHY code now _always_ disables interrupts on the
PHY, even if WoL is active and we're entering hibernate mode.

That's clearly the wrong thing to be doing when people expect WoL to
be able to wake their systems up from hibernate.

The more I think about it, the more I'm coming to the conclusion that
the blamed commit is wrong as it is coded today.

It also occurs to me that systems need to cope with the PHY's INT signal
being asserted through boot, because in the case of WoL-triggered resume
from hibernate mode, the PHY's INT signal will be asserted _while_ the
system is booting up (and remember, recovery from hibernate mode is
essentially no different from a normal system boot with the exception
that the kernel notices a special signature in the swap partition, and
then reloads state from there.)

So, boot has to cope properly with the PHY's interrupt being asserted
in order for WoL from hibernate to work.

It seems to me that a revert of the blamed commit is in order, and a
different approach is needed?

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

2023-08-03 20:51:38

by Florian Fainelli

[permalink] [raw]

Subject: Re: [PATCH net-next v2 02/19] net: phy: add a shutdown procedure

On 8/3/23 11:16, Uwe Kleine-König wrote:
> Hello,
>
> this patch became commit e2f016cf775129c050d6c79483073423db15c79a and is
> contained in v5.11-rc1.
>
> It broke wake-on-lan on my NAS (an ARM machine with an Armada 370 SoC,
> armada-370-netgear-rn104.dts). The used phy driver is marvell.c. I only
> report it now as I just upgraded that machine from Debian 11 (with
> kernel 5.10.x) to Debian 12 (with kernel 6.1.x).
>
> Commenting out phy_disable_interrupts(...) in v6.1.41's phy_shutdown()
> fixes the problem for me.
>
> On Sun, Nov 01, 2020 at 02:50:57PM +0200, Ioana Ciornei wrote:
>> In case of a board which uses a shared IRQ we can easily end up with an
>> IRQ storm after a forced reboot.
>>
>> For example, a 'reboot -f' will trigger a call to the .shutdown()
>> callbacks of all devices. Because phylib does not implement that hook,
>> the PHY is not quiesced, thus it can very well leave its IRQ enabled.
>>
>> At the next boot, if that IRQ line is found asserted by the first PHY
>> driver that uses it, but _before_ the driver that is _actually_ keeping
>> the shared IRQ asserted is probed, the IRQ is not going to be
>> acknowledged, thus it will keep being fired preventing the boot process
>> of the kernel to continue. This is even worse when the second PHY driver
>> is a module.
>>
>> To fix this, implement the .shutdown() callback and disable the
>> interrupts if these are used.
>
> I don't know how this should interact with wake-on-lan, but I would
> expect that there is a way to fix this without reintroducing the problem
> fixed by this change. However I cannot say if this needs fixing in the
> generic phy code or the phy driver. Any hints?

It depends upon what the PHY drivers and underlying hardware are capable
and willing to do. Some PHY drivers will shutdown the TX path completely
since you do not need that part to receive Wake-on-LAN packets and pass
them up to the PHY and/or MAC Wake-on-LAN matching logic. This would
invite us to let individual PHY drivers make a decision as to what they
want to do in a .shutdown() routine that would then need to be added to
each and every driver that wants to do something special. In the absence
of said routine, you could default to calling phy_disable_interrupts()
unless the PHY has WoL enabled?

phydev::wol_enabled reflects whether the PHY and/or the MAC has
Wake-on-LAN enabled which could you could key off to "nullify" what the
shutdown does.

>
>> Note that we are still susceptible to IRQ storms if the previous kernel
>> exited with a panic or if the bootloader left the shared IRQ active, but
>> there is absolutely nothing we can do about these cases.
>
> I'd say the bootloader could handle that, knowing that for some machines
> changing the bootloader isn't an option.

There is also the case of the boot loader not touching any
PHY/MAC/networking and just booting as fast as possible to the kernel.
It really becomes a problem that can be distributed against multiple SW
agents to solve it, though clearly the kernel could do it too, so why
not keep it there I guess.
--
Florian