Re: [PATCH net-next v1 7/7] net: usb: lan78xx: Enable EEE support with phylink integration

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Jan 13, 2025 at 02:45:14PM +0100, Andrew Lunn wrote:
> On Mon, Jan 13, 2025 at 01:29:37PM +0000, Russell King (Oracle) wrote:
> > On Mon, Jan 13, 2025 at 01:42:06PM +0100, Oleksij Rempel wrote:
> > > On Thu, Jan 09, 2025 at 08:33:07PM +0100, Andrew Lunn wrote:
> > > > > Andrew had a large patch set, which added the phylib core code, and
> > > > > fixed up many drivers. This was taken by someone else, and only
> > > > > Andrew's core phylib code was merged along with the code for their
> > > > > driver, thus breaking a heck of a lot of other drivers.
> > > > 
> > > > As Russell says, i did convert all existing drivers over the new
> > > > internal API, and removed some ugly parts of the old EEE core code.
> > > > I'm not too happy we only got part way with my patches. Having this in
> > > > between state makes the internal APIs much harder to deal with, and as
> > > > Russell says, we broke a few MAC drivers because the rest did not get
> > > > merged.
> > > > 
> > > > Before we think about extensions to the kAPI, we first need to finish
> > > > the refactor. Get all MAC drivers over to the newer internal API and
> > > > remove phy_init_eee() which really does need to die. My patches have
> > > > probably bit rotted a bit, but i doubt they are unusable. So i would
> > > > like to see them merged. I would however leave phylink drivers to
> > > > Russell. He went a slight different way than i did, and he should get
> > > > to decide how phylink should support this.
> > > 
> > > Hi Andrew,
> > > 
> > > Ok. If I see it correctly, all patches from the
> > > v6.4-rc6-net-next-ethtool-eee-v7 branch, which I was working on, have been
> > > merged by now. The missing parts are patches from the
> > > v6.3-rc3-net-next-ethtool-eee-v5 branch.
> > > 
> > > More precisely, the following non-phylink drivers still need to be addressed:
> > > drivers/net/ethernet/broadcom/asp2
> > > drivers/net/ethernet/broadcom/genet
> > > drivers/net/ethernet/samsung/sxgbe
> > > drivers/net/usb/lan78xx - this one is in progress
> > > 
> > > Unfortunately, I won’t be able to accomplish this before the merge window, as I
> > > am currently on sick leave. However, I promise to take care of it as soon as
> > > possible.
> > 
> > Does any of this include mvneta?
> 
> Hi Russell 
> 
> I asked Oleksij to skip MAC drivers using phylink. I'm not sure it
> makes sense to merge my phylink changes for EEE and then replace them
> with your EEE implementation.

Doing an audit on today's net-next after I've fixed up another driver
for the phylib-eee fallout, we have two obvious breakages remaining
in drivers that directly use phylib:

drivers/net/ethernet/samsung/sxgbe/sxgbe_ethtool.c:

        edata->tx_lpi_timer = priv->tx_lpi_timer;

        return phy_ethtool_get_eee(dev->phydev, edata);

drivers/net/ethernet/broadcom/genet/bcmgenet.c:

        e->tx_lpi_enabled = p->tx_lpi_enabled;
        e->tx_lpi_timer = bcmgenet_umac_readl(priv, UMAC_EEE_LPI_TIMER);

        return phy_ethtool_get_eee(dev->phydev, e);

Two others change ->tx_lpi_timer after calling phy_ethtool_get_eee()
and thus are unaffected:

drivers/net/ethernet/realtek/r8169_main.c:
drivers/net/usb/lan78xx.c

Howeve,r I wonder whether lan78xx_set_eee() is racy:

        ret = phy_ethtool_set_eee(net->phydev, edata);
        if (ret < 0)
                goto out;

        buf = (u32)edata->tx_lpi_timer;
        ret = lan78xx_write_reg(dev, EEE_TX_LPI_REQ_DLY, buf);

since phy_ethtool_set_eee() will have bounced the link if the timer
was changed.

I'm also wondering whether r8169 programs its LPI timer correctly.

I think all phylink using drivers network are now no longer affected.

I'm unsure about many DSA drivers. mt753x:

        u32 set, mask = LPI_THRESH_MASK | LPI_MODE_EN;

        if (e->tx_lpi_timer > 0xFFF)
                return -EINVAL;

        set = LPI_THRESH_SET(e->tx_lpi_timer);
        if (!e->tx_lpi_enabled)
                /* Force LPI Mode without a delay */
                set |= LPI_MODE_EN;
        mt7530_rmw(priv, MT753X_PMEEECR_P(port), mask, set);

Why force LPI *without* a delay if tx_lpi_enabled is false? This
seems to go against the documented API:

 * @tx_lpi_enabled: Whether the interface should assert its tx lpi, given
 *      that eee was negotiated.

qca8k_set_mac_eee() sets the LPI enabled based off eee->eee_enabled.
It doesn't seem to change the register on link up/down, so I wonder
how the autoneg resolution is handled. Maybe it isn't, so maybe it's
buggy.

b53_set_mac_eee() looks similar to qca8k_set_mac_eee().

So there's still work to be done.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!