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

[Jiri Pirko <jiri@resnulli.us>]

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