[PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Jianhui Zhao]

If a phydevice use c45, its phy_id property is always 0, so
this adds a c45_ids sysfs attribute group contains from mmd0
to mmd31 to MDIO devices.
This attribute group can be useful when debugging problems
related to phy drivers.

Likes this:
/sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd0
/sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd1
...
/sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd31

Signed-off-by: Jianhui Zhao <zhaojh329@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Russell King <linux@armlinux.org.uk>
---
V2 -> V3: Use the most efficient implementation.
V1 -> V2: putting all 32 values in a subdirectory, one file per MMD

 .../ABI/testing/sysfs-class-net-phydev        |  10 ++
 drivers/net/phy/phy_device.c                  | 115 +++++++++++++++++-
 2 files changed, 124 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev
index ac722dd5e694..aefddd911b04 100644
--- a/Documentation/ABI/testing/sysfs-class-net-phydev
+++ b/Documentation/ABI/testing/sysfs-class-net-phydev
@@ -63,3 +63,13 @@ Description:
 		only used internally by the kernel and their placement are
 		not meant to be stable across kernel versions. This is intended
 		for facilitating the debugging of PHY drivers.
+
+What:		/sys/class/mdio_bus/<bus>/<device>/c45_ids/mmd<n>
+Date:		November 2023
+KernelVersion:	6.4
+Contact:	netdev@vger.kernel.org
+Description:
+		This attribute group c45_ids contains 32 mmd id attribute from mmd0 to mmd31
+		as reported by the device during bus enumeration, encoded in hexadecimal.
+		This ID is used to match the device with the appropriate
+		driver.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 17d0d0555a79..b1bbbb42f020 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -602,7 +602,120 @@ static struct attribute *phy_dev_attrs[] = {
 	&dev_attr_phy_dev_flags.attr,
 	NULL,
 };
