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

2020-10-29 10:11:58

Subject: [PATCH net-next 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 == 0)
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.

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 | 57 ++++++++++----
drivers/net/phy/at803x.c | 42 ++++++++--
drivers/net/phy/bcm-cygnus.c | 2 +-
drivers/net/phy/bcm-phy-lib.c | 37 ++++++++-
drivers/net/phy/bcm-phy-lib.h | 1 +
drivers/net/phy/bcm54140.c | 39 +++++++---
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 | 59 ++++++++++----
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 | 128 +++++++++++++++++++++++++++----
include/linux/phy.h | 3 +
16 files changed, 484 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-10-29 10:11:59

by Ioana Ciornei

[permalink] [raw]

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

2020-10-29 10:13:00

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next 07/19] net: phy: mscc: 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.

Also, remove the .did_interrupt() callback since it's not anymore used.

Signed-off-by: Ioana Ciornei <[email protected]>
---
drivers/net/phy/mscc/mscc_main.c | 56 ++++++++++++++++----------------
1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index b705121c9d26..ae790fd3734c 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1541,16 +1541,6 @@ static int vsc85xx_config_init(struct phy_device *phydev)
return 0;
}

-static int vsc8584_did_interrupt(struct phy_device *phydev)
-{
- int rc = 0;
-
- if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
- rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
-
- return (rc < 0) ? 0 : rc & MII_VSC85XX_INT_MASK_MASK;
-}
-
static int vsc8514_config_pre_init(struct phy_device *phydev)
{
/* These are the settings to override the silicon default
@@ -1948,6 +1938,24 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
return rc;
}

+static irqreturn_t vsc85xx_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status;
+
+ irq_status = phy_read(phydev, MII_VSC85XX_INT_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 vsc85xx_config_aneg(struct phy_device *phydev)
{
int rc;
@@ -2114,7 +2122,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc85xx_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2139,9 +2147,8 @@ static struct phy_driver vsc85xx_driver[] = {
.config_aneg = &vsc85xx_config_aneg,
.aneg_done = &genphy_aneg_done,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8574_probe,
@@ -2163,7 +2170,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc8514_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2187,7 +2194,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc85xx_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2211,7 +2218,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc85xx_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2235,7 +2242,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc85xx_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2259,7 +2266,7 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc85xx_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
@@ -2283,9 +2290,8 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc8584_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8574_probe,
@@ -2308,9 +2314,8 @@ static struct phy_driver vsc85xx_driver[] = {
.config_init = &vsc8584_config_init,
.config_aneg = &vsc85xx_config_aneg,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8584_probe,
@@ -2335,7 +2340,6 @@ static struct phy_driver vsc85xx_driver[] = {
.handle_interrupt = &vsc8584_handle_interrupt,
.ack_interrupt = &vsc85xx_ack_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8574_probe,
@@ -2359,9 +2363,8 @@ static struct phy_driver vsc85xx_driver[] = {
.config_aneg = &vsc85xx_config_aneg,
.aneg_done = &genphy_aneg_done,
.read_status = &vsc85xx_read_status,
- .ack_interrupt = &vsc85xx_ack_interrupt,
+ .handle_interrupt = vsc85xx_handle_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8574_probe,
@@ -2388,7 +2391,6 @@ static struct phy_driver vsc85xx_driver[] = {
.handle_interrupt = &vsc8584_handle_interrupt,
.ack_interrupt = &vsc85xx_ack_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8584_probe,
@@ -2413,7 +2415,6 @@ static struct phy_driver vsc85xx_driver[] = {
.handle_interrupt = &vsc8584_handle_interrupt,
.ack_interrupt = &vsc85xx_ack_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8584_probe,
@@ -2438,7 +2439,6 @@ static struct phy_driver vsc85xx_driver[] = {
.handle_interrupt = &vsc8584_handle_interrupt,
.ack_interrupt = &vsc85xx_ack_interrupt,
.config_intr = &vsc85xx_config_intr,
- .did_interrupt = &vsc8584_did_interrupt,
.suspend = &genphy_suspend,
.resume = &genphy_resume,
.probe = &vsc8584_probe,
--
2.28.0

2020-10-29 10:13:35

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next 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]>
---
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-10-29 10:13:45

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next 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]>
---
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 a32da9ee98ab..8739faf78560 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 f388cacd992a..e71af3aa4878 100644
--- a/drivers/net/phy/bcm54140.c
+++ b/drivers/net/phy/bcm54140.c
@@ -674,7 +674,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;
@@ -683,12 +683,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)
@@ -843,7 +854,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-10-29 10:14:16

by Ioana Ciornei

[permalink] [raw]

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

2020-10-29 10:14:39

by Ioana Ciornei

[permalink] [raw]

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

2020-10-29 10:14:45

by Ioana Ciornei

[permalink] [raw]

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

2020-10-29 10:15:01

by Ioana Ciornei

[permalink] [raw]

Subject: [PATCH net-next 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]>
---
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-10-29 10:15:09

by Ioana Ciornei

[permalink] [raw]

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

2020-10-29 10:15:43

by Ioana Ciornei

[permalink] [raw]

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

2020-10-30 21:58:41

by Heiner Kallweit

[permalink] [raw]

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

On 29.10.2020 11:07, 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.
>
> 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 == 0)
> 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.
>

Looks good to me. The current interrupt support in phylib basically
just covers the link change interrupt and we need more flexibility.

And even in the current limited use case we face smaller issues.
One reason is that INTR_STATUS typically is self-clearing on read.
phylib has to deal with the case that did_interrupt may or may not
have read INTR_STATUS already.

I'd just like to avoid the term "shared interrupt", because it has
a well-defined meaning. Our major concern isn't shared interrupts
but support for multiple interrupt sources (in addition to
link change) in a PHY.

WRT implementing a shutdown hook another use case was mentioned
recently: https://lkml.org/lkml/2020/9/30/451
But that's not really relevant here and just fyi.

> 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.
>
> 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 | 57 ++++++++++----
> drivers/net/phy/at803x.c | 42 ++++++++--
> drivers/net/phy/bcm-cygnus.c | 2 +-
> drivers/net/phy/bcm-phy-lib.c | 37 ++++++++-
> drivers/net/phy/bcm-phy-lib.h | 1 +
> drivers/net/phy/bcm54140.c | 39 +++++++---
> 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 | 59 ++++++++++----
> 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 | 128 +++++++++++++++++++++++++++----
> include/linux/phy.h | 3 +
> 16 files changed, 484 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]>
>

2020-10-30 22:09:13

by Vladimir Oltean

[permalink] [raw]

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

On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> I'd just like to avoid the term "shared interrupt", because it has
> a well-defined meaning. Our major concern isn't shared interrupts
> but support for multiple interrupt sources (in addition to
> link change) in a PHY.

You may be a little bit confused Heiner.
This series adds support for exactly _that_ meaning of shared interrupts.
Shared interrupts (aka wired-OR on the PCB) don't work today with the
PHY library. I have a board that won't even boot to prompt when the
interrupt lines of its 2 PHYs are enabled, that this series fixes.
You might need to take another look through the commit messages I'm afraid.

2020-10-30 22:44:29

by Heiner Kallweit

[permalink] [raw]

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

On 30.10.2020 23:06, Vladimir Oltean wrote:
> On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
>> I'd just like to avoid the term "shared interrupt", because it has
>> a well-defined meaning. Our major concern isn't shared interrupts
>> but support for multiple interrupt sources (in addition to
>> link change) in a PHY.
>
> You may be a little bit confused Heiner.
> This series adds support for exactly _that_ meaning of shared interrupts.
> Shared interrupts (aka wired-OR on the PCB) don't work today with the
> PHY library. I have a board that won't even boot to prompt when the
> interrupt lines of its 2 PHYs are enabled, that this series fixes.
> You might need to take another look through the commit messages I'm afraid.
>
Right, I was focusing on the PTP irq use case.

2020-10-30 22:46:52

by Heiner Kallweit

[permalink] [raw]

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

On 29.10.2020 11:07, 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.
>
> 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 == 0)

Here I have a concern, bits may be set even if the respective interrupt
source isn't enabled. Therefore we may falsely blame a device to have
triggered the interrupt. irq_status should be masked with the actually
enabled irq source bits.

> 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.
>
> 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 | 57 ++++++++++----
> drivers/net/phy/at803x.c | 42 ++++++++--
> drivers/net/phy/bcm-cygnus.c | 2 +-
> drivers/net/phy/bcm-phy-lib.c | 37 ++++++++-
> drivers/net/phy/bcm-phy-lib.h | 1 +
> drivers/net/phy/bcm54140.c | 39 +++++++---
> 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 | 59 ++++++++++----
> 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 | 128 +++++++++++++++++++++++++++----
> include/linux/phy.h | 3 +
> 16 files changed, 484 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]>
>

2020-10-30 22:50:47

by Vladimir Oltean

[permalink] [raw]

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

On Sat, Oct 31, 2020 at 12:06:42AM +0200, Vladimir Oltean wrote:
> On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> > I'd just like to avoid the term "shared interrupt", because it has
> > a well-defined meaning. Our major concern isn't shared interrupts
> > but support for multiple interrupt sources (in addition to
> > link change) in a PHY.
>
> You may be a little bit confused Heiner.
> This series adds support for exactly _that_ meaning of shared interrupts.
> Shared interrupts (aka wired-OR on the PCB) don't work today with the
> PHY library. I have a board that won't even boot to prompt when the
> interrupt lines of its 2 PHYs are enabled, that this series fixes.
> You might need to take another look through the commit messages I'm afraid.

Maybe this diagram will help you visualize better.

time
|
| PHY 1 PHY 2 has pending IRQ
| | (e.g. link up)
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_HANDLED via |
| phy_clear_interrupt() |
| | |
| | |
| v |
| handling of shared IRQ |
| ends here |
| | |
| | v
| | PHY 2 still has pending IRQ
| | because, you know, it wasn't actually
| | serviced
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_HANDLED via |
| phy_clear_interrupt() |
| | |
| | |
| v |
| handling of shared IRQ |
| ends here |
| | PHY 2: Hey! It's me! Over here!
| | |
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_HANDLED via |
| phy_clear_interrupt() |
| | |
| | |
| v |
| handling of shared IRQ |
| ends here |
| | PHY 2: Srsly?
| | |
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| ... ...
|
| 21 seconds later
|
| RCU stall
v

This happens because today, the way phy_interrupt() is written, you can
only return IRQ_NONE and give the other driver a chance _if_ your driver
implements .did_interrupt(). But the kernel documentation of
.did_interrupt() only recommends to implement that function if you are a
multi-PHY package driver (otherwise stated, the hardware chip has an
embedded shared IRQ). But as things stand, _everybody_ should implement
.did_interrupt() in order for any combination of PHY drivers to support
shared IRQs.

What Ioana is proposing, and this is something that I fully agree with,
is that we just get rid of the layering where the PHY library tries to
be helpful but instead invites everybody to write systematically bad
code. Anyone knows how to write an IRQ handler with eyes closed, but the
fact that .did_interrupt() is mandatory for proper shared IRQ support is
not obvious to everybody, it seems. So let's just have a dedicated IRQ
handling function per each PHY driver, so that we don't get confused in
this sloppy mess of return values, and the code can actually be
followed.

Even _with_ Ioana's changes, there is one more textbook case of shared
interrupts causing trouble, and that is actually the reason why nobody
likes them except hardware engineers who don't get to deal with this.

time
|
| PHY 1 probed
| (module or built-in)
| | PHY 2 has pending IRQ
| | (it had link up from previous
| v boot, or from bootloader, etc)
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_NONE as |
| it should v
| | PHY 2 still has pending IRQ
| | but its handler wasn't called
| | because its driver has not been
| | yet loaded
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_NONE as |
| it should v
| | PHY 2: Not again :(
| | |
| v |
| phy_interrupt() |
| called for PHY 1 |
| | |
| v |
| returns IRQ_NONE as |
| it should |
| | |
| ... ...
| | |
| | PHY 2 driver never gets probed
| | either because it's a module or
| | because the system is too busy
| | checking PHY 1 over and over
| | again for an interrupt that
| | it did not trigger
| | |
| ... ...
|
| 21 seconds later
|
| RCU stall
v

The way that it's solved is that it's never 100% solved.
This one you can just avoid, but never recover from it.
To avoid it, you must ensure from your previous boot environments
(bootloader, kexec) that the IRQ line is not pending. Because if you
leave the shared IRQ line pending, the system is kaput if it happens to
probe the drivers in the wrong order (aka don't probe first the driver
that will clear that shared IRQ). It's like Minesweeper, only worse.

That's why the shutdown hook is there, as a best-effort attempt for
Linux to clean up after itself. But we're always at the mercy of the
bootloader, or even at the mercy of chance. If the previous kernel
panicked, there's no orderly cleanup to speak of.

Hope it's clearer now.

2020-10-30 23:38:18

by Andrew Lunn

[permalink] [raw]

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

> > - 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 == 0)
>
> Here I have a concern, bits may be set even if the respective interrupt
> source isn't enabled. Therefore we may falsely blame a device to have
> triggered the interrupt. irq_status should be masked with the actually
> enabled irq source bits.

Hi Heiner

I would say that is a driver implementation detail, for each driver to
handle how it needs to handle it. I've seen some hardware where the
interrupt status is already masked with the interrupt enabled
bits. I've soon other hardware where it is not.

For example code, what is listed above is O.K. The real implementation
in a driver need knowledge of the hardware.

Andrew

2020-10-31 05:24:54

by Ioana Ciornei

[permalink] [raw]

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

On Sat, Oct 31, 2020 at 12:36:27AM +0100, Andrew Lunn wrote:
> > > - 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 == 0)
> >
> > Here I have a concern, bits may be set even if the respective interrupt
> > source isn't enabled. Therefore we may falsely blame a device to have
> > triggered the interrupt. irq_status should be masked with the actually
> > enabled irq source bits.
>
> Hi Heiner
>
> I would say that is a driver implementation detail, for each driver to
> handle how it needs to handle it. I've seen some hardware where the
> interrupt status is already masked with the interrupt enabled
> bits. I've soon other hardware where it is not.
>
> For example code, what is listed above is O.K. The real implementation
> in a driver need knowledge of the hardware.
>

Hi,

As Andrew said, that is just an example code that could work for some
devices but should be extended depending on how the actual PHY is
working.

For example, the VSC8584 will still be trigerring the link state machine
just on the link change interrupt, I am not changing this:

static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev)
{
irqreturn_t ret;
int irq_status;

irq_status = phy_read(phydev, MII_VSC85XX_INT_STATUS);
if (irq_status < 0)
return IRQ_NONE;

/* Timestamping IRQ does not set a bit in the global INT_STATUS, so
* irq_status would be 0.
*/
ret = vsc8584_handle_ts_interrupt(phydev);
if (!(irq_status & MII_VSC85XX_INT_MASK_MASK))
return ret;

if (irq_status & MII_VSC85XX_INT_MASK_EXT)
vsc8584_handle_macsec_interrupt(phydev);

if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG)
phy_trigger_machine(phydev);

