[net-next 0/2] net: phy: Fallback to C22 for PHY register read/write

[net-next 0/2] net: phy: Fallback to C22 for PHY register read/write

[Niklas Söderlund]

Hello,

This series extends the behavior of the SIOCGMIIREG and SIOCSMIIREG 
IOCTLs to use the convenience functions mmd_phy_read() and 
mmd_phy_write() for C45 access. This brings the benefit of the core 
falling back to indirect C22 access of the underlying driver do not 
itself provide C45 read/write capability.

Patch 1/2 exposes the read/write functions making them available to the 
IOCTL callback. While patch 2/2 changes the IOCTL callback to make use 
of them.

Without this change using tools like phytool to read/write registers on 
a C45 only PHY using a C22 only driver fails as the IOCTL callback don't 
know how to talk C45 on the mdio bus.

Niklas Söderlund (2):
  net: phy: Expose the direct mdiobus access functions
  net: phy: Fallback to C22 access if needed in phy_mii_ioctl()

 drivers/net/phy/phy-core.c | 10 ++++++----
 drivers/net/phy/phy.c      | 18 ++++++++++++------
 include/linux/phy.h        | 12 ++++++++++++
 3 files changed, 30 insertions(+), 10 deletions(-)

-- 
2.46.0

[net-next 1/2] net: phy: Expose the direct mdiobus access functions

[Niklas Söderlund]

Expose the direct mdiobus read and write functions. These will be needed
to refactor the SIOCGMIIREG and SIOCSMIIREG IOCTLs to fallback to
indirect C45 access if needed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/phy/phy-core.c | 10 ++++++----
 include/linux/phy.h        | 12 ++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 1f98b6a96c15..2845294053eb 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -542,8 +542,8 @@ static void mmd_phy_indirect(struct mii_bus *bus, int phy_addr, int devad,
 			devad | MII_MMD_CTRL_NOINCR);
 }
 
