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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