Re: [RFC PATCH 1/2] net: phy: let the driver register its own IRQ handler

[Heiner Kallweit <hkallweit1@gmail.com>]

On 26.02.2020 00:08, Michael Walle wrote:
> There are more and more PHY drivers which has more than just the PHY
> link change interrupts. For example, temperature thresholds or PTP
> interrupts.
> 
> At the moment it is not possible to correctly handle interrupts for PHYs
> which has a clear-on-read interrupt status register. It is also likely
> that the current approach of the phylib isn't working for all PHYs out
> there.
> 
> Therefore, this patch let the PHY driver register its own interrupt
> handler. To notify the phylib about a link change, the interrupt handler
> has to call the new function phy_drv_interrupt().
> 

We have phy_driver callback handle_interrupt for custom interrupt
handlers. Any specific reason why you can't use it for your purposes?

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/net/phy/phy.c        | 15 +++++++++++++++
>  drivers/net/phy/phy_device.c |  6 ++++--
>  include/linux/phy.h          |  2 ++
>  3 files changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d76e038cf2cb..f25aacbcf1d9 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -942,6 +942,21 @@ void phy_mac_interrupt(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(phy_mac_interrupt);
>  
> +/**
> + * phy_drv_interrupt - PHY drivers says the link has changed
> + * @phydev: phy_device struct with changed link
> + *
> + * The PHY driver may implement his own interrupt handler. It will call this
> + * function to notify us about a link change. Trigger the state machine and
> + * work a work queue.
> + */

This function would duplicate phy_mac_interrupt().

> +void phy_drv_interrupt(struct phy_device *phydev)
> +{
> +	/* Trigger a state machine change */
> +	phy_trigger_machine(phydev);
> +}
> +EXPORT_SYMBOL(phy_drv_interrupt);
> +
>  static void mmd_eee_adv_to_linkmode(unsigned long *advertising, u16 eee_adv)
>  {
>  	linkmode_zero(advertising);
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 6a5056e0ae77..6d8c94e61251 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -965,7 +965,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
>  		return rc;
>  
>  	phy_prepare_link(phydev, handler);
> -	if (phy_interrupt_is_valid(phydev))
> +	if (phy_interrupt_is_valid(phydev) &&
> +	    phydev->drv->flags & PHY_HAS_OWN_IRQ_HANDLER)
>  		phy_request_interrupt(phydev);

Here most likely a ! is missing. because as-is you would break
current phylib interrupt mode. Where in the PHY driver (in which
callback) do you want to register your own interrupt handler?

>  
>  	return 0;
> @@ -2411,7 +2412,8 @@ EXPORT_SYMBOL(phy_validate_pause);
>  
>  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->flags & PHY_HAS_OWN_IRQ_HANDLER);
>  }
>  
>  /**
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index c570e162e05e..46f73b94fd60 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -75,6 +75,7 @@ extern const int phy_10gbit_features_array[1];
>  
>  #define PHY_IS_INTERNAL		0x00000001
>  #define PHY_RST_AFTER_CLK_EN	0x00000002
> +#define PHY_HAS_OWN_IRQ_HANDLER	0x00000004
>  #define MDIO_DEVICE_IS_PHY	0x80000000
>  
>  /* Interface Mode definitions */
> @@ -1235,6 +1236,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n,
>  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_drv_interrupt(struct phy_device *phydev);
>  void phy_start_machine(struct phy_device *phydev);
>  void phy_stop_machine(struct phy_device *phydev);
>  void phy_ethtool_ksettings_get(struct phy_device *phydev,
>