Re: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute

[Jacob Keller <jacob.e.keller@intel.com>]

On 9/17/2020 8:54 PM, Parav Pandit wrote:
> 
> 
>> From: Jacob Keller <jacob.e.keller@intel.com>
>> Sent: Friday, September 18, 2020 12:00 AM
>>
>>
>> On 9/17/2020 10:20 AM, Parav Pandit wrote:
>>> A PCI sub-function (SF) represents a portion of the device similar to
>>> PCI VF.
>>>
>>> In an eswitch, PCI SF may have port which is normally represented
>>> using a representor netdevice.
>>> To have better visibility of eswitch port, its association with SF,
>>> and its representor netdevice, introduce a PCI SF port flavour.
>>>
>>> When devlink port flavour is PCI SF, fill up PCI SF attributes of the
>>> port.
>>>
>>> Extend port name creation using PCI PF and SF number scheme on best
>>> effort basis, so that vendor drivers can skip defining their own
>>> scheme.
>>
>> What does this mean? What's the scheme used? 
>>
> Scheme used is equivalent as what is used for PCI VF ports. pfNvfM.
> It is pfNsfM.
> Below example shows the representor netdevice name as 'eni10npf0sf44' built by systemd/udev using phys_port_name.
> 
>> Do drivers still have the option to make their own scheme? If so, why?
> Today we have two types of drivers (mlx5_core, netdevsim) which uses devlink core which creates the name.
> Or other drivers (bnxt, nfp) which doesn't yet migrated to use devlink infra for PCI PF, VF ports.
> Such drivers are phys_port_name and other ndos.
> It is not the role of this patch to block those drivers, but any new implementation doesn't need to hand code switch_id and phys_port_name related ndos for SF.
> For example, bnxt_vf_rep_get_phys_port_name().
> 


Ok, thanks for the explanation.

>> It's not obvious to me in this patch where the numbering scheme comes from. It
>> looks like it's still up to the caller to set the numbers.
>>
> Naming scheme for PCI PF and PCI VF port flavours already exist.
> Scheme is equivalent for PCI SF flavour.
> 
> I thought example is good enough to show that, but I will update commit message to describe this scheme to make it clear. pfNsfM.
>  

I think I just hadn't quite moved from "sf number" to "name of the
netdevice" and was thinking of scheme for how the sf number is selected,
which isn't really what the statement was about.

>>>>>> An example view of a PCI SF port.
>>>
>>> $ devlink port show netdevsim/netdevsim10/2
>>> netdevsim/netdevsim10/2: type eth netdev eni10npf0sf44 flavour pcisf
>> controller 0 pfnum 0 sfnum 44 external false splittable false
>>>   function:
>>>     hw_addr 00:00:00:00:00:00
>>>
>>> devlink port show netdevsim/netdevsim10/2 -jp {
>>>     "port": {
>>>         "netdevsim/netdevsim10/2": {
>>>             "type": "eth",
>>>             "netdev": "eni10npf0sf44",
>>>             "flavour": "pcisf",
>>>             "controller": 0,
>>>             "pfnum": 0,
>>>             "sfnum": 44,
>>>             "external": false,
>>>             "splittable": false,
>>>             "function": {
>>>                 "hw_addr": "00:00:00:00:00:00"
>>>             }
>>>         }
>>>     }
>>> }
>>>
>>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>> ---
>>>  include/net/devlink.h        | 17 +++++++++++++++++
>>>  include/uapi/linux/devlink.h |  7 +++++++
>>>  net/core/devlink.c           | 37 ++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 61 insertions(+)
>>>
> 
> 
>>>  static int __devlink_port_phys_port_name_get(struct devlink_port
>> *devlink_port,
>>>  					     char *name, size_t len)
>>>  {
>>> @@ -7855,6 +7889,9 @@ static int
>> __devlink_port_phys_port_name_get(struct devlink_port *devlink_port,
>>>  		n = snprintf(name, len, "pf%uvf%u",
>>>  			     attrs->pci_vf.pf, attrs->pci_vf.vf);
>>>  		break;
>>> +	case DEVLINK_PORT_FLAVOUR_PCI_SF:
>>> +		n = snprintf(name, len, "pf%usf%u", attrs->pci_sf.pf, attrs-
>>> pci_sf.sf);
>>> +		break;
>>>  	}
>>>
> This is where the naming scheme is done, like pcipf and pcivf port flavours.
> 
>>>  	if (n >= len)
>>>