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

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@microchip.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