netdev - Re: [PATCH net-next] net: phy: marvell10g: add downshift tunable support

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: <YUsa3z8KsuqS64k8@shell.armlinux.org.uk>
Date:   Wed, 22 Sep 2021 13:00:31 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Marek Beh__n <kabel@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next] net: phy: marvell10g: add downshift tunable
 support

On Fri, Sep 17, 2021 at 03:58:01PM +0100, Russell King (Oracle) wrote:
> On Fri, Sep 17, 2021 at 04:45:03PM +0200, Andrew Lunn wrote:
> > > +static int mv3310_set_downshift(struct phy_device *phydev, u8 ds)
> > > +{
> > > +	struct mv3310_priv *priv = dev_get_drvdata(&phydev->mdio.dev);
> > > +	u16 val;
> > > +	int err;
> > > +
> > > +	if (!priv->has_downshift)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (ds == DOWNSHIFT_DEV_DISABLE)
> > > +		return phy_clear_bits_mmd(phydev, MDIO_MMD_PCS, MV_PCS_DSC1,
> > > +					  MV_PCS_DSC1_ENABLE);
> > > +
> > > +	/* FIXME: The default is disabled, so should we disable? */
> > > +	if (ds == DOWNSHIFT_DEV_DEFAULT_COUNT)
> > > +		ds = 2;
> > 
> > Hi Russell
> > 
> > Rather than a FIXME, maybe just document that the hardware default is
> > disabled, which does not make too much sense, so default to 2 attempts?
> 
> Sadly, the downshift parameters aren't documented at all in the kernel,
> and one has to dig into the ethtool source to find out what they mean:
> 
> DOWNSHIFT_DEV_DEFAULT_COUNT -
> 	ethtool --set-phy-tunable ethN downshift on
> DOWNSHIFT_DEV_DISABLE -
> 	ethtool --set-phy-tunable ethN downshift off
> otherwise:
> 	ethtool --set-phy-tunable ethN downshift count N
> 
> This really needs to be documented somewhere in the kernel.

I was hoping that this would cause further discussion on what the
exact meaning of "DOWNSHIFT_DEV_DEFAULT_COUNT" is. Clearly, it's
meant to turn downshift on, but what does "default" actually mean?

If we define "default" as "whatever the hardware defaults to" then
for this phy, that would be turning off downshift.

So, should we rename "DOWNSHIFT_DEV_DEFAULT_COUNT" to be
"DOWNSHIFT_DEV_ENABLE" rather than trying to imply that it's
some kind of default that may need to be made up?

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