netdev - Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through phy_mii

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: <YYBBHsFEwGdPJw3b@lunn.ch>
Date:   Mon, 1 Nov 2021 20:33:50 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        linux-kernel@...r.kernel.org, Vignesh Raghavendra <vigneshr@...com>
Subject: Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through
 phy_mii_ioctl()

On Mon, Nov 01, 2021 at 08:28:59PM +0200, Grygorii Strashko wrote:
> This patch enables access to C22 PHY MMD address space through

I'm not sure the terminology is correct here. I think it actually
enables access to C45 address space, making use of C45 over C22.

> phy_mii_ioctl() SIOCGMIIREG/SIOCSMIIREG IOCTLs. It checks if R/W request is
> received with C45 flag enabled while MDIO bus doesn't support C45 and, in
> this case, tries to treat prtad as PHY MMD selector and use MMD API.
> 
> With this change it's possible to r/w PHY MMD registers with phytool, for
> example, before:
> 
>   phytool read eth0/0x1f:0/0x32
>   0xffea
> 
> after:
>   phytool read eth0/0x1f:0/0x32
>   0x00d1
> @@ -300,8 +300,19 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>  			prtad = mii_data->phy_id;
>  			devad = mii_data->reg_num;
>  		}
> -		mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad,
> -						 devad);
> +		if (mdio_phy_id_is_c45(mii_data->phy_id) &&
> +		    phydev->mdio.bus->probe_capabilities <= MDIOBUS_C22) {
> +			phy_lock_mdio_bus(phydev);
> +
> +			mii_data->val_out = __mmd_phy_read(phydev->mdio.bus,
> +							   mdio_phy_id_devad(mii_data->phy_id),
> +							   prtad,
> +							   mii_data->reg_num);
> +
> +			phy_unlock_mdio_bus(phydev);
> +		} else {
> +			mii_data->val_out = mdiobus_read(phydev->mdio.bus, prtad, devad);
> +		}

The layering look wrong here. You are trying to perform MDIO bus
operation here, so it seems odd to perform __mmd_phy_read(). I wonder
if it would be cleaner to move C45 over C22 down into the mdiobus_
level API?

      Andrew