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: <736f67be-a21a-40ba-9eeb-d94040d56dc2@lunn.ch>
Date: Wed, 14 Aug 2024 15:55:52 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Tristram.Ha@...rochip.com
Cc: Woojung.Huh@...rochip.com, UNGLinuxDriver@...rochip.com,
	devicetree@...r.kernel.org, f.fainelli@...il.com, olteanv@...il.com,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, 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

On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@...rochip.com wrote:
> > > +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
> 
> The PHY and ACL interrupt handling can be removed, but the SGMII
> interrupt handling cannot as the SGMII port is simulated as having an
> internal PHY but the regular PHY interrupt processing will not clear the
> interrupt.
> 
> Furthermore, there will be a situation where the SGMII interrupt is
> triggered before the PHY interrupt handling function is registered.

This is one of the reasons i suggested a PCS driver. Look at
drivers/net/dsa/mv88e6xxx/pcs-6185.c as an example, how it handles
interrupts from the PCS. And it is a similar setup, the switch has an
interrupt controller, and the PCS driver requests the interrupt. PCS
drivers do not need to be complex. pcs-6185.c has an empty AN restart
callback, pcs_config does nothing, etc.

	  Andrew