netdev - Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45 devices

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]

Date:   Wed, 17 Jun 2020 18:44:51 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Calvin Johnson <calvin.johnson@....nxp.com>
Cc:     Jeremy Linton <jeremy.linton@....com>, Jon <jon@...id-run.com>,
        Cristi Sovaiala <cristian.sovaiala@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Andrew Lunn <andrew@...n.ch>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        netdev@...r.kernel.org, linux.cj@...il.com
Subject: Re: [PATCH v1 1/3] net: phy: Allow mdio buses to auto-probe c45
 devices

On Wed, Jun 17, 2020 at 10:45:33PM +0530, Calvin Johnson wrote:
> From: Jeremy Linton <jeremy.linton@....com>
> 
> The mdiobus_scan logic is currently hardcoded to only
> work with c22 devices. This works fairly well in most
> cases, but its possible a c45 device doesn't respond
> despite being a standard phy. If the parent hardware
> is capable, it makes sense to scan for c22 devices before
> falling back to c45.
> 
> As we want this to reflect the capabilities of the STA,
> lets add a field to the mii_bus structure to represent
> the capability. That way devices can opt into the extended
> scanning. Existing users should continue to default to c22
> only scanning as long as they are zero'ing the structure
> before use.
> 
> Signed-off-by: Jeremy Linton <jeremy.linton@....com>
> Signed-off-by: Calvin Johnson <calvin.johnson@....nxp.com>

I know that we've hashed this out quite a bit already, but I would like
to point out that include/linux/mdio.h contains:

 * struct mdio_if_info - Ethernet controller MDIO interface
 * @mode_support: MDIO modes supported.  If %MDIO_SUPPORTS_C22 is set then
 *      MII register access will be passed through with @devad =
 *      %MDIO_DEVAD_NONE.  If %MDIO_EMULATE_C22 is set then access to
 *      commonly used clause 22 registers will be translated into
 *      clause 45 registers.

#define MDIO_SUPPORTS_C22               1
#define MDIO_SUPPORTS_C45               2
#define MDIO_EMULATE_C22                4

While this structure is not applicable to phylib or mii_bus, it may be
worth considering that there already exist definitions for identifying
the properties of the underlying bus.

> ---
> 
>  drivers/net/phy/mdio_bus.c | 17 +++++++++++++++--
>  include/linux/phy.h        |  7 +++++++
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index 6ceee82b2839..e6c179b89907 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -739,10 +739,23 @@ EXPORT_SYMBOL(mdiobus_free);
>   */
>  struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
>  {
> -	struct phy_device *phydev;
> +	struct phy_device *phydev = ERR_PTR(-ENODEV);
>  	int err;
>  
> -	phydev = get_phy_device(bus, addr, false);
> +	switch (bus->probe_capabilities) {
> +	case MDIOBUS_C22:
> +		phydev = get_phy_device(bus, addr, false);
> +		break;
> +	case MDIOBUS_C45:
> +		phydev = get_phy_device(bus, addr, true);
> +		break;
> +	case MDIOBUS_C22_C45:
> +		phydev = get_phy_device(bus, addr, false);
> +		if (IS_ERR(phydev))
> +			phydev = get_phy_device(bus, addr, true);
> +		break;
> +	}
> +
>  	if (IS_ERR(phydev))
>  		return phydev;
>  
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 9248dd2ce4ca..50e5312b2304 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -298,6 +298,13 @@ struct mii_bus {
>  	/* RESET GPIO descriptor pointer */
>  	struct gpio_desc *reset_gpiod;
>  
> +	/* bus capabilities, used for probing */
> +	enum {
> +		MDIOBUS_C22 = 0,
> +		MDIOBUS_C45,
> +		MDIOBUS_C22_C45,
> +	} probe_capabilities;

I think it would be better to reserve "0" to mean that no capabilities
have been declared.  We hae the situation where we have mii_bus that
exist which do support C45, but as they stand, probe_capabilities will
be zero, and with your definitions above, that means MDIOBUS_C22.

It seems this could lock in some potential issues later down the line
if we want to use this elsewhere.

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