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

[PATCH net-next 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.

Heiner Kallweit (3):
  net: phy: export phy_queue_state_machine
  net: phy: add callback for custom interrupt handler to struct
    phy_driver
  net: phy: move handling latched link-down to phylib state machine

 drivers/net/phy/phy-c45.c    | 12 ------------
 drivers/net/phy/phy.c        | 28 +++++++++++++++++++++++-----
 drivers/net/phy/phy_device.c | 14 +-------------
 include/linux/phy.h          | 13 ++++++++++++-
 4 files changed, 36 insertions(+), 31 deletions(-)

-- 
2.21.0

[PATCH net-next 1/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 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.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King - ARM Linux admin <linux@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 e88854292..20955836c 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 int 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 7180b1d1e..b133d59f3 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
@@ -1137,6 +1136,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 int 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

[PATCH net-next 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>
---
 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 20955836c..8030d0a97 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -774,8 +774,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 b133d59f3..f90158c67 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -536,6 +536,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 3/3] net: phy: move handling latched link-down to phylib state machine

[Heiner Kallweit]

Especially with fibre links there may be very short link drops. And if
interrupt handling is slow we may miss such a link drop. To deal with
this we remove the double link status read from the generic link status
read functions, and call the state machine twice instead.
The flag for double-reading link status can be set by phy_mac_interrupt
from hard irq context, therefore we have to use an atomic operation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>
---
 drivers/net/phy/phy-c45.c    | 12 ------------
 drivers/net/phy/phy.c        | 11 +++++++++++
 drivers/net/phy/phy_device.c | 14 +-------------
 include/linux/phy.h          |  8 ++++++++
 4 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index b9d414578..63d9e8483 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -223,18 +223,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
 		devad = __ffs(mmd_mask);
 		mmd_mask &= ~BIT(devad);
 
-		/* The link state is latched low so that momentary link
-		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
-		 */
-		if (!phy_polling_mode(phydev)) {
-			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
-			if (val < 0)
-				return val;
-			else if (val & MDIO_STAT1_LSTATUS)
-				continue;
-		}
-
 		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8030d0a97..575412ff5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -778,6 +778,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 		if (phydev->drv->handle_interrupt(phydev))
 			goto phy_err;
 	} else {
+		set_bit(PHY_LINK_DOUBLE_READ, &phydev->atomic_flags);
 		/* reschedule state queue work to run as soon as possible */
 		phy_trigger_machine(phydev);
 	}
@@ -969,6 +970,15 @@ void phy_state_machine(struct work_struct *work)
 			phydev->drv->link_change_notify(phydev);
 	}
 
+	/* link-down is latched, in order not to lose a link-up event we have
+	 * to read the link status twice
+	 */
+	if (test_and_clear_bit(PHY_LINK_DOUBLE_READ, &phydev->atomic_flags)) {
+		if (!phydev->link)
+			phy_trigger_machine(phydev);
+		return;
+	}
+
 	/* Only re-schedule a PHY state machine change if we are polling the
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
 	 * between states from phy_mac_interrupt().
@@ -992,6 +1002,7 @@ void phy_state_machine(struct work_struct *work)
  */
 void phy_mac_interrupt(struct phy_device *phydev)
 {
+	set_bit(PHY_LINK_DOUBLE_READ, &phydev->atomic_flags);
 	/* Trigger a state machine change */
 	phy_trigger_machine(phydev);
 }
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index dcc93a873..f2a78baa8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1704,23 +1704,11 @@ int genphy_update_link(struct phy_device *phydev)
 {
 	int status;
 
-	/* The link state is latched low so that momentary link
-	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
-	 */
-	if (!phy_polling_mode(phydev)) {
-		status = phy_read(phydev, MII_BMSR);
-		if (status < 0)
-			return status;
-		else if (status & BMSR_LSTATUS)
-			goto done;
-	}
-
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
 	if (status < 0)
 		return status;
-done:
+
 	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
 	phydev->autoneg_complete = status & BMSR_ANEGCOMPLETE ? 1 : 0;
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f90158c67..1d1dcfa90 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -332,6 +332,10 @@ struct phy_c45_device_ids {
 	u32 device_ids[8];
 };
 
+enum phy_atomic_flags {
+	PHY_LINK_DOUBLE_READ,
+};
+
 /* phy_device: An instance of a PHY
  *
  * drv: Pointer to the driver for this PHY instance
@@ -346,6 +350,7 @@ struct phy_c45_device_ids {
  * sysfs_links: Internal boolean tracking sysfs symbolic links setup/removal.
  * loopback_enabled: Set true if this phy has been loopbacked successfully.
  * state: state of the PHY for management purposes
+ * atomic_flags: flags accessed in atomic context
  * dev_flags: Device-specific flags used by the PHY driver.
  * link_timeout: The number of timer firings to wait before the
  * giving up on the current attempt at acquiring a link
@@ -394,6 +399,9 @@ struct phy_device {
 
 	enum phy_state state;
 
+	/* flags used in atomic context */
+	unsigned long atomic_flags;
+
 	u32 dev_flags;
 
 	phy_interface_t interface;
-- 
2.21.0

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

[Florian Fainelli]

On 5/27/2019 11:28 AM, Heiner Kallweit wrote:
> We face the issue that link change interrupt and link status may be
> reported by different 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.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Russell King - ARM Linux admin <linux@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 e88854292..20955836c 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 int jiffies)

