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]

Message-ID: <Zcs8LsRrbOfUdIL7@mev-dev>
Date: Tue, 13 Feb 2024 10:53:50 +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 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?

Thanks

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