> -----Original Message-----
> From: Jiri Pirko <j...@resnulli.us>
> Sent: Wednesday, February 14, 2024 12:45 AM
> To: Keller, Jacob E <jacob.e.kel...@intel.com>
> Cc: Michal Swiatkowski <michal.swiatkow...@linux.intel.com>; intel-wired-
> l...@lists.osuosl.org; net...@vger.kernel.org; Kubiak, Michal
> <michal.kub...@intel.com>; Fijalkowski, Maciej <maciej.fijalkow...@intel.com>;
> Samudrala, Sridhar <sridhar.samudr...@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kits...@intel.com>; Drewek, Wojciech
> <wojciech.dre...@intel.com>; pio.raczyn...@gmail.com; Piotr Raczynski
> <piotr.raczyn...@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.kel...@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.swiatkow...@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.swiatkow...@linux.intel.com
> wrote:
> >>>>>> From: Piotr Raczynski <piotr.raczyn...@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.dre...@intel.com>
> >>>>>> Signed-off-by: Piotr Raczynski <piotr.raczyn...@intel.com>
> >>>>>> Signed-off-by: Michal Swiatkowski <michal.swiatkow...@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
