Re: [PATCH] net: phy: disable eee due to errata on various KSZ switches

[Tim Harvey]

On Mon, Oct 7, 2024 at 9:57 AM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> On Mon, Oct 07, 2024 at 09:38:59AM -0700, Tim Harvey wrote:
> > On Sat, Oct 5, 2024 at 9:46 AM Andrew Lunn <andrew@xxxxxxx> wrote:
> > >
> > > On Fri, Oct 04, 2024 at 02:32:35PM -0700, Tim Harvey wrote:
> > > > The well-known errata regarding EEE not being functional on various KSZ
> > > > switches has been refactored a few times. Recently the refactoring has
> > > > excluded several switches that the errata should also apply to.
> > >
> > > Does the commit message say why?
> > >
> > > Does this need a Fixes: tag?
> > >
> >
> > Hi Andrew,
> >
> > Good question. I couldn't really figure out what fixes tag would be
> > appropriate as this code has changed a few times and broken in strange
> > ways. Here's a history as best I can tell:
> >
> > The original workaround for the errata was applied with a register
> > write to manually disable the EEE feature in MMD 7:60 which was being
> > applied for KSZ9477/KSZ9897/KSZ9567 switch ID's.
> >
> > Then came commit ("26dd2974c5b5 net: phy: micrel: Move KSZ9477 errata
> > fixes to PHY driver") and commit ("6068e6d7ba50 net: dsa: microchip:
> > remove KSZ9477 PHY errata handling") which moved the errata from the
> > switch driver to the PHY driver but only for PHY_ID_KSZ9477 (PHY ID)
> > however that PHY code was dead code because an entry was never added
> > for PHY_ID_KSZ9477 via MODULE_DEVICE_TABLE. So even if we add a
> > 'Fixes: 6068e6d7ba50' it would not be fixed.
> >
> > This was apparently realized much later and commit ("54a4e5c16382 net:
> > phy: micrel: add Microchip KSZ 9477 to the device table") added the
> > PHY_ID_KSZ9477 to the PHY driver. I believe the code was proper at
> > this point.
> >
> > Later commit ("6149db4997f5 net: phy: micrel: fix KSZ9477 PHY issues
> > after suspend/resume") breaks this again for all but KSZ9897 by only
> > applying the errata for that PHY ID.
> >
> > The most recent time this was affected was with commit ("08c6d8bae48c
> > net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)") which
> > removes the blatant register write to MMD 7:60 and replaces it by
> > setting phydev->eee_broken_modes = -1 so that the generic phy-c45 code
> > disables EEE but this is only done for the KSZ9477_CHIP_ID (Switch ID)
> > so its still broken at this point for the other switches that have
> > this errata.
> >
> > So at this point, the only commit that my patch would apply over is
> > the most recent 08c6d8bae48c but that wouldn't fix any of the previous
> > issues and it would be unclear what switch was broken at what point in
> > time.
>
> O.K, so its a mess :-(
>
> Lets look at this from a different direction. Which stable kernels do
> you actually care about? Is 6.6 enough for you? Do you need 6.1? 4.19?
>
> You should use a Fixed tag which goes back far enough for you. The
> patch itself might not apply that far back, but once you get it merged
> and backported as far as it does go, you can submit ported versions
> for older kernel, referencing the original fix commit hash number.
>

Yes... a mess. The difficulty everyone likely has with something like
this is they really can only test what they have as it's not super
clear each switch has for a PHY ID. It also wasn't really obvious to
me what switches had this errata (I just went through every switch in
the list of supported models in enum ksz_model and looked up their
errata which fortunately was public).

6.6 is good enough for me. I will resubmit with a fixes for the most
recent commit as well as include the above history in the commit as
well.

Best Regards,

Tim