In case phydev->irq is modified unconditionally to a valid IRQ, handling
the IRQ may lead to a nullptr exception if no interrupt handler is
registered to the phy driver. phy_interrupt calls a
phy_device->handle_interrupt unconditionally. And interrupts are enabled
in phy_connect_direct if phydev->irq is not equal to PHY_POLL or
PHY_MAC_INTERRUPT, so it does not check for a phy driver providing an ISR.
Adding an additonal check for a valid interrupt handler in phy_attach_direct
function, and falling back to polling mode if not, should prevent for
such nullptr exceptions.
Moreover, the ADIN1100 phy driver is extended with an interrupt handler
for changes in the link status.
Andre Werner (2):
net: phy: phy_device: Prevent nullptr exceptions on ISR
net: phy: adin1100: Add interrupt support for link change
drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++
drivers/net/phy/phy_device.c | 13 +++++----
2 files changed, 64 insertions(+), 5 deletions(-)
--
2.43.0
An interrupt handler was added to the driver as well as functions
to enable interrupts at the phy.
There are several interrupts maskable at the phy, but only link change
interrupts are handled by the driver yet.
Signed-off-by: Andre Werner <[email protected]>
---
v4:
- Change read-modify-write behavior as suggested to phy_modify_mmd.
---
drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
1 file changed, 56 insertions(+)
diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
index 7619d6185801..7c82384e5d30 100644
--- a/drivers/net/phy/adin1100.c
+++ b/drivers/net/phy/adin1100.c
@@ -18,6 +18,12 @@
#define PHY_ID_ADIN1110 0x0283bc91
#define PHY_ID_ADIN2111 0x0283bca1
+#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021
+#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1)
+
+#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011
+#define ADIN_LINK_STAT_CHNG BIT(1)
+
#define ADIN_FORCED_MODE 0x8000
#define ADIN_FORCED_MODE_EN BIT(0)
@@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev)
return genphy_c45_config_aneg(phydev);
}
+static int adin_phy_ack_intr(struct phy_device *phydev)
+{
+ /* Clear pending interrupts */
+ int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ ADIN_PHY_SUBSYS_IRQ_STATUS);
+
+ return rc < 0 ? rc : 0;
+}
+
+static int adin_config_intr(struct phy_device *phydev)
+{
+ int ret;
+ u16 irq_mask;
+
+ ret = adin_phy_ack_intr(phydev);
+
+ if (ret)
+ return ret;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
+ else
+ irq_mask = 0;
+
+ return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
+ ADIN_PHY_SUBSYS_IRQ_MASK,
+ ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
+}
+
+static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev)
+{
+ int irq_status;
+
+ irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2,
+ ADIN_PHY_SUBSYS_IRQ_STATUS);
+ if (irq_status < 0) {
+ phy_error(phydev);
+ return IRQ_NONE;
+ }
+
+ if (!(irq_status & ADIN_LINK_STAT_CHNG))
+ return IRQ_NONE;
+
+ phy_trigger_machine(phydev);
+
+ return IRQ_HANDLED;
+}
+
static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
{
int ret;
@@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = {
.probe = adin_probe,
.config_aneg = adin_config_aneg,
.read_status = adin_read_status,
+ .config_intr = adin_config_intr,
+ .handle_interrupt = adin_phy_handle_interrupt,
.set_loopback = adin_set_loopback,
.suspend = adin_suspend,
.resume = adin_resume,
--
2.43.0
If phydev->irq is set unconditionally, check
for valid interrupt handler or fall back to polling mode to prevent
nullptr exceptions in interrupt service routine.
Signed-off-by: Andre Werner <[email protected]>
---
v4:
- Delete phydev_warn and comment as suggested.
---
drivers/net/phy/phy_device.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..7cbe7c714177 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1413,6 +1413,11 @@ int phy_sfp_probe(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_sfp_probe);
+static bool phy_drv_supports_irq(struct phy_driver *phydrv)
+{
+ return phydrv->config_intr && phydrv->handle_interrupt;
+}
+
/**
* phy_attach_direct - attach a network device to a given PHY device pointer
* @dev: network device to attach
@@ -1527,6 +1532,9 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
if (phydev->dev_flags & PHY_F_NO_IRQ)
phydev->irq = PHY_POLL;
+ if (!phy_drv_supports_irq(phydev->drv) && phy_interrupt_is_valid(phydev))
+ phydev->irq = PHY_POLL;
+
/* Port is set to PORT_TP by default and the actual PHY driver will set
* it to different value depending on the PHY configuration. If we have
* the generic PHY driver we can't figure it out, thus set the old
@@ -2992,11 +3000,6 @@ s32 phy_get_internal_delay(struct phy_device *phydev, struct device *dev,
}
EXPORT_SYMBOL(phy_get_internal_delay);
-static bool phy_drv_supports_irq(struct phy_driver *phydrv)
-{
- return phydrv->config_intr && phydrv->handle_interrupt;
-}
-
static int phy_led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
--
2.43.0
On Sun, Jan 21, 2024 at 08:54:48PM +0100, Andre Werner wrote:
> If phydev->irq is set unconditionally, check
> for valid interrupt handler or fall back to polling mode to prevent
> nullptr exceptions in interrupt service routine.
>
> Signed-off-by: Andre Werner <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
The 01/21/2024 20:54, Andre Werner wrote:
Hi Andre,
> An interrupt handler was added to the driver as well as functions
> to enable interrupts at the phy.
>
> There are several interrupts maskable at the phy, but only link change
> interrupts are handled by the driver yet.
>
> Signed-off-by: Andre Werner <[email protected]>
> ---
> v4:
> - Change read-modify-write behavior as suggested to phy_modify_mmd.
Usually it is good to keep the change log also from the previous
versions, so it is easier to see what has been changed.
> ---
> drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 56 insertions(+)
>
> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
> index 7619d6185801..7c82384e5d30 100644
> --- a/drivers/net/phy/adin1100.c
> +++ b/drivers/net/phy/adin1100.c
> @@ -18,6 +18,12 @@
> #define PHY_ID_ADIN1110 0x0283bc91
> #define PHY_ID_ADIN2111 0x0283bca1
>
> +#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021
> +#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1)
> +
> +#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011
> +#define ADIN_LINK_STAT_CHNG BIT(1)
> +
> #define ADIN_FORCED_MODE 0x8000
> #define ADIN_FORCED_MODE_EN BIT(0)
>
> @@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev)
> return genphy_c45_config_aneg(phydev);
> }
>
> +static int adin_phy_ack_intr(struct phy_device *phydev)
> +{
> + /* Clear pending interrupts */
> + int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> + ADIN_PHY_SUBSYS_IRQ_STATUS);
> +
> + return rc < 0 ? rc : 0;
> +}
> +
> +static int adin_config_intr(struct phy_device *phydev)
> +{
> + int ret;
> + u16 irq_mask;
Please use reverse x-mas notation here.
> +
> + ret = adin_phy_ack_intr(phydev);
> +
No new line here, between ret and if.
> + if (ret)
> + return ret;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> + irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
> + else
> + irq_mask = 0;
> +
> + return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
> + ADIN_PHY_SUBSYS_IRQ_MASK,
> + ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
> +}
> +
> +static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev)
> +{
> + int irq_status;
> +
> + irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2,
> + ADIN_PHY_SUBSYS_IRQ_STATUS);
> + if (irq_status < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + if (!(irq_status & ADIN_LINK_STAT_CHNG))
> + return IRQ_NONE;
> +
> + phy_trigger_machine(phydev);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
> {
> int ret;
> @@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = {
> .probe = adin_probe,
> .config_aneg = adin_config_aneg,
> .read_status = adin_read_status,
> + .config_intr = adin_config_intr,
> + .handle_interrupt = adin_phy_handle_interrupt,
> .set_loopback = adin_set_loopback,
> .suspend = adin_suspend,
> .resume = adin_resume,
> --
> 2.43.0
>
>
--
/Horatiu
Dear Maintainers,
I'm a bit confused about the patch submitting process for net-next.
Do I need to send the patchset again, if the merge window is opened again and
the patchset was previously submitted as RFC?
Best regards.
Andre
On Mon, 22 Jan 2024, Horatiu Vultur wrote:
> The 01/21/2024 20:54, Andre Werner wrote:
>
> Hi Andre,
>
>
>> An interrupt handler was added to the driver as well as functions
>> to enable interrupts at the phy.
>>
>> There are several interrupts maskable at the phy, but only link change
>> interrupts are handled by the driver yet.
>>
>> Signed-off-by: Andre Werner <[email protected]>
>> ---
>> v4:
>> - Change read-modify-write behavior as suggested to phy_modify_mmd.
>
> Usually it is good to keep the change log also from the previous
> versions, so it is easier to see what has been changed.
>
>> ---
>> drivers/net/phy/adin1100.c | 56 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/net/phy/adin1100.c b/drivers/net/phy/adin1100.c
>> index 7619d6185801..7c82384e5d30 100644
>> --- a/drivers/net/phy/adin1100.c
>> +++ b/drivers/net/phy/adin1100.c
>> @@ -18,6 +18,12 @@
>> #define PHY_ID_ADIN1110 0x0283bc91
>> #define PHY_ID_ADIN2111 0x0283bca1
>>
>> +#define ADIN_PHY_SUBSYS_IRQ_MASK 0x0021
>> +#define ADIN_LINK_STAT_CHNG_IRQ_EN BIT(1)
>> +
>> +#define ADIN_PHY_SUBSYS_IRQ_STATUS 0x0011
>> +#define ADIN_LINK_STAT_CHNG BIT(1)
>> +
>> #define ADIN_FORCED_MODE 0x8000
>> #define ADIN_FORCED_MODE_EN BIT(0)
>>
>> @@ -136,6 +142,54 @@ static int adin_config_aneg(struct phy_device *phydev)
>> return genphy_c45_config_aneg(phydev);
>> }
>>
>> +static int adin_phy_ack_intr(struct phy_device *phydev)
>> +{
>> + /* Clear pending interrupts */
>> + int rc = phy_read_mmd(phydev, MDIO_MMD_VEND2,
>> + ADIN_PHY_SUBSYS_IRQ_STATUS);
>> +
>> + return rc < 0 ? rc : 0;
>> +}
>> +
>> +static int adin_config_intr(struct phy_device *phydev)
>> +{
>> + int ret;
>> + u16 irq_mask;
>
> Please use reverse x-mas notation here.
>
>> +
>> + ret = adin_phy_ack_intr(phydev);
>> +
>
> No new line here, between ret and if.
>
>> + if (ret)
>> + return ret;
>> +
>> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> + irq_mask = ADIN_LINK_STAT_CHNG_IRQ_EN;
>> + else
>> + irq_mask = 0;
>> +
>> + return phy_modify_mmd(phydev, MDIO_MMD_VEND2,
>> + ADIN_PHY_SUBSYS_IRQ_MASK,
>> + ADIN_LINK_STAT_CHNG_IRQ_EN, irq_mask);
>> +}
>> +
>> +static irqreturn_t adin_phy_handle_interrupt(struct phy_device *phydev)
>> +{
>> + int irq_status;
>> +
>> + irq_status = phy_read_mmd(phydev, MDIO_MMD_VEND2,
>> + ADIN_PHY_SUBSYS_IRQ_STATUS);
>> + if (irq_status < 0) {
>> + phy_error(phydev);
>> + return IRQ_NONE;
>> + }
>> +
>> + if (!(irq_status & ADIN_LINK_STAT_CHNG))
>> + return IRQ_NONE;
>> +
>> + phy_trigger_machine(phydev);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> static int adin_set_powerdown_mode(struct phy_device *phydev, bool en)
>> {
>> int ret;
>> @@ -275,6 +329,8 @@ static struct phy_driver adin_driver[] = {
>> .probe = adin_probe,
>> .config_aneg = adin_config_aneg,
>> .read_status = adin_read_status,
>> + .config_intr = adin_config_intr,
>> + .handle_interrupt = adin_phy_handle_interrupt,
>> .set_loopback = adin_set_loopback,
>> .suspend = adin_suspend,
>> .resume = adin_resume,
>> --
>> 2.43.0
>>
>>
>
> --
> /Horatiu
>
On Sun, Jan 21, 2024 at 08:54:48PM +0100, Andre Werner wrote:
> If phydev->irq is set unconditionally, check
> for valid interrupt handler or fall back to polling mode to prevent
> nullptr exceptions in interrupt service routine.
>
> Signed-off-by: Andre Werner <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Mon, Jan 29, 2024 at 01:51:11PM +0100, Andre Werner wrote:
> Dear Maintainers,
>
> I'm a bit confused about the patch submitting process for net-next.
> Do I need to send the patchset again, if the merge window is opened again and
> the patchset was previously submitted as RFC?
RFC patches should in theory never be merged. So that alone is
sufficient to require a resend. Anything sent during the merge window
is also not eligible for merging.
So please rebase on net-next, collect and add any Acked-by:
Reviewed-by: etc and resend.
Andrew