netdev - Re: [PATCH v4 net-next 4/5] drivers/net/phy: add helpers to get/set PLCA configuration

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: <Y4+E/H5ZXqdd4med@gvm01>
Date:   Tue, 6 Dec 2022 19:07:56 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH v4 net-next 4/5] drivers/net/phy: add helpers to get/set
 PLCA configuration

On Tue, Dec 06, 2022 at 02:59:39PM +0100, Andrew Lunn wrote:
> > +/* MDIO Manageable Devices (MMDs). */
> > +#define MDIO_MMD_OATC14		MDIO_MMD_VEND2
> 
> As i said in a comment somewhere, i would prefer you use
> MDIO_MMD_VEND2, not MDIO_MMD_OATC14. We want the gentle reminder that
> these registers can contain anything the vendor wants, including, but
> not limited to, PLCA.
Ok, I'll fix that.

> > +/* Open Alliance TC14 PLCA CTRL0 register */
> > +#define MDIO_OATC14_PLCA_EN	0x8000  /* PLCA enable */
> > +#define MDIO_OATC14_PLCA_RST	0x4000  /* PLCA reset */
> 
> These are bits, so use the BIT macro. When this was part of mii.h,
> that file used this hex format so it made sense to follow that
> format. Now you are in a few file, you should use the macro.
Sure, I just moved the code in the new file without thinking of changing
this. Will do.

> > +/* Open Alliance TC14 PLCA CTRL1 register */
> > +#define MDIO_OATC14_PLCA_NCNT	0xff00	/* PLCA node count */
> > +#define MDIO_OATC14_PLCA_ID	0x00ff	/* PLCA local node ID */
> > +
> > +/* Open Alliance TC14 PLCA STATUS register */
> > +#define MDIO_OATC14_PLCA_PST	0x8000	/* PLCA status indication */
> > +
> > +/* Open Alliance TC14 PLCA TOTMR register */
> > +#define MDIO_OATC14_PLCA_TOT	0x00ff
> > +
> > +/* Open Alliance TC14 PLCA BURST register */
> > +#define MDIO_OATC14_PLCA_MAXBC	0xff00
> > +#define MDIO_OATC14_PLCA_BTMR	0x00ff
> > +
> > +#endif /* __MDIO_OPEN_ALLIANCE__ */
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index a87a4b3ffce4..dace5d3b29ad 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/mii.h>
> >  #include <linux/phy.h>
> >  
> > +#include "mdio-open-alliance.h"
> > +
> >  /**
> >   * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities
> >   * @phydev: target phy_device struct
> > @@ -931,6 +933,184 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
> >  }
> >  EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
> >  
> > +/**
> > + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
> > + * @phydev: target phy_device struct
> > + * @plca_cfg: output structure to store the PLCA configuration
> > + *
> > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
> > + *   Management Registers specifications, this function can be used to retrieve
> > + *   the current PLCA configuration from the standard registers in MMD 31.
> > + */
> > +int genphy_c45_plca_get_cfg(struct phy_device *phydev,
> > +			    struct phy_plca_cfg *plca_cfg)
> > +{
> > +	int ret;
> > +
> > +	ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	plca_cfg->version = ret;
> 
> It would be good to verify this value, and return -ENODEV if it is not
> valid.
Got it, thanks.

Piergiorgio