[PATCH] net: phy: update fields of mii_ioctl_data for transferring C45 data.

[PATCH] net: phy: update fields of mii_ioctl_data for transferring C45 data.

[renjun wang]

The phy_id is used as u32 type in function mdio_phy_id_is_c45()
with the 30th bit for distinguishing C22 and C45. The reg_num is
also used as u32 type in function mdiobus_c45_read() or someplace
else. For more C45 information needed and data structure alignment
consideration, change these two fields to __u32 type which can make
user space program transferring clause 45 type information to kernel
directly.

Signed-off-by: renjun wang <renjunw0@foxmail.com>
---
 include/uapi/linux/mii.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
index 39f7c44baf53..68c085b049de 100644
--- a/include/uapi/linux/mii.h
+++ b/include/uapi/linux/mii.h
@@ -176,8 +176,8 @@
 
 /* This structure is used in all SIOCxMIIxxx ioctl calls */
 struct mii_ioctl_data {
-	__u16		phy_id;
-	__u16		reg_num;
+	__u32		phy_id;
+	__u32		reg_num;
 	__u16		val_in;
 	__u16		val_out;
 };
-- 
2.39.2

Re: [PATCH] net: phy: update fields of mii_ioctl_data for transferring C45 data.

[Heiner Kallweit]

On 20.04.2024 02:41, renjun wang wrote:
> The phy_id is used as u32 type in function mdio_phy_id_is_c45()
> with the 30th bit for distinguishing C22 and C45. The reg_num is
> also used as u32 type in function mdiobus_c45_read() or someplace
> else. For more C45 information needed and data structure alignment
> consideration, change these two fields to __u32 type which can make

What do you mean with alignment consideration?

> user space program transferring clause 45 type information to kernel
> directly.
> 

With this change you break userspace. And in general: If you make
such a change, you should also use it.

> Signed-off-by: renjun wang <renjunw0@foxmail.com>
> ---
>  include/uapi/linux/mii.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h
> index 39f7c44baf53..68c085b049de 100644
> --- a/include/uapi/linux/mii.h
> +++ b/include/uapi/linux/mii.h
> @@ -176,8 +176,8 @@
>  
>  /* This structure is used in all SIOCxMIIxxx ioctl calls */
>  struct mii_ioctl_data {
> -	__u16		phy_id;
> -	__u16		reg_num;
> +	__u32		phy_id;
> +	__u32		reg_num;
>  	__u16		val_in;
>  	__u16		val_out;
>  };

Re: [PATCH] net: phy: update fields of mii_ioctl_data for transferring C45 data.

[Russell King (Oracle)]

On Sat, Apr 20, 2024 at 08:41:10AM +0800, renjun wang wrote:
> The phy_id is used as u32 type in function mdio_phy_id_is_c45()
> with the 30th bit for distinguishing C22 and C45. The reg_num is
> also used as u32 type in function mdiobus_c45_read() or someplace
> else. For more C45 information needed and data structure alignment
> consideration, change these two fields to __u32 type which can make
> user space program transferring clause 45 type information to kernel
> directly.

You haven't understood the user API, and are making a change that will
break existing userspace users of this API. Hence this is not
acceptable.

mdio_phy_id_is_c45() is indeed for determining if the "phy_id" field
in mii_ioctl_data is indicating a clause 45 transaction. This checks
the following bits, defined using these macros:

#define MDIO_PHY_ID_C45                 0x8000
#define MDIO_PHY_ID_PRTAD               0x03e0
#define MDIO_PHY_ID_DEVAD               0x001f

and all of these fit within the __u16.

The format used by userspace pre-dates clause 45, and bit 15 was
added to indicate clause 45 along with the "prtad" in bits 9 to
5.

However, the kernel translates this data structure into something
that the MDIO accessors can deal with. See for example phy_mii_ioctl().

Also note that the old method of shoe-horning clause 45 into just
one set of mii bus methods has gone, meaning there is now no need
to convert from mii_ioctl_data into a 32-bit reg_num.

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

Re: [PATCH] net: phy: update fields of mii_ioctl_data for transferring C45 data.

[Andrew Lunn]

On Sat, Apr 20, 2024 at 08:41:10AM +0800, renjun wang wrote:
> The phy_id is used as u32 type in function mdio_phy_id_is_c45()
> with the 30th bit for distinguishing C22 and C45. The reg_num is
> also used as u32 type in function mdiobus_c45_read() or someplace
> else. For more C45 information needed and data structure alignment
> consideration, change these two fields to __u32 type which can make
> user space program transferring clause 45 type information to kernel
> directly.
> 
> Signed-off-by: renjun wang <renjunw0@foxmail.com>
> ---
>  include/uapi/linux/mii.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h

Just adding a general point to others comments. You have to be very
careful with changes to files inside include/uapi. These define the
API between user space and the kernel. You cannot make changes which
break existing binaries of user space tools.

Sometimes you can add new members to the end of a structure. Sometimes
you can add new enum values after all other enums, but you cannot make
changes in the middle.


    Andrew

---
pw-bot: cr