mod_delayed_work() takes an unsigned long delay argument, so I would
replicate that here as well. Other than that, making PHY_STATE_TIME
local to phy.c is definitively a good idea.

>  {
>  	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 7180b1d1e..b133d59f3 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
> @@ -1137,6 +1136,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 int 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);
> 

-- 
Florian

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

[Florian Fainelli]

On 5/27/2019 11:28 AM, Heiner Kallweit wrote:
> 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>
> ---
>  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 20955836c..8030d0a97 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -774,8 +774,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))

If Russell is okay with such a model where the PHY state machine still
manages the interrupts at large, and only calls a specific callback for
specific even handling, that's fine. We might have to allow PHY drivers
to let them specify what they want to get passed to
request_threaded_irq(), or leave it to them to do it.
-- 
Florian

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

[Heiner Kallweit]

On 27.05.2019 21:25, Florian Fainelli wrote:
> 
> 
> On 5/27/2019 11:28 AM, Heiner Kallweit wrote:
>> 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>
>> ---
>>  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 20955836c..8030d0a97 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -774,8 +774,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))
> 
> If Russell is okay with such a model where the PHY state machine still
> manages the interrupts at large, and only calls a specific callback for
> specific even handling, that's fine. We might have to allow PHY drivers
> to let them specify what they want to get passed to
> request_threaded_irq(), or leave it to them to do it.
> 
This proposed easy model should be able to cover quite some use cases.
One constraint may be that interrupts are disabled if phylib state
machine isn't in a started state. Means most likely it's not able to
cover e.g. the requirement to allow temperature warning interrupts if
PHY is in state HALTED.

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

[Russell King - ARM Linux admin]

On Mon, May 27, 2019 at 08:28:04PM +0200, Heiner Kallweit wrote:
> We face the issue that link change interrupt and link status may be
> reported by different layers. As a result the link change interrupt

I'd describe this as "different PHY layers" to make it clear that we're
talking about the different blocks (eg, PMA vs PCS) in the PHY.

> 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.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>

It should be noted that on its own, this change isn't useful, as
there's no way for a driver to use it without the following patch.

Other than that...

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

> ---
>  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 e88854292..20955836c 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 int 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 7180b1d1e..b133d59f3 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
> @@ -1137,6 +1136,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 int 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
> 
> 
> 

-- 
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

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

[Russell King - ARM Linux admin]

On Mon, May 27, 2019 at 08:28:48PM +0200, Heiner Kallweit wrote:
> 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.

Looks fine to me.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Suggested-by: Russell King - ARM Linux admin <linux@armlinux.org.uk>

Suggested-by: Russell King <rmk+kernel@armlinux.org.uk>
Acked-by: Russell King <rmk+kernel@armlinux.org.uk>

Thanks.

> ---
>  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 20955836c..8030d0a97 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -774,8 +774,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 b133d59f3..f90158c67 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -536,6 +536,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
> 
> 
> 

-- 
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

Re: [PATCH net-next 3/3] net: phy: move handling latched link-down to phylib state machine

[Russell King - ARM Linux admin]

On Mon, May 27, 2019 at 08:29:45PM +0200, Heiner Kallweit wrote:
> Especially with fibre links there may be very short link drops. And if
> interrupt handling is slow we may miss such a link drop. To deal with
> this we remove the double link status read from the generic link status
> read functions, and call the state machine twice instead.
> The flag for double-reading link status can be set by phy_mac_interrupt
> from hard irq context, therefore we have to use an atomic operation.

