On Sat, Oct 31, 2020 at 11:18:18AM +0100, Heiner Kallweit wrote:

On 31.10.2020 00:36, Andrew Lunn wrote:

quoted

quoted

quoted

- 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

Hi Andrew,

quoted

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.

Sure, I just wanted to add the comment before others simply copy and
paste this (pseudo) code. And in patch 9 (aquantia) and 18 (realtek)
it is used as is. And IIRC at least the Aquantia PHY doesn't mask
the interrupt status.

Hi Heiner,

If I understand correctly what you are suggesting, the
.handle_interrupt() for the aquantia would look like this:

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

	irq_status = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_TX_VEND_INT_STATUS2);
	if (irq_status < 0) {
		phy_error(phydev);
		return IRQ_NONE;
	}

	if (!(irq_status & MDIO_AN_TX_VEND_INT_STATUS2_MASK))
		return IRQ_NONE;

	phy_trigger_machine(phydev);

	return IRQ_HANDLED;
}

So only return IRQ_HANDLED when one of the bits from INT_STATUS
corresponding with the enabled interrupts are set, not if any other link
status bit is set.

Ok, I'll send a v2 with these kind of changes.

Ioana