[PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's

[PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's

[Heiner Kallweit]

This series tries to address few problematic aspects raised by
Russell. Concrete example is the Marvell 88x3310, the changes
should be helpful for other complex C45 PHY's too.

v2:
- added patch enabling interrupts also if phylib state machine
  isn't started
- removed patch dealing with the double link status read
  This one needs little bit more thinking and will go separately.

Heiner Kallweit (3):
  net: phy: enable interrupts when PHY is attached already
  net: phy: add callback for custom interrupt handler to struct
    phy_driver
  net: phy: export phy_queue_state_machine

 drivers/net/phy/phy.c        | 53 +++++++++++++++++++++++-------------
 drivers/net/phy/phy_device.c |  2 +-
 include/linux/phy.h          |  6 +++-
 3 files changed, 40 insertions(+), 21 deletions(-)

-- 
2.21.0

[PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already

[Heiner Kallweit]

This patch is a step towards allowing PHY drivers to handle more
interrupt sources than just link change. E.g. several PHY's have
built-in temperature monitoring and can raise an interrupt if a
temperature threshold is exceeded. We may be interested in such
interrupts also if the phylib state machine isn't started.
Therefore move enabling interrupts to phy_request_interrupt().

v2:
- patch added to series

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c        | 36 ++++++++++++++++++++++--------------
 drivers/net/phy/phy_device.c |  2 +-
 include/linux/phy.h          |  1 +
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index e88854292..d90d9863e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -799,10 +799,10 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 }
 
 /**
- * phy_request_interrupt - request interrupt for a PHY device
+ * phy_request_interrupt - request and enable interrupt for a PHY device
  * @phydev: target phy_device struct
  *
- * Description: Request the interrupt for the given PHY.
+ * Description: Request and enable the interrupt for the given PHY.
  *   If this fails, then we set irq to PHY_POLL.
  *   This should only be called with a valid IRQ number.
  */
@@ -817,10 +817,30 @@ void phy_request_interrupt(struct phy_device *phydev)
 		phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
 			    err, phydev->irq);
 		phydev->irq = PHY_POLL;
+	} else {
+		if (phy_enable_interrupts(phydev)) {
+			phydev_warn(phydev, "Can't enable interrupt, falling back to polling\n");
+			phy_free_interrupt(phydev);
+			phydev->irq = PHY_POLL;
+		}
 	}
 }
 EXPORT_SYMBOL(phy_request_interrupt);
 
+/**
+ * phy_free_interrupt - disable and free interrupt for a PHY device
+ * @phydev: target phy_device struct
+ *
+ * Description: Disable and free the interrupt for the given PHY.
+ *   This should only be called with a valid IRQ number.
+ */
+void phy_free_interrupt(struct phy_device *phydev)
+{
+	phy_disable_interrupts(phydev);
+	free_irq(phydev->irq, phydev);
+}
+EXPORT_SYMBOL(phy_free_interrupt);
+
 /**
  * phy_stop - Bring down the PHY link, and stop checking the status
  * @phydev: target phy_device struct
@@ -835,9 +855,6 @@ void phy_stop(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	if (phy_interrupt_is_valid(phydev))
-		phy_disable_interrupts(phydev);
-
 	phydev->state = PHY_HALTED;
 
 	mutex_unlock(&phydev->lock);
@@ -864,8 +881,6 @@ EXPORT_SYMBOL(phy_stop);
  */
 void phy_start(struct phy_device *phydev)
 {
-	int err;
-
 	mutex_lock(&phydev->lock);
 
 	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
@@ -877,13 +892,6 @@ void phy_start(struct phy_device *phydev)
 	/* if phy was suspended, bring the physical link up again */
 	__phy_resume(phydev);
 
-	/* make sure interrupts are enabled for the PHY */
-	if (phy_interrupt_is_valid(phydev)) {
-		err = phy_enable_interrupts(phydev);
-		if (err < 0)
-			goto out;
-	}
-
 	phydev->state = PHY_UP;
 
 	phy_start_machine(phydev);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d288da9a..d71b3ed52 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1013,7 +1013,7 @@ void phy_disconnect(struct phy_device *phydev)
 		phy_stop(phydev);
 
 	if (phy_interrupt_is_valid(phydev))
-		free_irq(phydev->irq, phydev);
+		phy_free_interrupt(phydev);
 
 	phydev->adjust_link = NULL;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7180b1d1e..72e1196f9 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1147,6 +1147,7 @@ int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd);
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
 void phy_request_interrupt(struct phy_device *phydev);
