On 9/17/20 10:18 PM, Parav Pandit wrote:
>
>> From: David Ahern <dsah...@gmail.com>
>> Sent: Friday, September 18, 2020 1:32 AM
>>
>> On 9/17/20 11:20 AM, Parav Pandit wrote:
>>> diff --git a/include/net/devlink.h b/include/net/devlink.h index
>>> 48b1c1ef1ebd..1edb558125b0 100644
>>> --- a/include/net/devlink.h
>>> +++ b/include/net/devlink.h
>>> @@ -83,6 +83,20 @@ struct devlink_port_pci_vf_attrs {
>>> u8 external:1;
>>> };
>>>
>>> +/**
>>> + * struct devlink_port_pci_sf_attrs - devlink port's PCI SF
>>> +attributes
>>> + * @controller: Associated controller number
>>> + * @pf: Associated PCI PF number for this port.
>>> + * @sf: Associated PCI SF for of the PCI PF for this port.
>>> + * @external: when set, indicates if a port is for an external
>>> +controller */ struct devlink_port_pci_sf_attrs {
>>> + u32 controller;
>>> + u16 pf;
>>> + u32 sf;
>>
>> Why a u32? Do you expect to support that many SFs? Seems like even a u16 is
>> more than you can adequately name within an IFNAMESZ buffer.
>>
> I think u16 is likely enough, which let use creates 64K SF ports which is a
> lot. :-)
> Was little concerned that it shouldn't fall short in few years. So picked
> u32.
> Users will be able to make use of alternative names so just because IFNAMESZ
> is 16 characters, do not want to limit sfnum size.
> What do you think?
>
>>
>>> + u8 external:1;
>>> +};
>>> +
>>> /**
>>> * struct devlink_port_attrs - devlink port object
>>> * @flavour: flavour of the port
>>
>>
>>> diff --git a/net/core/devlink.c b/net/core/devlink.c index
>>> e5b71f3c2d4d..fada660fd515 100644
>>> --- a/net/core/devlink.c
>>> +++ b/net/core/devlink.c
>>> @@ -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;
>>> }
>>>
>>> if (n >= len)
>>>
>>
>> And as I noted before, this function continues to grow device names and it is
>> going to spill over the IFNAMESZ buffer and EINVAL is going to be confusing.
>> It
>> really needs better error handling back to users (not kernel buffers).
> Alternative names [1] should help to overcome the limitation of IFNAMESZ.
> For error code EINVAL, should it be ENOSPC?
> If so, should I insert a pre-patch in this series?
>
> [1] ip link property add dev DEVICE [ altname NAME .. ]
>
You keep adding patches that extend the template based names. Those are
going to cause odd EINVAL failures (the absolute worst kind of
configuration failure) with no way for a user to understand why the
command is failing, and you need to handle that. Returning an extack
message back to the user is preferred.
Yes, the altnames provides a solution after the the netdevice has been
created, but I do not see how that works when the netdevice is created
as part of devlink commands using the template names approach.