-static int mmd_phy_read(struct mii_bus *bus, int phy_addr, bool is_c45,
-			int devad, u32 regnum)
+int mmd_phy_read(struct mii_bus *bus, int phy_addr, bool is_c45,
+		 int devad, u32 regnum)
 {
 	if (is_c45)
 		return __mdiobus_c45_read(bus, phy_addr, devad, regnum);
@@ -552,9 +552,10 @@ static int mmd_phy_read(struct mii_bus *bus, int phy_addr, bool is_c45,
 	/* Read the content of the MMD's selected register */
 	return __mdiobus_read(bus, phy_addr, MII_MMD_DATA);
 }
+EXPORT_SYMBOL(mmd_phy_read);
 
-static int mmd_phy_write(struct mii_bus *bus, int phy_addr, bool is_c45,
-			 int devad, u32 regnum, u16 val)
+int mmd_phy_write(struct mii_bus *bus, int phy_addr, bool is_c45,
+		  int devad, u32 regnum, u16 val)
 {
 	if (is_c45)
 		return __mdiobus_c45_write(bus, phy_addr, devad, regnum, val);
@@ -563,6 +564,7 @@ static int mmd_phy_write(struct mii_bus *bus, int phy_addr, bool is_c45,
 	/* Write the data into MMD's selected register */
 	return __mdiobus_write(bus, phy_addr, MII_MMD_DATA, val);
 }
+EXPORT_SYMBOL(mmd_phy_write);
 
 /**
  * __phy_read_mmd - Convenience function for reading a register
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a98bc91a0cde..10af3e3711fa 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1402,6 +1402,18 @@ int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);
 	__ret; \
 })
 
+/*
+ * mmd_phy_read - Convenience function for direct mdiobus read.
+ */
+int mmd_phy_read(struct mii_bus *bus, int phy_addr, bool is_c45, int devad,
+		 u32 regnum);
+
+/*
+ * mmd_phy_write - Convenience function for direct mdiobus write.
+ */
+int mmd_phy_write(struct mii_bus *bus, int phy_addr, bool is_c45,
+		  int devad, u32 regnum, u16 val);
+
 /*
  * __phy_read_mmd - Convenience function for reading a register
  * from an MMD on a given PHY.
-- 
2.46.0

Re: [net-next 1/2] net: phy: Expose the direct mdiobus access functions

[Jakub Kicinski]

On Fri,  6 Sep 2024 11:39:54 +0200 Niklas Söderlund wrote:
> Expose the direct mdiobus read and write functions. These will be needed
> to refactor the SIOCGMIIREG and SIOCSMIIREG IOCTLs to fallback to
> indirect C45 access if needed.

I'm not sure Andrew is convinced in the sub-thread on patch 2, but also
I don't understand why you need patch 1 at all. The callers and callees
are in the same module are you're adding non-GPL exports, or am I
misreading?
-- 
pw-bot: cr

Re: [net-next 1/2] net: phy: Expose the direct mdiobus access functions

[Russell King (Oracle)]

On Tue, Sep 10, 2024 at 04:19:34PM -0700, Jakub Kicinski wrote:
> On Fri,  6 Sep 2024 11:39:54 +0200 Niklas Söderlund wrote:
> > Expose the direct mdiobus read and write functions. These will be needed
> > to refactor the SIOCGMIIREG and SIOCSMIIREG IOCTLs to fallback to
> > indirect C45 access if needed.
> 
> I'm not sure Andrew is convinced in the sub-thread on patch 2, but also
> I don't understand why you need patch 1 at all. The callers and callees
> are in the same module are you're adding non-GPL exports, or am I
> misreading?

I don't think any of this is required, or even desirable, and I am
of the opinion that falling back to indirect C45 accesses is a bad
thing when this API can be used to access devices other than the
attached PHY that is being used for that decision making.

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

[net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii_ioctl()

[Niklas Söderlund]

If a C45 only PHY is attached to a driver that only knows how to talk
C22 phylib will fallback and use indirect access. This frees the driver
from having to implement this themself.

The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
these convenience functions and instead fail if a C45 PHY is used
together with a driver that only knows how to speak C22.

Fix this by using the two convince functions that knows when to fallback
to indirect access to read/write to the MDIO bus when needed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/net/phy/phy.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4f3e742907cb..89f52bb123aa 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
 			prtad = mdio_phy_id_prtad(mii_data->phy_id);
 			devad = mdio_phy_id_devad(mii_data->phy_id);
-			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
-					       mii_data->reg_num);
 
+			mutex_lock(&phydev->mdio.bus->mdio_lock);
+			ret = mmd_phy_read(phydev->mdio.bus, prtad,
+					   phydev->is_c45, devad,
+					   mii_data->reg_num);
+			mutex_unlock(&phydev->mdio.bus->mdio_lock);
 		} else {
 			ret = mdiobus_read(phydev->mdio.bus, mii_data->phy_id,
 					   mii_data->reg_num);
@@ -403,11 +406,14 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 			}
 		}
 
-		if (mdio_phy_id_is_c45(mii_data->phy_id))
-			mdiobus_c45_write(phydev->mdio.bus, prtad, devad,
-					  mii_data->reg_num, val);
-		else
+		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
+			mutex_lock(&phydev->mdio.bus->mdio_lock);
+			mmd_phy_write(phydev->mdio.bus, prtad, phydev->is_c45,
+				      devad, mii_data->reg_num, val);
+			mutex_unlock(&phydev->mdio.bus->mdio_lock);
+		} else {
 			mdiobus_write(phydev->mdio.bus, prtad, devad, val);
+		}
 
 		if (prtad == phydev->mdio.addr &&
 		    devad == MII_BMCR &&
-- 
2.46.0

Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii_ioctl()

[Andrew Lunn]

On Fri, Sep 06, 2024 at 11:39:55AM +0200, Niklas Söderlund wrote:
> If a C45 only PHY is attached to a driver that only knows how to talk
> C22 phylib will fallback and use indirect access. This frees the driver
> from having to implement this themself.
> 
> The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
> these convenience functions and instead fail if a C45 PHY is used
> together with a driver that only knows how to speak C22.
> 
> Fix this by using the two convince functions that knows when to fallback
> to indirect access to read/write to the MDIO bus when needed.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/net/phy/phy.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 4f3e742907cb..89f52bb123aa 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
>  		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
>  			prtad = mdio_phy_id_prtad(mii_data->phy_id);
>  			devad = mdio_phy_id_devad(mii_data->phy_id);
> -			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
> -					       mii_data->reg_num);
>  
> +			mutex_lock(&phydev->mdio.bus->mdio_lock);
> +			ret = mmd_phy_read(phydev->mdio.bus, prtad,
> +					   phydev->is_c45, devad,

Using phydev->is_c45 is probably wrong.

mii_data->phy_id is the device on the bus you want to access. It does
not need to be the same device as the MAC is using. Just because the
device the MAC is using is a c45 device does not mean the device you
are trying to access is.

Maybe i gave you some bad advice. Sorry.

This API is reasonably well known to be a foot gun. You should only be
using it for debug, and actually using it, even only to read
registers, can mess up a PHY/phylib.

The API gives you the ability to perform a C22 bus transaction, or a
C45 bus transaction on any arbitrary device. That is all you need for
debug, you can do C45 over C22 in user space. Yes, there are race
conditions, but this API already has race conditions, which is part of
why it is a foot gun.

    Andrew

Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii_ioctl()

[Niklas Söderlund]

On 2024-09-06 23:04:50 +0200, Andrew Lunn wrote:
> On Fri, Sep 06, 2024 at 11:39:55AM +0200, Niklas Söderlund wrote:
> > If a C45 only PHY is attached to a driver that only knows how to talk
> > C22 phylib will fallback and use indirect access. This frees the driver
> > from having to implement this themself.
> > 
> > The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
> > these convenience functions and instead fail if a C45 PHY is used
> > together with a driver that only knows how to speak C22.
> > 
> > Fix this by using the two convince functions that knows when to fallback
> > to indirect access to read/write to the MDIO bus when needed.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/net/phy/phy.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 4f3e742907cb..89f52bb123aa 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> >  		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> >  			prtad = mdio_phy_id_prtad(mii_data->phy_id);
> >  			devad = mdio_phy_id_devad(mii_data->phy_id);
> > -			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
> > -					       mii_data->reg_num);
> >  
> > +			mutex_lock(&phydev->mdio.bus->mdio_lock);
> > +			ret = mmd_phy_read(phydev->mdio.bus, prtad,
> > +					   phydev->is_c45, devad,
> 
> Using phydev->is_c45 is probably wrong.
> 
> mii_data->phy_id is the device on the bus you want to access. It does
> not need to be the same device as the MAC is using. Just because the
> device the MAC is using is a c45 device does not mean the device you
> are trying to access is.

I think phydev->is_c45. The outer if clause is checking if the PHY I 
want to talk to is C45, or not. The phydev->is_c45 just express if the 
MAC can talk C45, or if it only supports C22. This is the variable 
mmd_phy_read() uses to determine if it shall fallback to indirect C45.

> 
> Maybe i gave you some bad advice. Sorry.
> 
> This API is reasonably well known to be a foot gun. You should only be
> using it for debug, and actually using it, even only to read
> registers, can mess up a PHY/phylib.
> 
> The API gives you the ability to perform a C22 bus transaction, or a
> C45 bus transaction on any arbitrary device. That is all you need for
> debug, you can do C45 over C22 in user space. Yes, there are race
> conditions, but this API already has race conditions, which is part of
> why it is a foot gun.

I agree it's not the best. But is it not better to have the IOCTLs 
working as they already exists, rather then have it function depending 
on the combination of if the MAC and PHY both speak C22 or C45?

-- 
Kind Regards,
Niklas Söderlund

Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii_ioctl()

[Russell King (Oracle)]

On Fri, Sep 06, 2024 at 11:36:08PM +0200, Niklas Söderlund wrote:
> On 2024-09-06 23:04:50 +0200, Andrew Lunn wrote:
> > On Fri, Sep 06, 2024 at 11:39:55AM +0200, Niklas Söderlund wrote:
> > > If a C45 only PHY is attached to a driver that only knows how to talk
> > > C22 phylib will fallback and use indirect access. This frees the driver
> > > from having to implement this themself.
> > > 
> > > The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
> > > these convenience functions and instead fail if a C45 PHY is used
> > > together with a driver that only knows how to speak C22.
> > > 
> > > Fix this by using the two convince functions that knows when to fallback
> > > to indirect access to read/write to the MDIO bus when needed.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/net/phy/phy.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > > index 4f3e742907cb..89f52bb123aa 100644
> > > --- a/drivers/net/phy/phy.c
> > > +++ b/drivers/net/phy/phy.c
> > > @@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> > >  		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > >  			prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > >  			devad = mdio_phy_id_devad(mii_data->phy_id);
> > > -			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
> > > -					       mii_data->reg_num);
> > >  
> > > +			mutex_lock(&phydev->mdio.bus->mdio_lock);
> > > +			ret = mmd_phy_read(phydev->mdio.bus, prtad,
> > > +					   phydev->is_c45, devad,
> > 
> > Using phydev->is_c45 is probably wrong.
> > 
> > mii_data->phy_id is the device on the bus you want to access. It does
> > not need to be the same device as the MAC is using. Just because the
> > device the MAC is using is a c45 device does not mean the device you
> > are trying to access is.
> 
> I think phydev->is_c45. The outer if clause is checking if the PHY I 
> want to talk to is C45, or not. The phydev->is_c45 just express if the 
> MAC can talk C45, or if it only supports C22. This is the variable 
> mmd_phy_read() uses to determine if it shall fallback to indirect C45.

Nevertheless, this is wrong for two reasons:

1) the MII access API is a raw bus access debugging API, so its up to
userspace to do the necessary accesses. Yes, it's racy... it isn't
supposed to be used for production purposes.

2) the MII access API gives access to any MDIO device on the bus that
the _associated_ PHY is attached to. This includes non-PHY devices.
Just because the PHY that is attached to the netdev *might* support
C45-over-C22 (not every PHY does), this is unique to PHYs and is not
true of every MDIO device. So "upgrading" a C45 access to a
C45-over-C22 could end up corrupting other devices on the bus.

So, I'm afraid that I disagree with your change.

> I agree it's not the best. But is it not better to have the IOCTLs 
> working as they already exists, rather then have it function depending 
> on the combination of if the MAC and PHY both speak C22 or C45?

If the bus supports C22, and userspace asks for a C22 transaction, a
C22 transaction will happen on the bus.

If the bus supports C45, and userspace asks for a C45 transaction, a
C45 transaction will happen on the bus.

Otherwise, an error is returned.

Just because PHYs have this special C45-over-C22 thing, does not mean
that the low level debugging API should use C45-over-C22 for C45
transactions when the bus does not support C45.

I re-iterate, the MII bus access ioctls provide access to any device
on the the bus not specifically the PHY that you have attached to
the netdev. Using the properties of the attached PHY is meaningless
here.

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