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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 13 Feb 2024 12:55:07 +0100
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Jiri Pirko <jiri@...nulli.us>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	jacob.e.keller@...el.com, michal.kubiak@...el.com,
	maciej.fijalkowski@...el.com, sridhar.samudrala@...el.com,
	przemyslaw.kitszel@...el.com, wojciech.drewek@...el.com,
	pio.raczynski@...il.com,
	Piotr Raczynski <piotr.raczynski@...el.com>
Subject: Re: [iwl-next v1 07/15] ice: add auxiliary device sfnum attribute

On Tue, Feb 13, 2024 at 12:29:40PM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 10:53:50AM CET, michal.swiatkowski@...ux.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@...ux.intel.com wrote:
> >> >From: Piotr Raczynski <piotr.raczynski@...el.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@...el.com>
> >> >Signed-off-by: Piotr Raczynski <piotr.raczynski@...el.com>
> >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.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
> >> >
> >> >