| Message ID | 20250304190504.3743-5-darinzon@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | PHC support in ENA driver | expand |
> +static ssize_t ena_phc_exp_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct ena_adapter *adapter = dev_get_drvdata(dev); > + > + if (!ena_phc_is_active(adapter)) > + return 0; > + > + return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n", > + adapter->ena_dev->phc.stats.phc_exp); I've not been following previous versions of this patch, so i could be repeating questions already asked.... ena_adapter represents a netdev? /* adapter specific private data structure */ struct ena_adapter { struct ena_com_dev *ena_dev; /* OS defined structs */ struct net_device *netdev; So why are you not using the usual statistics interface for a netdev? Andrew
On Tue, 4 Mar 2025 22:07:39 +0100 Andrew Lunn wrote: > I've not been following previous versions of this patch, so i could be > repeating questions already asked.... > > ena_adapter represents a netdev? > > /* adapter specific private data structure */ > struct ena_adapter { > struct ena_com_dev *ena_dev; > /* OS defined structs */ > struct net_device *netdev; > > So why are you not using the usual statistics interface for a netdev? I asked them to do this. They are using a PTP device as a pure clock. The netdev doesn't support any HW timestamping, so none of the stats are related to packets. The PTP clock could as well be attached to a storage PCIe device. In that way it's more like the OCP PTP driver or other pure clocks in my eyes. And such drivers use sysfs, given they have no netdev.
On Tue, Mar 04, 2025 at 02:58:57PM -0800, Jakub Kicinski wrote: > On Tue, 4 Mar 2025 22:07:39 +0100 Andrew Lunn wrote: > > I've not been following previous versions of this patch, so i could be > > repeating questions already asked.... > > > > ena_adapter represents a netdev? > > > > /* adapter specific private data structure */ > > struct ena_adapter { > > struct ena_com_dev *ena_dev; > > /* OS defined structs */ > > struct net_device *netdev; > > > > So why are you not using the usual statistics interface for a netdev? > > I asked them to do this. > They are using a PTP device as a pure clock. The netdev doesn't support > any HW timestamping, so none of the stats are related to packets. So how intertwined is the PHC with the network device? Can it be separated into a different driver? Moved into drivers/ptp? We have already been asked if this means network drivers can be configured via sysfs. Clearly we don't want that, so we want to get this code out of drivers/net if possible. Andrew
On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote: > > I asked them to do this. > > They are using a PTP device as a pure clock. The netdev doesn't support > > any HW timestamping, so none of the stats are related to packets. > > So how intertwined is the PHC with the network device? Can it be > separated into a different driver? Moved into drivers/ptp? > > We have already been asked if this means network drivers can be > configured via sysfs. Clearly we don't want that, so we want to get > this code out of drivers/net if possible. Is it good enough to move the relevant code to a ptp/ or phc/ dir under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require some weird abstractions, not sure if it's warranted?
On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote: > On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote: > > > I asked them to do this. > > > They are using a PTP device as a pure clock. The netdev doesn't support > > > any HW timestamping, so none of the stats are related to packets. > > > > So how intertwined is the PHC with the network device? Can it be > > separated into a different driver? Moved into drivers/ptp? > > > > We have already been asked if this means network drivers can be > > configured via sysfs. Clearly we don't want that, so we want to get > > this code out of drivers/net if possible. > > Is it good enough to move the relevant code to a ptp/ or phc/ dir > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require > some weird abstractions, not sure if it's warranted? In normal world, where linux kernel driver model is respected, one will write separate driver for PTP and place it under drivers/ptp. This current series doesn't belong to netdev at all. Thanks >
On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote: > On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote: > > > I asked them to do this. > > > They are using a PTP device as a pure clock. The netdev doesn't support > > > any HW timestamping, so none of the stats are related to packets. > > > > So how intertwined is the PHC with the network device? Can it be > > separated into a different driver? Moved into drivers/ptp? > > > > We have already been asked if this means network drivers can be > > configured via sysfs. Clearly we don't want that, so we want to get > > this code out of drivers/net if possible. > > Is it good enough to move the relevant code to a ptp/ or phc/ dir > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require > some weird abstractions, not sure if it's warranted? mtd devices have been doing this for decades. And the auxiliary bus seems to be a reinvention of the mtd concepts. As i said, it comes down to how intertwined the PHC is with the network device. Andrew
On Thu, Mar 06, 2025 at 04:40:55PM +0100, Andrew Lunn wrote: > On Wed, Mar 05, 2025 at 06:36:37PM -0800, Jakub Kicinski wrote: > > On Wed, 5 Mar 2025 16:33:10 +0100 Andrew Lunn wrote: > > > > I asked them to do this. > > > > They are using a PTP device as a pure clock. The netdev doesn't support > > > > any HW timestamping, so none of the stats are related to packets. > > > > > > So how intertwined is the PHC with the network device? Can it be > > > separated into a different driver? Moved into drivers/ptp? > > > > > > We have already been asked if this means network drivers can be > > > configured via sysfs. Clearly we don't want that, so we want to get > > > this code out of drivers/net if possible. > > > > Is it good enough to move the relevant code to a ptp/ or phc/ dir > > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would require > > some weird abstractions, not sure if it's warranted? > > mtd devices have been doing this for decades. And the auxiliary bus > seems to be a reinvention of the mtd concepts. No, it is not. MTD concepts are no different from standard register_to_other_subsystem practice, where driver stays in one subsystem to be used by another. Auxillary bus is different in that it splits drivers to their logical parts and places them in right subsystems. Thanks
> > > > > > I've not been following previous versions of this patch, so i could > > > > > > be repeating questions already asked.... > > > > > > > > > > > > ena_adapter represents a netdev? > > > > > > Yes, ena_adapter represents a netdev. > > > > > > /* adapter specific private data structure */ struct ena_adapter { > > > > > > struct ena_com_dev *ena_dev; > > > > > > /* OS defined structs */ > > > > > > struct net_device *netdev; > > > > > > > > > > > > So why are you not using the usual statistics interface for a netdev? > > > > > > > > > > I asked them to do this. > > > > > They are using a PTP device as a pure clock. The netdev doesn't > > > > > support any HW timestamping, so none of the stats are related to packets. > > > > > > > > So how intertwined is the PHC with the network device? Can it be > > > > separated into a different driver? Moved into drivers/ptp? > > > > The PHC device is not a HW timestamping device but rather a PTP clock that is integrated with the networking device under the same PCI device. Enabling or disabling the ENA PHC requires reconfiguring the ENA network device. > > > > We have already been asked if this means network drivers can be > > > > configured via sysfs. Clearly we don't want that, so we want to > > > > get this code out of drivers/net if possible. > > > > > > Is it good enough to move the relevant code to a ptp/ or phc/ dir > > > under ...thernet/amazon/ena/ ? Moving it to ptp/ proper would > > > require some weird abstractions, not sure if it's warranted? We agree that relocating the PHC code to drivers/ptp would introduce unnecessary abstractions and require synchronization mechanisms between drivers. As with other ENA features, the PHC-related code is contained within a dedicated file in the ethernet/amazon/ena/ directory. Would this be an acceptable approach? Thanks, David > > mtd devices have been doing this for decades. And the auxiliary bus > > seems to be a reinvention of the mtd concepts. > > No, it is not. MTD concepts are no different from standard > register_to_other_subsystem practice, where driver stays in one subsystem > to be used by another. > > Auxillary bus is different in that it splits drivers to their logical parts and places > them in right subsystems. > > Thanks
diff --git a/drivers/net/ethernet/amazon/ena/ena_sysfs.c b/drivers/net/ethernet/amazon/ena/ena_sysfs.c index d0ded92d..441085d2 100644 --- a/drivers/net/ethernet/amazon/ena/ena_sysfs.c +++ b/drivers/net/ethernet/amazon/ena/ena_sysfs.c @@ -65,6 +65,82 @@ static ssize_t ena_phc_enable_get(struct device *dev, static DEVICE_ATTR(phc_enable, S_IRUGO | S_IWUSR, ena_phc_enable_get, ena_phc_enable_set); +/* Takes into account max u64 value, null and new line characters */ +#define ENA_PHC_STAT_MAX_LEN 22 + +static ssize_t ena_phc_cnt_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ena_adapter *adapter = dev_get_drvdata(dev); + + if (!ena_phc_is_active(adapter)) + return 0; + + return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n", + adapter->ena_dev->phc.stats.phc_cnt); +} + +static DEVICE_ATTR(phc_cnt, S_IRUGO, ena_phc_cnt_show, NULL); + +static ssize_t ena_phc_exp_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ena_adapter *adapter = dev_get_drvdata(dev); + + if (!ena_phc_is_active(adapter)) + return 0; + + return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n", + adapter->ena_dev->phc.stats.phc_exp); +} + +static DEVICE_ATTR(phc_exp, S_IRUGO, ena_phc_exp_show, NULL); + +static ssize_t ena_phc_skp_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ena_adapter *adapter = dev_get_drvdata(dev); + + if (!ena_phc_is_active(adapter)) + return 0; + + return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n", + adapter->ena_dev->phc.stats.phc_skp); +} + +static DEVICE_ATTR(phc_skp, S_IRUGO, ena_phc_skp_show, NULL); + +static ssize_t ena_phc_err_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct ena_adapter *adapter = dev_get_drvdata(dev); + + if (!ena_phc_is_active(adapter)) + return 0; + + return snprintf(buf, ENA_PHC_STAT_MAX_LEN, "%llu\n", + adapter->ena_dev->phc.stats.phc_err); +} + +static DEVICE_ATTR(phc_err, S_IRUGO, ena_phc_err_show, NULL); + +static struct attribute *phc_stats_attrs[] = { + &dev_attr_phc_cnt.attr, + &dev_attr_phc_exp.attr, + &dev_attr_phc_skp.attr, + &dev_attr_phc_err.attr, + NULL +}; + +static struct attribute_group phc_stats_group = { + .attrs = phc_stats_attrs, + .name = "phc_stats", +}; + /****************************************************************************** *****************************************************************************/ int ena_sysfs_init(struct device *dev) @@ -72,6 +148,9 @@ int ena_sysfs_init(struct device *dev) if (device_create_file(dev, &dev_attr_phc_enable)) dev_err(dev, "Failed to create phc_enable sysfs entry"); + if (device_add_group(dev, &phc_stats_group)) + dev_err(dev, "Failed to create phc_stats sysfs group"); + return 0; } @@ -80,4 +159,5 @@ int ena_sysfs_init(struct device *dev) void ena_sysfs_terminate(struct device *dev) { device_remove_file(dev, &dev_attr_phc_enable); + device_remove_group(dev, &phc_stats_group); }
The patch allows retrieving PHC statistics through sysfs. In case the feature is not enabled (through `phc_enable` sysfs entry), no output will be written. Signed-off-by: David Arinzon <darinzon@amazon.com> --- drivers/net/ethernet/amazon/ena/ena_sysfs.c | 80 +++++++++++++++++++++ 1 file changed, 80 insertions(+)