| Message ID | 20220419083704.48573-2-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Extend sysfs to adjust PHY latency. | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 416 this patch: 416 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 297 this patch: 297 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 401 this patch: 401 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 104 lines checked |
| netdev/kdoc | fail | Errors and warnings before: 0 this patch: 1 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Apr 19, 2022 at 10:37:03AM +0200, Horatiu Vultur wrote: > Add adjustment support for latency for the phy using sysfs. > This is used to adjust the latency of the phy based on link mode > and direction. > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > .../ABI/testing/sysfs-class-net-phydev | 10 ++++ > drivers/net/phy/phy_device.c | 58 +++++++++++++++++++ > include/linux/phy.h | 9 +++ > 3 files changed, 77 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev > index ac722dd5e694..a99bbfeddb6f 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>/adjust_latency > +Date: April 2022 > +KernelVersion: 5.19 > +Contact: netdev@vger.kernel.org > +Description: > + This file adjusts the latency in the PHY. To set value, > + write three integers into the file: interface mode, RX latency, > + TX latency. When the file is read, it returns the supported > + interface modes and the latency values. https://lwn.net/Articles/378884/ The key design goal relating to attribute files is the stipulation - almost a mantra - of "one file, one value" or sometimes "one item per file". The idea here is that each attribute file should contain precisely one value. If multiple values are needed, then multiple files should be used. You also need to specify units in the documentation. > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 8406ac739def..80bf04ca0e02 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -529,6 +529,48 @@ static ssize_t phy_dev_flags_show(struct device *dev, > } > static DEVICE_ATTR_RO(phy_dev_flags); > > +static ssize_t adjust_latency_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct phy_device *phydev = to_phy_device(dev); > + ssize_t count = 0; > + int err, i; > + s32 rx, tx; > + > + for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; ++i) { > + err = phydev->drv->get_adj_latency(phydev, i, &rx, &tx); > + if (err == -EINVAL) -EOPNOTSUPP seems more likley than EINVAL. > + continue; > + > + count += sprintf(&buf[count], "%d rx: %d, tx: %d\n", i, rx, tx); Link modes as a number? Can you tell me off the top of you head what link mode 42 is? Also, if phydev->drv->get_adj_latency() returns any other error, it is likely rx and tx contain rubbish, yet you still add them to the output. > @@ -932,6 +932,15 @@ struct phy_driver { > int (*get_sqi)(struct phy_device *dev); > /** @get_sqi_max: Get the maximum signal quality indication */ > int (*get_sqi_max)(struct phy_device *dev); > + /** @set_adj_latency: Set the latency values of the PHY */ > + int (*set_adj_latency)(struct phy_device *dev, > + enum ethtool_link_mode_bit_indices link_mode, > + s32 rx, s32 tx); > + /** @get_latency: Get the latency values of the PHY */ > + int (*get_adj_latency)(struct phy_device *dev, > + enum ethtool_link_mode_bit_indices link_mode, > + s32 *rx, s32 *tx); You need to clearly document the return value here, that -EINVAL (or maybe -EOPNOTUSPP) has special meaning. I would also document the units, and what is supposed to happen if the stamper already has a hard coded offset. Andrew
diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev index ac722dd5e694..a99bbfeddb6f 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>/adjust_latency +Date: April 2022 +KernelVersion: 5.19 +Contact: netdev@vger.kernel.org +Description: + This file adjusts the latency in the PHY. To set value, + write three integers into the file: interface mode, RX latency, + TX latency. When the file is read, it returns the supported + interface modes and the latency values. diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 8406ac739def..80bf04ca0e02 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -529,6 +529,48 @@ static ssize_t phy_dev_flags_show(struct device *dev, } static DEVICE_ATTR_RO(phy_dev_flags); +static ssize_t adjust_latency_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct phy_device *phydev = to_phy_device(dev); + ssize_t count = 0; + int err, i; + s32 rx, tx; + + for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; ++i) { + err = phydev->drv->get_adj_latency(phydev, i, &rx, &tx); + if (err == -EINVAL) + continue; + + count += sprintf(&buf[count], "%d rx: %d, tx: %d\n", i, rx, tx); + } + + return count; +} + +static ssize_t adjust_latency_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct phy_device *phydev = to_phy_device(dev); + enum ethtool_link_mode_bit_indices link_mode; + int cnt, err = -EINVAL; + s32 rx, tx; + + cnt = sscanf(buf, "%u %d %d", &link_mode, &rx, &tx); + if (cnt != 3) + goto out; + + err = phydev->drv->set_adj_latency(phydev, link_mode, rx, tx); + if (err) + goto out; + + return count; +out: + return err; +} +static DEVICE_ATTR_RW(adjust_latency); + static struct attribute *phy_dev_attrs[] = { &dev_attr_phy_id.attr, &dev_attr_phy_interface.attr, @@ -3009,6 +3051,16 @@ static int phy_probe(struct device *dev) phydev->drv = phydrv; + if (phydev->drv && + phydev->drv->set_adj_latency && + phydev->drv->get_adj_latency) { + err = sysfs_create_file(&phydev->mdio.dev.kobj, + &dev_attr_adjust_latency.attr); + if (err) + phydev_err(phydev, "error creating 'adjust_latency' sysfs entry\n"); + err = 0; + } + /* Disable the interrupt if the PHY doesn't support it * but the interrupt is still a valid one */ @@ -3112,6 +3164,12 @@ static int phy_remove(struct device *dev) if (phydev->drv && phydev->drv->remove) phydev->drv->remove(phydev); + if (phydev->drv && + phydev->drv->set_adj_latency && + phydev->drv->get_adj_latency) + sysfs_remove_file(&phydev->mdio.dev.kobj, + &dev_attr_adjust_latency.attr); + /* Assert the reset signal */ phy_device_reset(phydev, 1); diff --git a/include/linux/phy.h b/include/linux/phy.h index 36ca2b5c2253..584e467ff4d1 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -932,6 +932,15 @@ struct phy_driver { int (*get_sqi)(struct phy_device *dev); /** @get_sqi_max: Get the maximum signal quality indication */ int (*get_sqi_max)(struct phy_device *dev); + /** @set_adj_latency: Set the latency values of the PHY */ + int (*set_adj_latency)(struct phy_device *dev, + enum ethtool_link_mode_bit_indices link_mode, + s32 rx, s32 tx); + /** @get_latency: Get the latency values of the PHY */ + int (*get_adj_latency)(struct phy_device *dev, + enum ethtool_link_mode_bit_indices link_mode, + s32 *rx, s32 *tx); + }; #define to_phy_driver(d) container_of(to_mdio_common_driver(d), \ struct phy_driver, mdiodrv)
Add adjustment support for latency for the phy using sysfs. This is used to adjust the latency of the phy based on link mode and direction. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- .../ABI/testing/sysfs-class-net-phydev | 10 ++++ drivers/net/phy/phy_device.c | 58 +++++++++++++++++++ include/linux/phy.h | 9 +++ 3 files changed, 77 insertions(+)