-ATTRIBUTE_GROUPS(phy_dev);
+
+static const struct attribute_group phy_dev_group = {
+	.attrs = phy_dev_attrs
+};
+
+struct phy_c45_devid_attribute {
+	struct device_attribute attr;
+	int index;
+};
+
+#define to_phy_c45_devid_attr(ptr) \
+	container_of(ptr, struct phy_c45_devid_attribute, attr)
+
+static ssize_t phy_c45_id_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct phy_c45_devid_attribute *devattr = to_phy_c45_devid_attr(attr);
+	struct phy_device *phydev = to_phy_device(dev);
+
+	if (!phydev->is_c45)
+		return 0;
+
+	return sprintf(buf, "0x%.8lx\n",
+		(unsigned long)phydev->c45_ids.device_ids[devattr->index]);
+}
+
+#define DEVICE_ATTR_C45_ID(i) \
+static struct phy_c45_devid_attribute dev_attr_phy_c45_id##i = { \
+	.attr = { \
+		.attr = { .name = __stringify(mmd##i), .mode = 0444 }, \
+		.show = phy_c45_id_show \
+	}, \
+	.index = i, \
+}
+
+DEVICE_ATTR_C45_ID(0);
+DEVICE_ATTR_C45_ID(1);
+DEVICE_ATTR_C45_ID(2);
+DEVICE_ATTR_C45_ID(3);
+DEVICE_ATTR_C45_ID(4);
+DEVICE_ATTR_C45_ID(5);
+DEVICE_ATTR_C45_ID(6);
+DEVICE_ATTR_C45_ID(7);
+DEVICE_ATTR_C45_ID(8);
+DEVICE_ATTR_C45_ID(9);
+DEVICE_ATTR_C45_ID(10);
+DEVICE_ATTR_C45_ID(11);
+DEVICE_ATTR_C45_ID(12);
+DEVICE_ATTR_C45_ID(13);
+DEVICE_ATTR_C45_ID(14);
+DEVICE_ATTR_C45_ID(15);
+DEVICE_ATTR_C45_ID(16);
+DEVICE_ATTR_C45_ID(17);
+DEVICE_ATTR_C45_ID(18);
+DEVICE_ATTR_C45_ID(19);
+DEVICE_ATTR_C45_ID(20);
+DEVICE_ATTR_C45_ID(21);
+DEVICE_ATTR_C45_ID(22);
+DEVICE_ATTR_C45_ID(23);
+DEVICE_ATTR_C45_ID(24);
+DEVICE_ATTR_C45_ID(25);
+DEVICE_ATTR_C45_ID(26);
+DEVICE_ATTR_C45_ID(27);
+DEVICE_ATTR_C45_ID(28);
+DEVICE_ATTR_C45_ID(29);
+DEVICE_ATTR_C45_ID(30);
+DEVICE_ATTR_C45_ID(31);
+
+static struct attribute *phy_c45_id_attrs[] = {
+	&dev_attr_phy_c45_id0.attr.attr,
+	&dev_attr_phy_c45_id1.attr.attr,
+	&dev_attr_phy_c45_id2.attr.attr,
+	&dev_attr_phy_c45_id3.attr.attr,
+	&dev_attr_phy_c45_id4.attr.attr,
+	&dev_attr_phy_c45_id5.attr.attr,
+	&dev_attr_phy_c45_id6.attr.attr,
+	&dev_attr_phy_c45_id7.attr.attr,
+	&dev_attr_phy_c45_id8.attr.attr,
+	&dev_attr_phy_c45_id9.attr.attr,
+	&dev_attr_phy_c45_id10.attr.attr,
+	&dev_attr_phy_c45_id11.attr.attr,
+	&dev_attr_phy_c45_id12.attr.attr,
+	&dev_attr_phy_c45_id13.attr.attr,
+	&dev_attr_phy_c45_id14.attr.attr,
+	&dev_attr_phy_c45_id15.attr.attr,
+	&dev_attr_phy_c45_id16.attr.attr,
+	&dev_attr_phy_c45_id17.attr.attr,
+	&dev_attr_phy_c45_id18.attr.attr,
+	&dev_attr_phy_c45_id19.attr.attr,
+	&dev_attr_phy_c45_id20.attr.attr,
+	&dev_attr_phy_c45_id21.attr.attr,
+	&dev_attr_phy_c45_id22.attr.attr,
+	&dev_attr_phy_c45_id23.attr.attr,
+	&dev_attr_phy_c45_id24.attr.attr,
+	&dev_attr_phy_c45_id25.attr.attr,
+	&dev_attr_phy_c45_id26.attr.attr,
+	&dev_attr_phy_c45_id27.attr.attr,
+	&dev_attr_phy_c45_id28.attr.attr,
+	&dev_attr_phy_c45_id29.attr.attr,
+	&dev_attr_phy_c45_id30.attr.attr,
+	&dev_attr_phy_c45_id31.attr.attr,
+	NULL,
+};
+
+static const struct attribute_group phy_dev_c45_ids_group = {
+	.name = "c45_ids",
+	.attrs = phy_c45_id_attrs
+};
+
+static const struct attribute_group *phy_dev_groups[] = {
+	&phy_dev_group,
+	&phy_dev_c45_ids_group,
+	NULL,
+};
 
 static const struct device_type mdio_bus_phy_type = {
 	.name = "PHY",
-- 
2.34.1

Re: [PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Jakub Kicinski]

On Wed, 14 Jun 2023 21:45:22 +0800 Jianhui Zhao wrote:
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Russell King <linux@armlinux.org.uk>

Did Andrew and Russell give review tags on the list ?
I don't see any given to v2.

Re: [PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Russell King (Oracle)]

On Wed, Jun 14, 2023 at 09:45:22PM +0800, Jianhui Zhao wrote:
> +static const struct attribute_group phy_dev_c45_ids_group = {
> +	.name = "c45_ids",
> +	.attrs = phy_c45_id_attrs
> +};

One last thing - is there any point to creating these attributes if
the PHY isn't c45?

We could add here:

	.is_visible = phy_dev_c45_visible,

with:

static umode_t phy_dev_c45_visible(struct kobject *kobj,
				   struct attribute *attr, int foo)
{
	struct phy_device *phydev = to_phy_device(kobj_to_dev(kobj));

	return phydev->is_c45 ? attr->mode : 0;
}

which will only show the c45 attributes if the PHY is a c45 PHY.

Andrew, any opinions?

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

Re: [PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Russell King (Oracle)]

On Thu, Jun 15, 2023 at 10:41:04PM -0700, Jakub Kicinski wrote:
> On Wed, 14 Jun 2023 21:45:22 +0800 Jianhui Zhao wrote:
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Reviewed-by: Russell King <linux@armlinux.org.uk>
> 
> Did Andrew and Russell give review tags on the list ?
> I don't see any given to v2.

Well spotted, I did not. I merely made a suggestion that the author has
picked up. The tel-tale is that my r-b line doesn't include "(Oracle)"
in this instance. That r-b will need to be removed whatever the outcome
of my further suggestion I've just sent.

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

Re: [PATCH V3] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Andrew Lunn]

On Fri, Jun 16, 2023 at 08:49:41AM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 14, 2023 at 09:45:22PM +0800, Jianhui Zhao wrote:
> > +static const struct attribute_group phy_dev_c45_ids_group = {
> > +	.name = "c45_ids",
> > +	.attrs = phy_c45_id_attrs
> > +};
> 
> One last thing - is there any point to creating these attributes if
> the PHY isn't c45?
> 
> We could add here:
> 
> 	.is_visible = phy_dev_c45_visible,
> 
> with:
> 
> static umode_t phy_dev_c45_visible(struct kobject *kobj,
> 				   struct attribute *attr, int foo)
> {
> 	struct phy_device *phydev = to_phy_device(kobj_to_dev(kobj));
> 
> 	return phydev->is_c45 ? attr->mode : 0;
> }
> 
> which will only show the c45 attributes if the PHY is a c45 PHY.
> 
> Andrew, any opinions?

There are PHYs which get detected via their C22 ID, but the driver
then uses C45. So phydev->is_c45 could be false yet the device does
have C45 IDs. But i don't see a good solution to this. If the point of
these values is to aid debugging matching devices to drivers, this
does not really matter. Its C22 ID is what will be used, and that
sysfs file will be populated.

So this .is_visible seems reasonable.

I suppose there is the flip condition. Do we want the C22 sysfs file
visible if there is no C22 ID? But that is probably ABI, we cannot
change it now.

   Andrew