return IRQ_HANDLED;
}

Ioana

2020-10-31 05:29:03

by Ioana Ciornei

[permalink] [raw]

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

On Fri, Oct 30, 2020 at 10:56:24PM +0100, Heiner Kallweit wrote:
> On 29.10.2020 11:07, 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.
> >
> > 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 == 0)
> > 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.
> >
>
> Looks good to me. The current interrupt support in phylib basically
> just covers the link change interrupt and we need more flexibility.
>
> And even in the current limited use case we face smaller issues.
> One reason is that INTR_STATUS typically is self-clearing on read.
> phylib has to deal with the case that did_interrupt may or may not
> have read INTR_STATUS already.
>
> I'd just like to avoid the term "shared interrupt", because it has
> a well-defined meaning. Our major concern isn't shared interrupts
> but support for multiple interrupt sources (in addition to
> link change) in a PHY.
>

I am not going to address this part, Vladimir did a good job in the
following emails describing exactly the problem that I am trying to fix
- shared interrupts even between PHYs which are not in the same package
or even the same type of device.

> WRT implementing a shutdown hook another use case was mentioned
> recently: https://lkml.org/lkml/2020/9/30/451
> But that's not really relevant here and just fyi.
>

I missed this thread. Thanks for the link!

Ioana

2020-10-31 10:20:07

