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

[Michael Walle]

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@xxxxxxxx/
> >
> > 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 */