+void phy_free_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
 int phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
-- 
2.21.0

[PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver

[Heiner Kallweit]

The phylib interrupt handler handles link change events only currently.
However PHY drivers may want to use other interrupt sources too,
e.g. to report temperature monitoring events. Therefore add a callback
to struct phy_driver allowing PHY drivers to implement a custom
interrupt handler.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 9 +++++++--
 include/linux/phy.h   | 3 +++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d90d9863e..068f0a126 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
 		return IRQ_NONE;
 
-	/* reschedule state queue work to run as soon as possible */
-	phy_trigger_machine(phydev);
+	if (phydev->drv->handle_interrupt) {
+		if (phydev->drv->handle_interrupt(phydev))
+			goto phy_err;
+	} else {
+		/* reschedule state queue work to run as soon as possible */
+		phy_trigger_machine(phydev);
+	}
 
 	if (phy_clear_interrupt(phydev))
 		goto phy_err;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 72e1196f9..16cd33915 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -537,6 +537,9 @@ struct phy_driver {
 	 */
 	int (*did_interrupt)(struct phy_device *phydev);
 
+	/* Override default interrupt handling */
+	int (*handle_interrupt)(struct phy_device *phydev);
+
 	/* Clears up any memory if needed */
 	void (*remove)(struct phy_device *phydev);
 
-- 
2.21.0

[PATCH net-next v2 3/3] net: phy: export phy_queue_state_machine

[Heiner Kallweit]

We face the issue that link change interrupt and link status may be
reported by different PHY layers. As a result the link change
interrupt may occur before the link status changes.
Export phy_queue_state_machine to allow PHY drivers to specify a
delay between link status change interrupt and link status check.

v2:
- change jiffies parameter type to unsigned long

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy.c | 8 +++++---
 include/linux/phy.h   | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 068f0a126..74dfd424c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -29,6 +29,8 @@
 #include <linux/uaccess.h>
 #include <linux/atomic.h>
 
+#define PHY_STATE_TIME	HZ
+
 #define PHY_STATE_STR(_state)			\
 	case PHY_##_state:			\
 		return __stringify(_state);	\
@@ -478,12 +480,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 }
 EXPORT_SYMBOL(phy_mii_ioctl);
 
-static void phy_queue_state_machine(struct phy_device *phydev,
-				    unsigned int secs)
+void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies)
 {
 	mod_delayed_work(system_power_efficient_wq, &phydev->state_queue,
-			 secs * HZ);
+			 jiffies);
 }
+EXPORT_SYMBOL(phy_queue_state_machine);
 
 static void phy_trigger_machine(struct phy_device *phydev)
 {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 16cd33915..dc4b51060 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -188,7 +188,6 @@ static inline const char *phy_modes(phy_interface_t interface)
 
 
 #define PHY_INIT_TIMEOUT	100000
-#define PHY_STATE_TIME		1
 #define PHY_FORCE_TIMEOUT	10
 
 #define PHY_MAX_ADDR	32
@@ -1140,6 +1139,7 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
 int phy_drivers_register(struct phy_driver *new_driver, int n,
 			 struct module *owner);
 void phy_state_machine(struct work_struct *work);
+void phy_queue_state_machine(struct phy_device *phydev, unsigned long jiffies);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
-- 
2.21.0

Re: [PATCH net-next v2 1/3] net: phy: enable interrupts when PHY is attached already

[Andrew Lunn]

On Thu, May 30, 2019 at 03:09:15PM +0200, Heiner Kallweit wrote:
> This patch is a step towards allowing PHY drivers to handle more
> interrupt sources than just link change. E.g. several PHY's have
> built-in temperature monitoring and can raise an interrupt if a
> temperature threshold is exceeded. We may be interested in such
> interrupts also if the phylib state machine isn't started.
> Therefore move enabling interrupts to phy_request_interrupt().
> 
> v2:
> - patch added to series
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 0/3] net: phy: improve handling of more complex C45 PHY's

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Thu, 30 May 2019 15:08:09 +0200

> This series tries to address few problematic aspects raised by
> Russell. Concrete example is the Marvell 88x3310, the changes
> should be helpful for other complex C45 PHY's too.
> 
> v2:
> - added patch enabling interrupts also if phylib state machine
>   isn't started
> - removed patch dealing with the double link status read
>   This one needs little bit more thinking and will go separately.

