Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 1 Nov 2021 20:33:50 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Grygorii Strashko <grygorii.strashko@ti.com>
Cc:     "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <kuba@kernel.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Florian Fainelli <f.fainelli@gmail.com>,
        linux-kernel@vger.kernel.org, Vignesh Raghavendra <vigneshr@ti.com>
Subject: Re: [RFC PATCH] net: phy/mdio: enable mmd indirect access through
 phy_mii_ioctl()
Message-ID: <YYBBHsFEwGdPJw3b@lunn.ch>
References: <20211101182859.24073-1-grygorii.strashko@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211101182859.24073-1-grygorii.strashko@ti.com>
Precedence: bulk

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