netdev - Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <301c5f90-0307-4c23-b867-6677d41dce47@lunn.ch>
Date: Sat, 10 Aug 2024 19:44:02 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung Huh <woojung.huh@...rochip.com>, UNGLinuxDriver@...rochip.com,
	devicetree@...r.kernel.org, Florian Fainelli <f.fainelli@...il.com>,
	Vladimir Oltean <olteanv@...il.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Marek Vasut <marex@...x.de>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts
 in KSZ9477/KSZ9893 switch families

> +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> +					   u8 *data)
> +{
> +	struct dsa_switch *ds = dev->ds;
> +	struct phy_device *phydev;
> +	int cnt = 0;
> +
> +	phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> +	if (*data & PORT_PHY_INT) {
> +		/* Handle the interrupt if there is no PHY device or its
> +		 * interrupt is not registered yet.
> +		 */
> +		if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> +			u8 phy_status;
> +
> +			ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> +				   &phy_status);
> +			if (phydev)
> +				phy_trigger_machine(phydev);
> +			++cnt;
> +			*data &= ~PORT_PHY_INT;
> +		}
> +	}

This looks like a layering violation. Why is this needed? An interrupt
controller generally has no idea what the individual interrupt is
about. It just calls into the interrupt core to get the handler
called, and then clears the interrupt. Why does that not work here?

What other DSA drivers do if they need to handle some of the
interrupts is just request the interrupt like any other driver:

https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-639x.c#L95

> +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data)
> +{
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 data32;
> +
> +	if (port > 0)
> +		return ksz9477_handle_port_irq(dev, port - 1, data);
> +
> +	ksz_read32(dev, REG_SW_INT_STATUS__4, &data32);
> +	if (data32 & LUE_INT) {
> +		u8 lue;
> +
> +		ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue);
> +		ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue);
> +		if (lue & LEARN_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue learn fail\n");
> +		if (lue & WRITE_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue write fail\n");
> +		ret = IRQ_HANDLED;
> +	}

https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/global1_atu.c#L474

	Andrew