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 <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Reviewed-by: Russell King <[email protected]>
---
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 | 123 +++++++++++++++++-
2 files changed, 132 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: [email protected]
+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..a05af6b75c83 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -602,7 +602,128 @@ 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;
+};
+
+static ssize_t phy_c45_id_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct phy_c45_devid_attribute *devattr =
+ container_of(attr, struct phy_c45_devid_attribute, 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 =
+ (struct phy_c45_devid_attribute *)container_of(attr, struct device_attribute, 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
On Fri, Jun 16, 2023 at 09:12:46PM +0800, Jianhui Zhao wrote:
> +static umode_t phy_dev_c45_visible(struct kobject *kobj, struct attribute *attr, int foo)
> +{
> + struct phy_c45_devid_attribute *devattr =
> + (struct phy_c45_devid_attribute *)container_of(attr, struct device_attribute, attr);
1. (struct phy_c45_devid_attribute *) cast is not required.
2. we now have two places that we convert the attribute to a
phy_c45_devid_attribute, it's time for a macro to do that.
> + struct phy_device *phydev = to_phy_device(kobj_to_dev(kobj));
> +
> + if (!phydev->is_c45 || phydev->c45_ids.device_ids[devattr->index] == 0xffffffff)
if (!phydev->is_c45 ||
phydev->c45_ids.device_ids[devattr->index] == 0xffffffff)
And lastly... please don't be so quick to post a new version.
https://www.kernel.org/doc/html/v6.1/process/maintainer-netdev.html#i-have-received-review-feedback-when-should-i-post-a-revised-version-of-the-patches
Particularly the bit about "Do not post a new version of the code if the
discussion about the previous version is still ongoing, unless directly
instructed by a reviewer." is relevent.
You have not given enough time for Andrew to respond to my suggestion
which I invited him to (by "Andrew, any opinions?"), instead rushing
out v4 that implements my suggestion without first waiting to see if
Andrew agrees with it.
This seems to be becoming common place, so I think it's about time I
created a vim macro to insert a boilerplate explaining this process in
every review. :(
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>+ struct phy_c45_devid_attribute *devattr =
>+ container_of(attr, struct phy_c45_devid_attribute, attr);
>+ struct phy_c45_devid_attribute *devattr =
>+ (struct phy_c45_devid_attribute *)container_of(attr, struct device_attribute, attr);
The two conversions is not same.
One is convert "struct device_attribute" to "struct phy_c45_devid_attribute",
and another one is convert "struct attribute" to "struct phy_c45_devid_attribute".
The second one must cast "struct device_attribute" returned from container_of
to "struct phy_c45_devid_attribute".
On Fri, Jun 16, 2023 at 09:54:55PM +0800, Jianhui Zhao wrote:
> >+ struct phy_c45_devid_attribute *devattr =
> >+ container_of(attr, struct phy_c45_devid_attribute, attr);
>
> >+ struct phy_c45_devid_attribute *devattr =
> >+ (struct phy_c45_devid_attribute *)container_of(attr, struct device_attribute, attr);
>
> The two conversions is not same.
You're right...
> One is convert "struct device_attribute" to "struct phy_c45_devid_attribute",
> and another one is convert "struct attribute" to "struct phy_c45_devid_attribute".
> The second one must cast "struct device_attribute" returned from container_of
> to "struct phy_c45_devid_attribute".
... but this isn't entirely correct.
Doing it properly:
struct phy_c45_devid_attribute *devattr =
container_of(attr, struct phy_c45_devid_attribute, attr.attr);
Number one rule with C programming: if you have to cast, you're
probably doing something wrong, so always look to see if there's an
alternative solution that doesn't involve casts.
Casts are one of the reasons why programming errors happen - it stops
the compiler being able to perform proper type checking. Always avoid
them as much as possible.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>Doing it properly:
>
> struct phy_c45_devid_attribute *devattr =
> container_of(attr, struct phy_c45_devid_attribute, attr.attr);
Thanks for the tip!