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

[PATCH V2] 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>
---
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                  | 103 +++++++++++++++++-
 2 files changed, 112 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..c09282818d45 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -602,7 +602,108 @@ 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
+};
+
+#define DEVICE_ATTR_C45_ID(i) \
+static ssize_t \
+phy_c45_id##i##_show(struct device *dev, \
+	struct device_attribute *attr, char *buf) \
+{ \
+	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[i]); \
+} \
+static struct device_attribute dev_attr_phy_c45_id##i = { \
+	.attr	= { .name = __stringify(mmd##i), .mode = 0444 }, \
+	.show	= phy_c45_id##i##_show \
+}
+
+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,
+	&dev_attr_phy_c45_id1.attr,
+	&dev_attr_phy_c45_id2.attr,
+	&dev_attr_phy_c45_id3.attr,
+	&dev_attr_phy_c45_id4.attr,
+	&dev_attr_phy_c45_id5.attr,
+	&dev_attr_phy_c45_id6.attr,
+	&dev_attr_phy_c45_id7.attr,
+	&dev_attr_phy_c45_id8.attr,
+	&dev_attr_phy_c45_id9.attr,
+	&dev_attr_phy_c45_id10.attr,
+	&dev_attr_phy_c45_id11.attr,
+	&dev_attr_phy_c45_id12.attr,
+	&dev_attr_phy_c45_id13.attr,
+	&dev_attr_phy_c45_id14.attr,
+	&dev_attr_phy_c45_id15.attr,
+	&dev_attr_phy_c45_id16.attr,
+	&dev_attr_phy_c45_id17.attr,
+	&dev_attr_phy_c45_id18.attr,
+	&dev_attr_phy_c45_id19.attr,
+	&dev_attr_phy_c45_id20.attr,
+	&dev_attr_phy_c45_id21.attr,
+	&dev_attr_phy_c45_id22.attr,
+	&dev_attr_phy_c45_id23.attr,
+	&dev_attr_phy_c45_id24.attr,
+	&dev_attr_phy_c45_id25.attr,
+	&dev_attr_phy_c45_id26.attr,
+	&dev_attr_phy_c45_id27.attr,
+	&dev_attr_phy_c45_id28.attr,
+	&dev_attr_phy_c45_id29.attr,
+	&dev_attr_phy_c45_id30.attr,
+	&dev_attr_phy_c45_id31.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 V2] net: phy: Add sysfs attribute for PHY c45 identifiers.

[Andrew Lunn]

> +#define DEVICE_ATTR_C45_ID(i) \
> +static ssize_t \
> +phy_c45_id##i##_show(struct device *dev, \
> +	struct device_attribute *attr, char *buf) \
> +{ \
> +	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[i]); \
> +} \

That is not the most efficient implementation.

You can have one generic

static ssize_t phy_c45_id_show(struct device *dev, char *buf, int i)
{
	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[i]);
}

And then your macros becomes

#define DEVICE_ATTR_C45_ID(i)			  \
static ssize_t					  \
phy_c45_id##i##_show(struct device *dev,	  \
	struct device_attribute *attr, char *buf) \
{						  \
	return phy_c45_id_show(dev, buf, i);	  \
}

	Andrew

---
pw-bot: cr

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

[Russell King (Oracle)]

On Tue, Jun 13, 2023 at 06:18:58PM +0200, Andrew Lunn wrote:
> > +#define DEVICE_ATTR_C45_ID(i) \
> > +static ssize_t \
> > +phy_c45_id##i##_show(struct device *dev, \
> > +	struct device_attribute *attr, char *buf) \
> > +{ \
> > +	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[i]); \
> > +} \
> 
> That is not the most efficient implementation.
> 
> You can have one generic
> 
> static ssize_t phy_c45_id_show(struct device *dev, char *buf, int i)
> {
> 	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[i]);
> }
> 
> And then your macros becomes
> 
> #define DEVICE_ATTR_C45_ID(i)			  \
> static ssize_t					  \
> phy_c45_id##i##_show(struct device *dev,	  \
> 	struct device_attribute *attr, char *buf) \
> {						  \
> 	return phy_c45_id_show(dev, buf, i);	  \
> }
> 

I have a further suggestion, which I think will result in yet more
efficiencies:

struct phy_c45_devid_attribute {
	struct device_attribute attr;
	int index;
};

#define to_phy_c45_devid_attr(attr) \
	container_of(attr, 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);
	unsigned long id;

	if (!phydev->is_c45)
		return 0;

	id = phydev->c45_ids.device_ids[devattr->index];

	return sprintf(buf, "0x%.8lx\n", id);
}

#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, \
}

which will probably result in less code size for a little larger data
size. Note that the references to this would need to be:

	&dev_attr_phy_c45_id1.attr.attr

in the array (adding an extra .attr).

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