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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