Re: [Intel-wired-lan] [iwl-next v1 07/15] ice: add auxiliary device sfnum attribute

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Feb 13, 2024 at 09:59:14AM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 08:27:16AM CET, michal.swiatkowski@linux.intel.com wrote:
> >From: Piotr Raczynski <piotr.raczynski@intel.com>
> >
> >Add read only sysfs attribute for each auxiliary subfunction
> >device. This attribute is needed for orchestration layer
> >to distinguish SF devices from each other since there is no
> >native devlink mechanism to represent the connection between
> >devlink instance and the devlink port created for the port
> >representor.
> >
> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> >Signed-off-by: Piotr Raczynski <piotr.raczynski@intel.com>
> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> >---
> > drivers/net/ethernet/intel/ice/ice_sf_eth.c | 31 +++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> >diff --git a/drivers/net/ethernet/intel/ice/ice_sf_eth.c b/drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >index ab90db52a8fc..abee733710a5 100644
> >--- a/drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >+++ b/drivers/net/ethernet/intel/ice/ice_sf_eth.c
> >@@ -224,6 +224,36 @@ static void ice_sf_dev_release(struct device *device)
> > 	kfree(sf_dev);
> > }
> > 
> >+static ssize_t
> >+sfnum_show(struct device *dev, struct device_attribute *attr, char *buf)
> >+{
> >+	struct devlink_port_attrs *attrs;
> >+	struct auxiliary_device *adev;
> >+	struct ice_sf_dev *sf_dev;
> >+
> >+	adev = to_auxiliary_dev(dev);
> >+	sf_dev = ice_adev_to_sf_dev(adev);
> >+	attrs = &sf_dev->dyn_port->devlink_port.attrs;
> >+
> >+	return sysfs_emit(buf, "%u\n", attrs->pci_sf.sf);
> >+}
> >+
> >+static DEVICE_ATTR_RO(sfnum);
> >+
> >+static struct attribute *ice_sf_device_attrs[] = {
> >+	&dev_attr_sfnum.attr,
> >+	NULL,
> >+};
> >+
> >+static const struct attribute_group ice_sf_attr_group = {
> >+	.attrs = ice_sf_device_attrs,
> >+};
> >+
> >+static const struct attribute_group *ice_sf_attr_groups[2] = {
> >+	&ice_sf_attr_group,
> >+	NULL
> >+};
> >+
> > /**
> >  * ice_sf_eth_activate - Activate Ethernet subfunction port
> >  * @dyn_port: the dynamic port instance for this subfunction
> >@@ -262,6 +292,7 @@ ice_sf_eth_activate(struct ice_dynamic_port *dyn_port,
> > 	sf_dev->dyn_port = dyn_port;
> > 	sf_dev->adev.id = id;
> > 	sf_dev->adev.name = "sf";
> >+	sf_dev->adev.dev.groups = ice_sf_attr_groups;
> 
> Ugh. Custom driver sysfs files like this are always very questionable.
> Don't do that please. If you need to expose sfnum, please think about
> some common way. Why exactly you need to expose it?

Uh, hard question. I will drop it and check if it still needed to expose
the sfnum, probably no, as I have never used this sysfs during testing.

Should devlink be used for it?

Thanks

> 
> pw-bot: cr
> 
> 
> > 	sf_dev->adev.dev.release = ice_sf_dev_release;
> > 	sf_dev->adev.dev.parent = &pdev->dev;
> > 
> >-- 
> >2.42.0
> >
> >