by Heiner Kallweit

[permalink] [raw]

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

On 31.10.2020 00:36, Andrew Lunn wrote:
>>> - 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 == 0)
>>
>> Here I have a concern, bits may be set even if the respective interrupt
>> source isn't enabled. Therefore we may falsely blame a device to have
>> triggered the interrupt. irq_status should be masked with the actually
>> enabled irq source bits.
>
> Hi Heiner
>
Hi Andrew,

> I would say that is a driver implementation detail, for each driver to
> handle how it needs to handle it. I've seen some hardware where the
> interrupt status is already masked with the interrupt enabled
> bits. I've soon other hardware where it is not.
>
Sure, I just wanted to add the comment before others simply copy and
paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
it is used as is. And IIRC at least the Aquantia PHY doesn't mask
the interrupt status.

> For example code, what is listed above is O.K. The real implementation
> in a driver need knowledge of the hardware.
>
> Andrew
> .
>
Heiner

2020-10-31 10:59:15

by Ioana Ciornei

[permalink] [raw]

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

On Sat, Oct 31, 2020 at 11:18:18AM +0100, Heiner Kallweit wrote:
> On 31.10.2020 00:36, Andrew Lunn wrote:
> >>> - 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 == 0)
> >>
> >> Here I have a concern, bits may be set even if the respective interrupt
> >> source isn't enabled. Therefore we may falsely blame a device to have
> >> triggered the interrupt. irq_status should be masked with the actually
> >> enabled irq source bits.
> >
> > Hi Heiner
> >
> Hi Andrew,
>
> > I would say that is a driver implementation detail, for each driver to
> > handle how it needs to handle it. I've seen some hardware where the
> > interrupt status is already masked with the interrupt enabled
> > bits. I've soon other hardware where it is not.
> >
> Sure, I just wanted to add the comment before others simply copy and
> paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> the interrupt status.
>

