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

[PATCH V5] 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 mmd id
attributes from mmd0 to mmd31 to MDIO devices. Note that only
mmd with valid value will exist. 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/mmd1
/sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd2
...
/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>
---
V4 -> V5: Remove cast operation and use macro to convert attribute to a phy_c45_devid_attribute.
V3 -> V4: Only mmd with valid value will exist.
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                  | 124 +++++++++++++++++-
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev
index ac722dd5e694..58a0a150229d 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 mmd id attributes from mmd0 to mmd31
+		as reported by the device during bus enumeration, encoded in hexadecimal.
+		Note that only mmd with valid value will exist. 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..39f663c2ad1a 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -602,7 +602,129 @@ 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.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->attr);
+	struct phy_device *phydev = to_phy_device(dev);
+
+	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 umode_t phy_dev_c45_visible(struct kobject *kobj, struct attribute *attr, int foo)
+{
+	struct phy_c45_devid_attribute *devattr = to_phy_c45_devid_attr(attr);
+	struct phy_device *phydev = to_phy_device(kobj_to_dev(kobj));
+
+	if (!phydev->is_c45 || phydev->c45_ids.device_ids[devattr->index] == 0xffffffff)
+		return 0;
+
+	return attr->mode;
+}
+
+static const struct attribute_group phy_dev_c45_ids_group = {
+	.name = "c45_ids",
+	.attrs = phy_c45_id_attrs,
+	.is_visible = phy_dev_c45_visible
+};
+
+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 V5] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Andrew Lunn]

On Fri, Jun 16, 2023 at 10:40:17PM +0800, Jianhui Zhao wrote:
> If a phydevice use c45, its phy_id property is always 0, so
> this adds a c45_ids sysfs attribute group contains mmd id
> attributes from mmd0 to mmd31 to MDIO devices. Note that only
> mmd with valid value will exist. 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/mmd1
> /sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd2
> ...
> /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>

Did you read Russells comments? And Jakubs? Please don't ignore them.

    Andrew

---
pw-bot: cr

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

[Russell King (Oracle)]

On Fri, Jun 16, 2023 at 10:40:17PM +0800, Jianhui Zhao wrote:
> If a phydevice use c45, its phy_id property is always 0, so
> this adds a c45_ids sysfs attribute group contains mmd id
> attributes from mmd0 to mmd31 to MDIO devices. Note that only
> mmd with valid value will exist. 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/mmd1
> /sys/bus/mdio_bus/devices/mdio-bus:05/c45_ids/mmd2
> ...
> /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>

NAK, because you haven't listened to all of the feedback that's been
given to you.

1. You haven't taken on my suggestion for reformatting the if().
2. You haven't updated my Reviewed-by, which in any case was conditional
   on the acceptance of all of my review comments.
3. You haven't removed Andrew's reviewed-by which he hasn't given you,
   and thus your commit message is a lie.
4. You haven't obeyed the "do not repost in under 24 hours"

I do not expect to see you post a V6 before at the very earliest Monday,
and if you do, I will NAK it purely on that point, and you will get an
even stronger email from me.

Stop adding unnecessarily to reviewers workloads.

Thanks.

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