| Message ID | 20240213072724.77275-8-michal.swiatkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: support devlink subfunctions | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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? pw-bot: cr > sf_dev->adev.dev.release = ice_sf_dev_release; > sf_dev->adev.dev.parent = &pdev->dev; > >-- >2.42.0 > >
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 > > > >
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? > >Thanks > >> >> pw-bot: cr >> >> >> > sf_dev->adev.dev.release = ice_sf_dev_release; >> > sf_dev->adev.dev.parent = &pdev->dev; >> > >> >-- >> >2.42.0 >> > >> >
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 > >> > > >> >
On 2/13/2024 3:55 AM, Michal Swiatkowski wrote: > 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? >> >> I vaguely remember some internal discussion about orchestration software wanting to know which subfunction was associated with which auxiliary device. However, I think a much better solution would be to expose the auxiliary device ID out of devlink_port instead, through devlink port. I can't find any notes on this and it was quite some time ago so maybe things have changed. If we enable support for user-space configurable sfnum, then we can just have the orchestration software pick its sfnum (or check the netlink return value from the port add), so probably this is not that useful.
Tue, Feb 13, 2024 at 11:04:00PM CET, jacob.e.keller@intel.com wrote: > > >On 2/13/2024 3:55 AM, Michal Swiatkowski wrote: >> 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? >>> >>> > >I vaguely remember some internal discussion about orchestration software >wanting to know which subfunction was associated with which auxiliary >device. However, I think a much better solution would be to expose the >auxiliary device ID out of devlink_port instead, through devlink port. > >I can't find any notes on this and it was quite some time ago so maybe >things have changed. > >If we enable support for user-space configurable sfnum, then we can just >have the orchestration software pick its sfnum (or check the netlink >return value from the port add), so probably this is not that useful. This is already solved by nested devlink. When you properly call devl_port_fn_devlink_set(), you link the SF devlink instance with the eswitch port representor. Then the user sees: $ devlink port pci/0000:08:00.1/98304: type eth netdev eth4 flavour pcisf controller 0 pfnum 1 sfnum 109 splittable false function: hw_addr 00:00:00:00:00:00 state active opstate attached roce enable nested_devlink: auxiliary/mlx5_core.sf.2
> -----Original Message----- > From: Jiri Pirko <jiri@resnulli.us> > Sent: Wednesday, February 14, 2024 12:45 AM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>; intel-wired- > lan@lists.osuosl.org; netdev@vger.kernel.org; Kubiak, Michal > <michal.kubiak@intel.com>; Fijalkowski, Maciej <maciej.fijalkowski@intel.com>; > Samudrala, Sridhar <sridhar.samudrala@intel.com>; Kitszel, Przemyslaw > <przemyslaw.kitszel@intel.com>; Drewek, Wojciech > <wojciech.drewek@intel.com>; pio.raczynski@gmail.com; Piotr Raczynski > <piotr.raczynski@intel.com> > Subject: Re: [iwl-next v1 07/15] ice: add auxiliary device sfnum attribute > > Tue, Feb 13, 2024 at 11:04:00PM CET, jacob.e.keller@intel.com wrote: > > > > > >On 2/13/2024 3:55 AM, Michal Swiatkowski wrote: > >> 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? > >>> > >>> > > > >I vaguely remember some internal discussion about orchestration software > >wanting to know which subfunction was associated with which auxiliary > >device. However, I think a much better solution would be to expose the > >auxiliary device ID out of devlink_port instead, through devlink port. > > > >I can't find any notes on this and it was quite some time ago so maybe > >things have changed. > > > >If we enable support for user-space configurable sfnum, then we can just > >have the orchestration software pick its sfnum (or check the netlink > >return value from the port add), so probably this is not that useful. > > This is already solved by nested devlink. When you properly call > devl_port_fn_devlink_set(), you link the SF devlink instance with the > eswitch port representor. Then the user sees: > > $ devlink port > pci/0000:08:00.1/98304: type eth netdev eth4 flavour pcisf controller 0 pfnum 1 > sfnum 109 splittable false > function: > hw_addr 00:00:00:00:00:00 state active opstate attached roce enable > nested_devlink: > auxiliary/mlx5_core.sf.2 > Excellent. That looks to resolve this entirely then. Thanks, Jake
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; sf_dev->adev.dev.release = ice_sf_dev_release; sf_dev->adev.dev.parent = &pdev->dev;