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: <BYAPR11MB3558DD354E8162D02E1FCC21EC862@BYAPR11MB3558.namprd11.prod.outlook.com>
Date: Tue, 13 Aug 2024 22:24:52 +0000
From: <Tristram.Ha@...rochip.com>
To: <andrew@...n.ch>
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

> > +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.