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

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

On Tue, Feb 13, 2024 at 12:29:40PM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 10:53:50AM CET, michal.swiatkowski@linux.intel.com wrote:
> >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?
> 
> sfnum is exposed over devlink on the port representor. If you need to
> expose it on the actual SF, we have to figure it out. But again, why?
> 
> 

Only one argument why which I have in my mind is to support it in the legacy
mode. But probably subfunctions shouldn't be supported in legacy mode.
Thanks, I will remove it and use sfnum expose from representor.

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