| 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
| ||
|
Date: Wed, 1 Mar 2023 12:32:08 +0000
From: Ken Sloat <ken.s@...iscite.com>
To: Nuno Sá <noname.nuno@...il.com>
CC: "pabeni@...hat.com" <pabeni@...hat.com>,
"edumazet@...gle.com" <edumazet@...gle.com>,
Michael Hennerich <michael.hennerich@...log.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Rob Herring <robh+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
Alexandru Tachici <alexandru.tachici@...log.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Ken Sloat <ken.s@...iscite.com>
Subject: RE: [PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of
fast link down
Hi Nuno,
> -----Original Message-----
> From: Nuno Sá <noname.nuno@...il.com>
> Sent: Wednesday, March 1, 2023 2:34 AM
> To: Ken Sloat <ken.s@...iscite.com>
> Cc: pabeni@...hat.com; edumazet@...gle.com; Michael Hennerich
> <michael.hennerich@...log.com>; David S. Miller
> <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>; Rob Herring
> <robh+dt@...nel.org>; Andrew Lunn <andrew@...n.ch>; Heiner Kallweit
> <hkallweit1@...il.com>; Russell King <linux@...linux.org.uk>; Alexandru
> Tachici <alexandru.tachici@...log.com>; netdev@...r.kernel.org;
> devicetree@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v2 1/2] net: phy: adin: Add flags to allow disabling of fast
> link down
>
> On Tue, 2023-02-28 at 13:49 -0500, Ken Sloat wrote:
> > "Enhanced Link Detection" is an ADI PHY feature that allows for
> > earlier detection of link down if certain signal conditions are met.
> > Also known on other PHYs as "Fast Link Down," this feature is for the
> > most part enabled by default on the PHY. This is not suitable for all
> > applications and breaks the IEEE standard as explained in the ADI
> > datasheet.
> >
> > To fix this, add override flags to disable fast link down for
> > 1000BASE-T and 100BASE-TX respectively by clearing any related feature
> > enable bits.
> >
> > This new feature was tested on an ADIN1300 but according to the
> > datasheet applies equally for 100BASE-TX on the ADIN1200.
> >
> > Signed-off-by: Ken Sloat <ken.s@...iscite.com>
> > ---
> > Changes in v2:
> > -Change "FLD" nomenclature to commonly used "Fast Link Down" phrase
> > in
> > source code and bindings. Also document this in the commit
> > comments.
> > -Utilize phy_clear_bits_mmd() in register bit operations.
> >
> > drivers/net/phy/adin.c | 43
> > ++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 43 insertions(+)
> >
> > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index
> > da65215d19bb..0bab7e4d3e29 100644
> > --- a/drivers/net/phy/adin.c
> > +++ b/drivers/net/phy/adin.c
> > @@ -69,6 +69,15 @@
> > #define ADIN1300_EEE_CAP_REG 0x8000
> > #define ADIN1300_EEE_ADV_REG 0x8001
> > #define ADIN1300_EEE_LPABLE_REG 0x8002
> > +#define ADIN1300_FLD_EN_REG 0x8E27 #define
> > +ADIN1300_FLD_PCS_ERR_100_EN BIT(7) #define
> > +ADIN1300_FLD_PCS_ERR_1000_EN BIT(6) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_100_EN BIT(5) #define
> > +ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN BIT(4) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_100_EN BIT(3) #define
> > +ADIN1300_FLD_SLCR_IN_ZDET_1000_EN BIT(2) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_100_EN BIT(1) #define
> > +ADIN1300_FLD_SLCR_IN_INVLD_1000_EN BIT(0)
> > #define ADIN1300_CLOCK_STOP_REG 0x9400
> > #define ADIN1300_LPI_WAKE_ERR_CNT_REG 0xa000
> >
> > @@ -508,6 +517,36 @@ static int adin_config_clk_out(struct phy_device
> > *phydev)
> > ADIN1300_GE_CLK_CFG_MASK, sel);
> > }
> >
> > +static int adin_fast_down_disable(struct phy_device *phydev) {
> > + struct device *dev = &phydev->mdio.dev;
> > + int rc;
> > +
> > + if (device_property_read_bool(dev, "adi,disable-fast-down-
> > 1000base-t")) {
> > + rc = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> > + ADIN1300_FLD_EN_REG,
> > +
> > ADIN1300_FLD_PCS_ERR_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_OUT_STUCK_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_ZDET_1000_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_INVLD_1000_EN);
> > + if (rc < 0)
> > + return rc;
> > + }
> > +
> > + if (device_property_read_bool(dev, "adi,disable-fast-down-
> > 100base-tx")) {
> > + phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1,
> > + ADIN1300_FLD_EN_REG,
> > + ADIN1300_FLD_PCS_ERR_100_EN
> > |
> > +
> > ADIN1300_FLD_SLCR_OUT_STUCK_100_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_ZDET_100_EN |
> > +
> > ADIN1300_FLD_SLCR_IN_INVLD_100_EN);
> > + if (rc < 0)
> > + return rc;
> > + }
>
> This is not exactly what I had in mind... I was suggesting something like
> caching the complete "bits word" in both of your if() statements and then
> just calling phy_clear_bits_mmd() once. If I'm not missing something
> obvious, something like this:
>
> u16 bits = 0; //or any other name more appropriate
>
> if (device_property_read_bool(...))
> bits = ADIN1300_FLD_PCS_ERR_1000_EN | ...
>
> if (device_property_read_bool())
> bits |= ...
>
> if (!bits)
> return 0;
>
> return phy_clear_bits_mmd();
>
> Does this sound sane to you?
>
> Anyways, this is also not a big deal...
Yes, I agree that would be cleaner. I will adjust.
>
> - Nuno Sá
Thanks
Sincerely,
Ken Sloat
Powered by blists - more mailing lists