Re: [PATCH net-next 00/19] net: phy: add support for shared interrupts (part 1)

[Ioana Ciornei]

On Sat, Oct 31, 2020 at 12:36:27AM +0100, Andrew Lunn wrote:
> > > - Every PHY driver gains a .handle_interrupt() implementation that, for
> > >   the most part, would look like below:
> > > 
> > > 	irq_status = phy_read(phydev, INTR_STATUS);
> > > 	if (irq_status < 0) {
> > > 		phy_error(phydev);
> > > 		return IRQ_NONE;
> > > 	}
> > > 
> > > 	if (irq_status == 0)
> > 
> > Here I have a concern, bits may be set even if the respective interrupt
> > source isn't enabled. Therefore we may falsely blame a device to have
> > triggered the interrupt. irq_status should be masked with the actually
> > enabled irq source bits.
> 
> Hi Heiner
> 
> I would say that is a driver implementation detail, for each driver to
> handle how it needs to handle it. I've seen some hardware where the
> interrupt status is already masked with the interrupt enabled
> bits. I've soon other hardware where it is not.
> 
> For example code, what is listed above is O.K. The real implementation
> in a driver need knowledge of the hardware.
> 

Hi,

As Andrew said, that is just an example code that could work for some
devices but should be extended depending on how the actual PHY is
working.

For example, the VSC8584 will still be trigerring the link state machine
just on the link change interrupt, I am not changing this:

static irqreturn_t vsc8584_handle_interrupt(struct phy_device *phydev)
{
	irqreturn_t ret;
	int irq_status;

	irq_status = phy_read(phydev, MII_VSC85XX_INT_STATUS);
	if (irq_status < 0)
		return IRQ_NONE;

	/* Timestamping IRQ does not set a bit in the global INT_STATUS, so
	 * irq_status would be 0.
	 */
	ret = vsc8584_handle_ts_interrupt(phydev);
	if (!(irq_status & MII_VSC85XX_INT_MASK_MASK))
		return ret;

	if (irq_status & MII_VSC85XX_INT_MASK_EXT)
		vsc8584_handle_macsec_interrupt(phydev);

	if (irq_status & MII_VSC85XX_INT_MASK_LINK_CHG)
		phy_trigger_machine(phydev);

	return IRQ_HANDLED;
}

Ioana