Series applied.

Re: [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver

[Michael Walle]

Hi,

this is an old thread and I know its already applied. But I'd like to 
hear your opinion on the following problem below.

> The phylib interrupt handler handles link change events only currently.
> However PHY drivers may want to use other interrupt sources too,
> e.g. to report temperature monitoring events. Therefore add a callback
> to struct phy_driver allowing PHY drivers to implement a custom
> interrupt handler.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phy.c | 9 +++++++--
>  include/linux/phy.h   | 3 +++
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d90d9863e..068f0a126 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void 
> *phy_dat)
>  	if (phydev->drv->did_interrupt && 
> !phydev->drv->did_interrupt(phydev))
>  		return IRQ_NONE;
> 
> -	/* reschedule state queue work to run as soon as possible */
> -	phy_trigger_machine(phydev);
> +	if (phydev->drv->handle_interrupt) {
> +		if (phydev->drv->handle_interrupt(phydev))
> +			goto phy_err;

There are PHYs which clears the interrupt already by reading the 
interrupt status register. To do something useful in handle_interrupt() 
I have to read the interrupt status register, thus clearing the pending 
interrupts.


> +	} else {
> +		/* reschedule state queue work to run as soon as possible */
> +		phy_trigger_machine(phydev);
> +	}
> 
>  	if (phy_clear_interrupt(phydev))
>  		goto phy_err;

But here the interrupts are cleared again, which means we might loose 
interrupt causes in between.

I could think of two different fixes:
  (1) handle_interrupt() has to take care to clear the interrupts and 
skip the phy_clear_interrupt() above.
  (2) handle_interrupt() might return a special return code which skips 
the phy_clear_interrupt

TBH, I'd prefer (1) but I don't know if it is allowed to change 
semantics afterwards. (Also, I've found no driver where 
handle_interrupt() is actually used for now?)

-michael

Re: [PATCH net-next v2 2/3] net: phy: add callback for custom interrupt handler to struct phy_driver

[Russell King - ARM Linux admin]

On Tue, Nov 19, 2019 at 11:33:47AM +0100, Michael Walle wrote:
> 
> Hi,
> 
> this is an old thread and I know its already applied. But I'd like to hear
> your opinion on the following problem below.
> 
> > The phylib interrupt handler handles link change events only currently.
> > However PHY drivers may want to use other interrupt sources too,
> > e.g. to report temperature monitoring events. Therefore add a callback
> > to struct phy_driver allowing PHY drivers to implement a custom
> > interrupt handler.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> > Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/phy/phy.c | 9 +++++++--
> >  include/linux/phy.h   | 3 +++
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index d90d9863e..068f0a126 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -772,8 +772,13 @@ static irqreturn_t phy_interrupt(int irq, void
> > *phy_dat)
> >  	if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
> >  		return IRQ_NONE;
> > 
> > -	/* reschedule state queue work to run as soon as possible */
> > -	phy_trigger_machine(phydev);
> > +	if (phydev->drv->handle_interrupt) {
> > +		if (phydev->drv->handle_interrupt(phydev))
> > +			goto phy_err;
> 
> There are PHYs which clears the interrupt already by reading the interrupt
> status register. To do something useful in handle_interrupt() I have to read
> the interrupt status register, thus clearing the pending interrupts.
> 
> 
> > +	} else {
> > +		/* reschedule state queue work to run as soon as possible */
> > +		phy_trigger_machine(phydev);
> > +	}
> > 
> >  	if (phy_clear_interrupt(phydev))
> >  		goto phy_err;
> 
> But here the interrupts are cleared again, which means we might loose
> interrupt causes in between.
> 
> I could think of two different fixes:
>  (1) handle_interrupt() has to take care to clear the interrupts and skip
> the phy_clear_interrupt() above.
>  (2) handle_interrupt() might return a special return code which skips the
> phy_clear_interrupt
> 
> TBH, I'd prefer (1) but I don't know if it is allowed to change semantics
> afterwards. (Also, I've found no driver where handle_interrupt() is actually
> used for now?)

I made the argument at the time that phylib should stop being a middle-
layer, but instead let PHY drivers take care of interrupt handling
themselves, just like we do elsewhere in the kernel.  I think your
case just shows that trying to keep the interrupt handling structured
inside phylib and trying to make all PHYs fit is just going to be
painful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up