I came up with a different solution to this - I haven't extensively
tested it yet though:

 drivers/net/phy/phy-c45.c    | 12 ------------
 drivers/net/phy/phy.c        | 29 +++++++++++++++++++----------
 drivers/net/phy/phy_device.c | 14 --------------
 3 files changed, 19 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 9e24d9569424..756d7711cbc5 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -222,18 +222,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
 		devad = __ffs(mmd_mask);
 		mmd_mask &= ~BIT(devad);
 
-		/* The link state is latched low so that momentary link
-		 * drops can be detected. Do not double-read the status
-		 * in polling mode to detect such short link drops.
-		 */
-		if (!phy_polling_mode(phydev)) {
-			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
-			if (val < 0)
-				return val;
-			else if (val & MDIO_STAT1_LSTATUS)
-				continue;
-		}
-
 		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7b3c5eec0129..2e7f0428e8fa 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -507,20 +507,29 @@ static int phy_config_aneg(struct phy_device *phydev)
  */
 static int phy_check_link_status(struct phy_device *phydev)
 {
-	int err;
+	int err, i;
 
 	WARN_ON(!mutex_is_locked(&phydev->lock));
 
-	err = phy_read_status(phydev);
-	if (err)
-		return err;
+	/* The link state is latched low so that momentary link drops can
+	 * be detected. If the link has failed, re-read the link status
+	 * to ensure that we are up to date with the current link state,
+	 * while notifying that the link status has changed.
+	 */
+	for (i = 0; i < 2; i++) {
+		err = phy_read_status(phydev);
+		if (err)
+			return err;
 
-	if (phydev->link && phydev->state != PHY_RUNNING) {
-		phydev->state = PHY_RUNNING;
-		phy_link_up(phydev);
-	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
-		phydev->state = PHY_NOLINK;
-		phy_link_down(phydev, true);
+		if (phydev->link && phydev->state != PHY_RUNNING) {
+			phydev->state = PHY_RUNNING;
+			phy_link_up(phydev);
+		} else if (!phydev->link && phydev->state != PHY_NOLINK) {
+			phydev->state = PHY_NOLINK;
+			phy_link_down(phydev, true);
+		}
+		if (phydev->link)
+			break;
 	}
 
 	return 0;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 77068c545de0..ccc292c0f585 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1704,20 +1704,6 @@ int genphy_update_link(struct phy_device *phydev)
 {
 	int status;
 
-	/* The link state is latched low so that momentary link
-	 * drops can be detected. Do not double-read the status
-	 * in polling mode to detect such short link drops.
-	 */
-	if (!phy_polling_mode(phydev)) {
-		status = phy_read(phydev, MII_BMSR);
-		if (status < 0) {
-			return status;
-		} else if (status & BMSR_LSTATUS) {
-			phydev->link = 1;
-			return 0;
-		}
-	}
-
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
 	if (status < 0)
-- 
2.7.4


-- 
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

Re: [PATCH net-next 3/3] net: phy: move handling latched link-down to phylib state machine

[Heiner Kallweit]

On 28.05.2019 15:15, Russell King - ARM Linux admin wrote:
> On Mon, May 27, 2019 at 08:29:45PM +0200, Heiner Kallweit wrote:
>> Especially with fibre links there may be very short link drops. And if
>> interrupt handling is slow we may miss such a link drop. To deal with
>> this we remove the double link status read from the generic link status
>> read functions, and call the state machine twice instead.
>> The flag for double-reading link status can be set by phy_mac_interrupt
>> from hard irq context, therefore we have to use an atomic operation.
> 
> I came up with a different solution to this - I haven't extensively
> tested it yet though:
> 
>  drivers/net/phy/phy-c45.c    | 12 ------------
>  drivers/net/phy/phy.c        | 29 +++++++++++++++++++----------
>  drivers/net/phy/phy_device.c | 14 --------------
>  3 files changed, 19 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 9e24d9569424..756d7711cbc5 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -222,18 +222,6 @@ int genphy_c45_read_link(struct phy_device *phydev)
>  		devad = __ffs(mmd_mask);
>  		mmd_mask &= ~BIT(devad);
>  
> -		/* The link state is latched low so that momentary link
> -		 * drops can be detected. Do not double-read the status
> -		 * in polling mode to detect such short link drops.
> -		 */
> -		if (!phy_polling_mode(phydev)) {
> -			val = phy_read_mmd(phydev, devad, MDIO_STAT1);
> -			if (val < 0)
> -				return val;
> -			else if (val & MDIO_STAT1_LSTATUS)
> -				continue;
> -		}
> -
>  		val = phy_read_mmd(phydev, devad, MDIO_STAT1);
>  		if (val < 0)
>  			return val;
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7b3c5eec0129..2e7f0428e8fa 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -507,20 +507,29 @@ static int phy_config_aneg(struct phy_device *phydev)
>   */
>  static int phy_check_link_status(struct phy_device *phydev)
>  {
> -	int err;
> +	int err, i;
>  
>  	WARN_ON(!mutex_is_locked(&phydev->lock));
>  
> -	err = phy_read_status(phydev);
> -	if (err)
> -		return err;
> +	/* The link state is latched low so that momentary link drops can
> +	 * be detected. If the link has failed, re-read the link status
> +	 * to ensure that we are up to date with the current link state,
> +	 * while notifying that the link status has changed.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		err = phy_read_status(phydev);
> +		if (err)
> +			return err;
>  
> -	if (phydev->link && phydev->state != PHY_RUNNING) {
> -		phydev->state = PHY_RUNNING;
> -		phy_link_up(phydev);
> -	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
> -		phydev->state = PHY_NOLINK;
> -		phy_link_down(phydev, true);
> +		if (phydev->link && phydev->state != PHY_RUNNING) {
> +			phydev->state = PHY_RUNNING;
> +			phy_link_up(phydev);
> +		} else if (!phydev->link && phydev->state != PHY_NOLINK) {
> +			phydev->state = PHY_NOLINK;
> +			phy_link_down(phydev, true);
> +		}
> +		if (phydev->link)
> +			break;

One drawback of this approach may be that if we have an up-down-up
cycle then callback link_change_notify isn't called for either
transition. In most cases this shouldn't be an issue, but it's not
the expected behavior.

>  	}
>  
>  	return 0;
> [...]

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

[Florian Fainelli]

On 5/27/19 12:36 PM, Heiner Kallweit wrote:
> On 27.05.2019 21:25, Florian Fainelli wrote:
>>
>>
>> On 5/27/2019 11:28 AM, Heiner Kallweit wrote:
>>> 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>
>>> ---
>>>  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 20955836c..8030d0a97 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -774,8 +774,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))
>>
>> If Russell is okay with such a model where the PHY state machine still
>> manages the interrupts at large, and only calls a specific callback for
>> specific even handling, that's fine. We might have to allow PHY drivers
>> to let them specify what they want to get passed to
>> request_threaded_irq(), or leave it to them to do it.
>>
> This proposed easy model should be able to cover quite some use cases.
> One constraint may be that interrupts are disabled if phylib state
> machine isn't in a started state. Means most likely it's not able to
> cover e.g. the requirement to allow temperature warning interrupts if
> PHY is in state HALTED.
> 

There is possibly an user case where having interrupts might be useful,
imagine your system overheated and you brought down the PHY into
PHY_HALTED stated, you might want to be able to receive thermal trip
point crossing indicating that the low threshold has been crossed, which
means you could resume operating the link again.
-- 
Florian

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

[Heiner Kallweit]

On 28.05.2019 21:37, Florian Fainelli wrote:
> On 5/27/19 12:36 PM, Heiner Kallweit wrote:
>> On 27.05.2019 21:25, Florian Fainelli wrote:
>>>
>>>
>>> On 5/27/2019 11:28 AM, Heiner Kallweit wrote:
>>>> 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>
>>>> ---
>>>>  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 20955836c..8030d0a97 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -774,8 +774,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))
>>>
>>> If Russell is okay with such a model where the PHY state machine still
>>> manages the interrupts at large, and only calls a specific callback for
>>> specific even handling, that's fine. We might have to allow PHY drivers
>>> to let them specify what they want to get passed to
>>> request_threaded_irq(), or leave it to them to do it.
>>>
>> This proposed easy model should be able to cover quite some use cases.
>> One constraint may be that interrupts are disabled if phylib state
>> machine isn't in a started state. Means most likely it's not able to
>> cover e.g. the requirement to allow temperature warning interrupts if
>> PHY is in state HALTED.
>>
> 
> There is possibly an user case where having interrupts might be useful,
> imagine your system overheated and you brought down the PHY into
> PHY_HALTED stated, you might want to be able to receive thermal trip
> point crossing indicating that the low threshold has been crossed, which
> means you could resume operating the link again.
> 
To be prepared for all use cases we would need to request and enable
interrupts as early as in phy_probe(). This would be before phy_init_hw()
has been called. Not sure whether that's safe. phy_init_hw() calls the
config_init callback that may use the interface mode. Therefore we
can't simply move phy_init_hw().