Re: [PATCH net-next v6 1/9] net: dsa: microchip: Add support for KSZ8463 global irq

[Vladimir Oltean]

On Thu, Mar 05, 2026 at 01:39:29PM +0100, Bastien Curutchet wrote:
> Hi Vladimir,
> 
> On 3/5/26 10:56 AM, Vladimir Oltean wrote:
> > On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider Electric) wrote:
> > > @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)
> > >   	unsigned int nhandled = 0;
> > >   	struct ksz_device *dev;
> > >   	unsigned int sub_irq;
> > > -	u8 data;
> > > +	u16 data;
> > >   	int ret;
> > >   	u8 n;
> > >   	dev = kirq->dev;
> > > -	/* Read interrupt status register */
> > > -	ret = ksz_read8(dev, kirq->reg_status, &data);
> > > +	/*
> > > +	 * Most of the KSZ switches have a 8-bits long interrupt status
> > > +	 * register, but the KSZ8463 has a 16-bits long one. The overread here
> > > +	 * is safe because we only iterate over kirq->nirqs in the below loop.
> > 
> > FWIW, this isn't the only thing making an overread "safe".
> > If the adjacent register also has "clear on read" semantics, that's not
> > good.
> > 
> > I can't tell whether that's the case here, though. There are just too
> > many hardware variations to check for. I just wanted to point out that
> > the reasoning is incomplete.
> > 
> You're right, I hadn't thought about the 'clear on read' case.
> 
> I'll use ksz_read16() only for the KSZ8463 to be 100% sure it won't break
> anything for other switches.

I'm still on the fence on whether to say this or not, because I don't
really want to get so involved in internal driver bookkeeping, but...
this driver is just becoming a hell to review, even if I want to
concentrate exclusively on correct API use, like I try to do.

Linus Walleij has added a new ks8995 driver, which has some overlap with
the common ksz driver for the KSZ8 family. Now he wants to remove the
overlapping device support:
https://lore.kernel.org/netdev/20260219-ks8995-fixups-v3-0-a7fc63fe1916@xxxxxxxxxx/

Maybe we should go the other way around, migrate KSZ8 support to the
ks8995 driver instead? The common ksz driver is becoming just extremely
convoluted to handle all hardware variations. Would it help in any way
to maintain cleaner code paths, what do you think?