Hi Heiner,

If I understand correctly what you are suggesting, the
.handle_interrupt() for the aquantia would look like this:

static irqreturn_t aqr_handle_interrupt(struct phy_device *phydev)
{
int irq_status;

irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
if (irq_status < 0) {
phy_error(phydev);
return IRQ_NONE;
}

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

phy_trigger_machine(phydev);

return IRQ_HANDLED;
}

So only return IRQ_HANDLED when one of the bits from INT_STATUS
corresponding with the enabled interrupts are set, not if any other link
status bit is set.

Ok, I'll send a v2 with these kind of changes.

Ioana

2020-10-31 14:34:32

by Andrew Lunn

[permalink] [raw]

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

> Sure, I just wanted to add the comment before others simply copy and
> paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> the interrupt status.

And that is were we are going to have issues with this patch set, and
need review by individual PHY driver maintainers, or a good look at
the datasheet.

Andrew

2020-10-31 14:53:08

by Ioana Ciornei

[permalink] [raw]

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

On Sat, Oct 31, 2020 at 03:32:15PM +0100, Andrew Lunn wrote:
> > Sure, I just wanted to add the comment before others simply copy and
> > paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
> > it is used as is. And IIRC at least the Aquantia PHY doesn't mask
> > the interrupt status.
>
> And that is were we are going to have issues with this patch set, and
> need review by individual PHY driver maintainers, or a good look at
> the datasheet.
>

Yep, I already started to comb through all the drivers and their
datasheets so that I can only check for the interrupts that the driver
enables.

Ioana