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

[Michael Walle <michael@walle.cc>]

Am 2020-02-26 22:17, schrieb Heiner Kallweit:
> On 26.02.2020 12:12, Michael Walle wrote:
>> Am 2020-02-26 08:27, schrieb Heiner Kallweit:
>>> 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?
>> 
>> Yes, as mentioned above this wont work for PHYs which has a 
>> clear-on-read
>> status register, because you may loose interrupts between 
>> handle_interrupt()
>> and phy_clear_interrupt().
>> 
>> See also
>>  
>> https://lore.kernel.org/netdev/bd47f8e1ebc04fa98856ed8d89b91419@walle.cc/
>> 
>> And esp. Russell reply. I tried using handle_interrupt() but it won't 
>> work
>> unless you make the ack_interrupt a NOOP. but even then it won't work 
>> because
>> the ack_interrupt is also used during setup etc.
>> 
> Right, now I remember .. So far we have only one user of the 
> handle_interrupt
> callback. Following a proposal from the quoted discussion, can you base 
> your
> patch on the following and check?
> Note: Even though you implement handle_interrupt, you still have to 
> implement
> ack_interrupt too.

I guess that should work, I can give that a try. But I'm also preparing 
support
for the BCM54140, a quad PHY. I guess we have the same problem with
did_interrupt(). Eg. to tell if there actually was an interrupt from 
that PHY
you have to read the status register which clears the interrupt. So
handle_interrupt() will left with no actual interrupt pending bits. I've 
had a
look at how the vsc8584 does it as it also uses did_interrupt() together 
with
handle_interrupt() and it looks like it only works because 
handle_interrupt()
doesn't actually read the pending bits again. Also I guess there is a 
chance
of missing interrupts between vsc8584_did_interrupt() and
vsc85xx_ack_interrupt() which both clears interrupts. Although I'm not 
sure
if that matters for the current implementation.

-michael

> 
> 
> ---
>  drivers/net/phy/mscc.c | 3 ++-
>  drivers/net/phy/phy.c  | 4 ++--
>  include/linux/phy.h    | 4 +++-
>  3 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index 937ac7da2..20b9d3ef5 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -2868,7 +2868,8 @@ static int vsc8584_handle_interrupt(struct
> phy_device *phydev)
>  #endif
> 
>  	phy_mac_interrupt(phydev);
> -	return 0;
> +
> +	return vsc85xx_ack_interrupt(phydev);
>  }
> 
>  static int vsc85xx_config_init(struct phy_device *phydev)
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d76e038cf..de52f0e82 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -725,10 +725,10 @@ static irqreturn_t phy_interrupt(int irq, void 
> *phy_dat)
>  	} else {
>  		/* reschedule state queue work to run as soon as possible */
>  		phy_trigger_machine(phydev);
> +		if (phy_clear_interrupt(phydev))
> +			goto phy_err;
>  	}
> 
> -	if (phy_clear_interrupt(phydev))
> -		goto phy_err;
>  	return IRQ_HANDLED;
> 
>  phy_err:
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 80f8b2158..9e2895ee4 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -560,7 +560,9 @@ struct phy_driver {
>  	 */
>  	int (*did_interrupt)(struct phy_device *phydev);
> 
> -	/* Override default interrupt handling */
> +	/* Override default interrupt handling. Handler has to ensure
> +	 * that interrupt is ack'ed.
> +	 */
>  	int (*handle_interrupt)(struct phy_device *phydev);
> 
>  	/* Clears up any memory if needed */