* [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 20:01 ` David Ahern
[not found] ` <fcb55cc1-3be3-3eaa-68d5-28b4d112e291@intel.com>
2020-09-17 17:20 ` [PATCH net-next v2 2/8] devlink: Support add and delete devlink port Parav Pandit
` (7 subsequent siblings)
8 siblings, 2 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
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.
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(+)
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;
+ u8 external:1;
+};
+
/**
* struct devlink_port_attrs - devlink port object
* @flavour: flavour of the port
@@ -104,6 +118,7 @@ struct devlink_port_attrs {
struct devlink_port_phys_attrs phys;
struct devlink_port_pci_pf_attrs pci_pf;
struct devlink_port_pci_vf_attrs pci_vf;
+ struct devlink_port_pci_sf_attrs pci_sf;
};
};
@@ -1230,6 +1245,8 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 contro
u16 pf, bool external);
void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 controller,
u16 pf, u16 vf, bool external);
+void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 controller,
+ u16 pf, u32 sf, bool external);
int devlink_sb_register(struct devlink *devlink, unsigned int sb_index,
u32 size, u16 ingress_pools_count,
u16 egress_pools_count, u16 ingress_tc_count,
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 631f5bdf1707..09c41b9ce407 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -195,6 +195,11 @@ enum devlink_port_flavour {
* port that faces the PCI VF.
*/
DEVLINK_PORT_FLAVOUR_VIRTUAL, /* Any virtual port facing the user. */
+
+ DEVLINK_PORT_FLAVOUR_PCI_SF, /* Represents eswitch port
+ * for the PCI SF. It is an internal
+ * port that faces the PCI SF.
+ */
};
enum devlink_param_cmode {
@@ -462,6 +467,8 @@ enum devlink_attr {
DEVLINK_ATTR_PORT_EXTERNAL, /* u8 */
DEVLINK_ATTR_PORT_CONTROLLER_NUMBER, /* u32 */
+
+ DEVLINK_ATTR_PORT_PCI_SF_NUMBER, /* u32 */
/* add new attributes above here, update the policy in devlink.c */
__DEVLINK_ATTR_MAX,
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
@@ -539,6 +539,15 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
if (nla_put_u8(msg, DEVLINK_ATTR_PORT_EXTERNAL, attrs->pci_vf.external))
return -EMSGSIZE;
break;
+ case DEVLINK_PORT_FLAVOUR_PCI_SF:
+ if (nla_put_u32(msg, DEVLINK_ATTR_PORT_CONTROLLER_NUMBER,
+ attrs->pci_sf.controller) ||
+ nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER, attrs->pci_sf.pf) ||
+ nla_put_u32(msg, DEVLINK_ATTR_PORT_PCI_SF_NUMBER, attrs->pci_sf.sf))
+ return -EMSGSIZE;
+ if (nla_put_u8(msg, DEVLINK_ATTR_PORT_EXTERNAL, attrs->pci_vf.external))
+ return -EMSGSIZE;
+ break;
case DEVLINK_PORT_FLAVOUR_PHYSICAL:
case DEVLINK_PORT_FLAVOUR_CPU:
case DEVLINK_PORT_FLAVOUR_DSA:
@@ -7808,6 +7817,31 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro
}
EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_vf_set);
+/**
+ * devlink_port_attrs_pci_sf_set - Set PCI SF port attributes
+ *
+ * @devlink_port: devlink port
+ * @controller: associated controller number for the devlink port instance
+ * @pf: associated PF for the devlink port instance
+ * @sf: associated SF of a PF for the devlink port instance
+ * @external: indicates if the port is for an external controller
+ */
+void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 controller,
+ u16 pf, u32 sf, bool external)
+{
+ struct devlink_port_attrs *attrs = &devlink_port->attrs;
+ int ret;
+
+ ret = __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_SF);
+ if (ret)
+ return;
+ attrs->pci_sf.controller = controller;
+ attrs->pci_sf.pf = pf;
+ attrs->pci_sf.sf = sf;
+ attrs->pci_sf.external = external;
+}
+EXPORT_SYMBOL_GPL(devlink_port_attrs_pci_sf_set);
+
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;
}
if (n >= len)
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-17 17:20 ` [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute Parav Pandit
@ 2020-09-17 20:01 ` David Ahern
2020-09-18 4:18 ` Parav Pandit
[not found] ` <fcb55cc1-3be3-3eaa-68d5-28b4d112e291@intel.com>
1 sibling, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-17 20:01 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
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.
> + 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).
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-17 20:01 ` David Ahern
@ 2020-09-18 4:18 ` Parav Pandit
2020-09-18 15:15 ` David Ahern
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 4:18 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: David Ahern <dsahern@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 .. ]
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-18 4:18 ` Parav Pandit
@ 2020-09-18 15:15 ` David Ahern
2020-09-18 16:13 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-18 15:15 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/20 10:18 PM, Parav Pandit wrote:
>
>> From: David Ahern <dsahern@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.
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-18 15:15 ` David Ahern
@ 2020-09-18 16:13 ` Parav Pandit
2020-09-19 4:49 ` David Ahern
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 16:13 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
Hi David,
> From: David Ahern <dsahern@gmail.com>
> Sent: Friday, September 18, 2020 8:45 PM
>
> On 9/17/20 10:18 PM, Parav Pandit wrote:
> >
> >> From: David Ahern <dsahern@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.
Sure, make sense.
I will run one short series after this one, to return extack in below code flow and other where extack return is possible.
In sysfs flow it is irrelevant anyway.
rtnl_getlink()
rtnl_phys_port_name_fill()
dev_get_phys_port_name()
ndo_phys_port_name()
is that ok?
This series is not really making phys_port_name any worse except that sfnum field width is bit larger than vfnum.
Now coming back to phys_port_name not fitting in 15 characters which can trigger -EINVAL error is very slim in most sane cases.
Let's take an example.
A controller in valid range of 0 to 16,
PF in range of 0 to 7,
VF in range of 0 to 255,
SF in range of 0 to 65536,
Will generate VF phys_port_name=c16pf7vf255 (11 chars + 1 null)
SF phys_port name = c16pf7sf65535 (13 chars + 1 null)
So yes, eth dev name won't fit in but phys_port_name failure is almost nil.
>
> 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.
systemd/udev version v245 understands the parent PCI device of netdev and phys_port_name to construct a unique netdevice name.
This is the example from this patch series for PF port.
udevadm test-builtin net_id /sys/class/net/eth0
Load module index
Parsed configuration file /usr/lib/systemd/network/99-default.link
Created link configuration context.
Using default interface naming scheme 'v245'.
ID_NET_NAMING_SCHEME=v245
ID_NET_NAME_PATH=eni10npf0
Unload module index
Unloaded link configuration context.
And for SF port,
udevadm test-builtin net_id /sys/class/net/eth1
Load module index
Parsed configuration file /usr/lib/systemd/network/99-default.link
Created link configuration context.
Using default interface naming scheme 'v245'.
ID_NET_NAMING_SCHEME=v245
ID_NET_NAME_PATH=eni10npf0sf44
Unload module index
Unloaded link configuration context.
Similar VF naming in places for I think more than a year now.
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-18 16:13 ` Parav Pandit
@ 2020-09-19 4:49 ` David Ahern
2020-09-19 5:35 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-19 4:49 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/18/20 10:13 AM, Parav Pandit wrote:
>> 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.
> Sure, make sense.
> I will run one short series after this one, to return extack in below code flow and other where extack return is possible.
> In sysfs flow it is irrelevant anyway.
yes, sysfs is a different problem.
> rtnl_getlink()
> rtnl_phys_port_name_fill()
> dev_get_phys_port_name()
> ndo_phys_port_name()
>
> is that ok?
sure. The overflow is not going to happen today with 1-10 SFs; but you
are pushing ever close to the limit hence the push.
> This series is not really making phys_port_name any worse except that sfnum field width is bit larger than vfnum.
>
> Now coming back to phys_port_name not fitting in 15 characters which can trigger -EINVAL error is very slim in most sane cases.
> Let's take an example.
> A controller in valid range of 0 to 16,
> PF in range of 0 to 7,
> VF in range of 0 to 255,
> SF in range of 0 to 65536,
>
> Will generate VF phys_port_name=c16pf7vf255 (11 chars + 1 null)
> SF phys_port name = c16pf7sf65535 (13 chars + 1 null)
> So yes, eth dev name won't fit in but phys_port_name failure is almost nil.
>
You lost me on that last sentence. Per your example for SF, adding 'eni'
or 'eth' as a prefix (which you commonly use in examples and which are
common prefixes) to that name and you overflow the IFNAMESZ limit.
(understand about the systemd integration and appreciate the forward
thinking about that).
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-19 4:49 ` David Ahern
@ 2020-09-19 5:35 ` Parav Pandit
0 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-19 5:35 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: David Ahern <dsahern@gmail.com>
> Sent: Saturday, September 19, 2020 10:19 AM
>
> On 9/18/20 10:13 AM, Parav Pandit wrote:
> >> 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.
> > Sure, make sense.
> > I will run one short series after this one, to return extack in below code flow
> and other where extack return is possible.
> > In sysfs flow it is irrelevant anyway.
>
> yes, sysfs is a different problem.
>
> > rtnl_getlink()
> > rtnl_phys_port_name_fill()
> > dev_get_phys_port_name()
> > ndo_phys_port_name()
> >
> > is that ok?
>
> sure. The overflow is not going to happen today with 1-10 SFs; but you are
> pushing ever close to the limit hence the push.
>
Ok. yeah. got it.
>
> > This series is not really making phys_port_name any worse except that sfnum
> field width is bit larger than vfnum.
> >
> > Now coming back to phys_port_name not fitting in 15 characters which can
> trigger -EINVAL error is very slim in most sane cases.
> > Let's take an example.
> > A controller in valid range of 0 to 16, PF in range of 0 to 7, VF in
> > range of 0 to 255, SF in range of 0 to 65536,
> >
> > Will generate VF phys_port_name=c16pf7vf255 (11 chars + 1 null) SF
> > phys_port name = c16pf7sf65535 (13 chars + 1 null) So yes, eth dev
> > name won't fit in but phys_port_name failure is almost nil.
> >
>
> You lost me on that last sentence. Per your example for SF, adding 'eni'
> or 'eth' as a prefix (which you commonly use in examples and which are
> common prefixes) to that name and you overflow the IFNAMESZ limit.
>
Right. Got you.
Will run short series for returning extack.
>
> (understand about the systemd integration and appreciate the forward thinking
> about that).
Thank you David.
^ permalink raw reply [flat|nested] 52+ messages in thread
[parent not found: <fcb55cc1-3be3-3eaa-68d5-28b4d112e291@intel.com>]
* RE: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
[not found] ` <fcb55cc1-3be3-3eaa-68d5-28b4d112e291@intel.com>
@ 2020-09-18 3:54 ` Parav Pandit
2020-09-18 23:04 ` Jacob Keller
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 3:54 UTC (permalink / raw)
To: Jacob Keller, davem, kuba, netdev; +Cc: Jiri Pirko
> 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().
> 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.
> >
> > 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)
> >
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute
2020-09-18 3:54 ` Parav Pandit
@ 2020-09-18 23:04 ` Jacob Keller
0 siblings, 0 replies; 52+ messages in thread
From: Jacob Keller @ 2020-09-18 23:04 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
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)
>>>
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH net-next v2 2/8] devlink: Support add and delete devlink port
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
[not found] ` <28cbe5b9-a39e-9299-8c9b-6cce63328f0f@intel.com>
2020-09-17 17:20 ` [PATCH net-next v2 3/8] devlink: Prepare code to fill multiple port function attributes Parav Pandit
` (6 subsequent siblings)
8 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Extended devlink interface for the user to add and delete port.
Extend devlink to connect user requests to driver to add/delete
such port in the device.
When driver routines are invoked, devlink instance lock is not held.
This enables driver to perform several devlink objects registration,
unregistration such as (port, health reporter, resource etc)
by using exising devlink APIs.
This also helps to uniformly used the code for port registration
during driver unload and during port deletion initiated by user.
Examples of add, show and delete commands:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
$ devlink port show netdevsim/netdevsim10/0
netdevsim/netdevsim10/0: type eth netdev eni10np1 flavour physical port 1 splittable false
$ devlink port add netdevsim/netdevsim10 flavour pcipf pfnum 0
$ devlink port show netdevsim/netdevsim10/1
netdevsim/netdevsim10/1: type eth netdev eni10npf0 flavour pcipf controller 0 pfnum 0 external false splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive
$ devlink port show netdevsim/netdevsim10/1 -jp
{
"port": {
"netdevsim/netdevsim10/1": {
"type": "eth",
"netdev": "eni10npf0",
"flavour": "pcipf",
"controller": 0,
"pfnum": 0,
"external": false,
"splittable": false,
"function": {
"hw_addr": "00:00:00:00:00:00",
"state": "inactive"
}
}
}
}
$ devlink port del netdevsim/netdevsim10/1
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
include/net/devlink.h | 38 ++++++++++++++++++++++++
net/core/devlink.c | 67 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 105 insertions(+)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 1edb558125b0..ebab2c0360d0 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -142,6 +142,17 @@ struct devlink_port {
struct mutex reporters_lock; /* Protects reporter_list */
};
+struct devlink_port_new_attrs {
+ enum devlink_port_flavour flavour;
+ unsigned int port_index;
+ u32 controller;
+ u32 sfnum;
+ u16 pfnum;
+ u8 port_index_valid:1,
+ controller_valid:1,
+ sfnum_valid:1;
+};
+
struct devlink_sb_pool_info {
enum devlink_sb_pool_type pool_type;
u32 size;
@@ -1189,6 +1200,33 @@ struct devlink_ops {
int (*port_function_hw_addr_set)(struct devlink *devlink, struct devlink_port *port,
const u8 *hw_addr, int hw_addr_len,
struct netlink_ext_ack *extack);
+ /**
+ * @port_new: Port add function.
+ *
+ * Should be used by device driver to let caller add new port of a specified flavour
+ * with optional attributes.
+ * Driver should return -EOPNOTSUPP if it doesn't support port addition of a specified
+ * flavour or specified attributes. Driver should set extack error message in case of fail
+ * to add the port.
+ * devlink core does not hold a devlink instance lock when this callback is invoked.
+ * Driver must ensures synchronization when adding or deleting a port. Driver must
+ * register a port with devlink core.
+ */
+ int (*port_new)(struct devlink *devlink, const struct devlink_port_new_attrs *attrs,
+ struct netlink_ext_ack *extack);
+ /**
+ * @port_del: Port delete function.
+ *
+ * Should be used by device driver to let caller delete port which was previously created
+ * using port_new() callback.
+ * Driver should return -EOPNOTSUPP if it doesn't support port deletion.
+ * Driver should set extack error message in case of fail to delete the port.
+ * devlink core does not hold a devlink instance lock when this callback is invoked.
+ * Driver must ensures synchronization when adding or deleting a port. Driver must
+ * register a port with devlink core.
+ */
+ int (*port_del)(struct devlink *devlink, unsigned int port_index,
+ struct netlink_ext_ack *extack);
};
static inline void *devlink_priv(struct devlink *devlink)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index fada660fd515..e93730065c57 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -991,6 +991,57 @@ static int devlink_nl_cmd_port_unsplit_doit(struct sk_buff *skb,
return devlink_port_unsplit(devlink, port_index, info->extack);
}
+static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct netlink_ext_ack *extack = info->extack;
+ struct devlink_port_new_attrs new_attrs = {};
+ struct devlink *devlink = info->user_ptr[0];
+
+ if (!info->attrs[DEVLINK_ATTR_PORT_FLAVOUR] ||
+ !info->attrs[DEVLINK_ATTR_PORT_PCI_PF_NUMBER]) {
+ NL_SET_ERR_MSG_MOD(extack, "Port flavour or PCI PF are not specified");
+ return -EINVAL;
+ }
+ new_attrs.flavour = nla_get_u16(info->attrs[DEVLINK_ATTR_PORT_FLAVOUR]);
+ new_attrs.pfnum = nla_get_u16(info->attrs[DEVLINK_ATTR_PORT_PCI_PF_NUMBER]);
+
+ if (info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+ new_attrs.port_index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+ new_attrs.port_index_valid = true;
+ }
+ if (info->attrs[DEVLINK_ATTR_PORT_CONTROLLER_NUMBER]) {
+ new_attrs.controller =
+ nla_get_u16(info->attrs[DEVLINK_ATTR_PORT_CONTROLLER_NUMBER]);
+ new_attrs.controller_valid = true;
+ }
+ if (info->attrs[DEVLINK_ATTR_PORT_PCI_SF_NUMBER]) {
+ new_attrs.sfnum = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_PCI_SF_NUMBER]);
+ new_attrs.sfnum_valid = true;
+ }
+
+ if (!devlink->ops->port_new)
+ return -EOPNOTSUPP;
+
+ return devlink->ops->port_new(devlink, &new_attrs, extack);
+}
+
+static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct netlink_ext_ack *extack = info->extack;
+ struct devlink *devlink = info->user_ptr[0];
+ unsigned int port_index;
+
+ if (!info->attrs[DEVLINK_ATTR_PORT_INDEX]) {
+ NL_SET_ERR_MSG_MOD(extack, "Port index is not specified");
+ return -EINVAL;
+ }
+ port_index = nla_get_u32(info->attrs[DEVLINK_ATTR_PORT_INDEX]);
+
+ if (!devlink->ops->port_del)
+ return -EOPNOTSUPP;
+ return devlink->ops->port_del(devlink, port_index, extack);
+}
+
static int devlink_nl_sb_fill(struct sk_buff *msg, struct devlink *devlink,
struct devlink_sb *devlink_sb,
enum devlink_command cmd, u32 portid,
@@ -7078,6 +7129,10 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
[DEVLINK_ATTR_TRAP_POLICER_RATE] = { .type = NLA_U64 },
[DEVLINK_ATTR_TRAP_POLICER_BURST] = { .type = NLA_U64 },
[DEVLINK_ATTR_PORT_FUNCTION] = { .type = NLA_NESTED },
+ [DEVLINK_ATTR_PORT_FLAVOUR] = { .type = NLA_U16 },
+ [DEVLINK_ATTR_PORT_PCI_PF_NUMBER] = { .type = NLA_U16 },
+ [DEVLINK_ATTR_PORT_PCI_SF_NUMBER] = { .type = NLA_U32 },
+ [DEVLINK_ATTR_PORT_CONTROLLER_NUMBER] = { .type = NLA_U32 },
};
static const struct genl_ops devlink_nl_ops[] = {
@@ -7117,6 +7172,18 @@ static const struct genl_ops devlink_nl_ops[] = {
.flags = GENL_ADMIN_PERM,
.internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
},
+ {
+ .cmd = DEVLINK_CMD_PORT_NEW,
+ .doit = devlink_nl_cmd_port_new_doit,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
+ },
+ {
+ .cmd = DEVLINK_CMD_PORT_DEL,
+ .doit = devlink_nl_cmd_port_del_doit,
+ .flags = GENL_ADMIN_PERM,
+ .internal_flags = DEVLINK_NL_FLAG_NO_LOCK,
+ },
{
.cmd = DEVLINK_CMD_SB_GET,
.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH net-next v2 3/8] devlink: Prepare code to fill multiple port function attributes
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 1/8] devlink: Introduce PCI SF port flavour and port attribute Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 2/8] devlink: Support add and delete devlink port Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
[not found] ` <0dc57740-48fb-d77f-dcdf-2607ef2dc545@intel.com>
2020-09-17 17:20 ` [PATCH net-next v2 4/8] devlink: Support get and set state of port function Parav Pandit
` (5 subsequent siblings)
8 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Prepare code to fill zero or more port function optional attributes.
Subsequent patch makes use of this to fill more port function
attributes.
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
net/core/devlink.c | 53 +++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/net/core/devlink.c b/net/core/devlink.c
index e93730065c57..d152489e48da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -570,6 +570,31 @@ static int devlink_nl_port_attrs_put(struct sk_buff *msg,
return 0;
}
+static int
+devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink_ops *ops,
+ struct devlink_port *port, struct sk_buff *msg,
+ struct netlink_ext_ack *extack, bool *msg_updated)
+{
+ u8 hw_addr[MAX_ADDR_LEN];
+ int hw_addr_len;
+ int err;
+
+ if (!ops->port_function_hw_addr_get)
+ return 0;
+
+ err = ops->port_function_hw_addr_get(devlink, port, hw_addr, &hw_addr_len, extack);
+ if (err) {
+ if (err == -EOPNOTSUPP)
+ return 0;
+ return err;
+ }
+ err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr);
+ if (err)
+ return err;
+ *msg_updated = true;
+ return 0;
+}
+
static int
devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *port,
struct netlink_ext_ack *extack)
@@ -577,36 +602,16 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
struct devlink *devlink = port->devlink;
const struct devlink_ops *ops;
struct nlattr *function_attr;
- bool empty_nest = true;
- int err = 0;
+ bool msg_updated = false;
+ int err;
function_attr = nla_nest_start_noflag(msg, DEVLINK_ATTR_PORT_FUNCTION);
if (!function_attr)
return -EMSGSIZE;
ops = devlink->ops;
- if (ops->port_function_hw_addr_get) {
- int hw_addr_len;
- u8 hw_addr[MAX_ADDR_LEN];
-
- err = ops->port_function_hw_addr_get(devlink, port, hw_addr, &hw_addr_len, extack);
- if (err == -EOPNOTSUPP) {
- /* Port function attributes are optional for a port. If port doesn't
- * support function attribute, returning -EOPNOTSUPP is not an error.
- */
- err = 0;
- goto out;
- } else if (err) {
- goto out;
- }
- err = nla_put(msg, DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, hw_addr_len, hw_addr);
- if (err)
- goto out;
- empty_nest = false;
- }
-
-out:
- if (err || empty_nest)
+ err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg, extack, &msg_updated);
+ if (err || !msg_updated)
nla_nest_cancel(msg, function_attr);
else
nla_nest_end(msg, function_attr);
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH net-next v2 4/8] devlink: Support get and set state of port function
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (2 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 3/8] devlink: Prepare code to fill multiple port function attributes Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 20:23 ` David Ahern
2020-09-17 17:20 ` [PATCH net-next v2 5/8] netdevsim: Add support for add and delete of a PCI PF port Parav Pandit
` (4 subsequent siblings)
8 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
devlink port function can be in active or inactive state.
Allow users to get and set port function's state.
Example of a PCI SF port which supports a port function:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
$ devlink port show
netdevsim/netdevsim10/0: type eth netdev eth0 flavour physical port 1 splittable false
$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum 44
$ devlink port show netdevsim/netdevsim10/11
netdevsim/netdevsim10/11: 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 state inactive
$ devlink port function set netdevsim/netdevsim10/11 hw_addr 00:11:22:33:44:55 state active
$ devlink port show netdevsim/netdevsim10/11 -jp
{
"port": {
"netdevsim/netdevsim10/11": {
"type": "eth",
"netdev": "eni10npf0sf44",
"flavour": "pcisf",
"controller": 0,
"pfnum": 0,
"sfnum": 44,
"external": false,
"splittable": false,
"function": {
"hw_addr": "00:11:22:33:44:55",
"state": "active"
}
}
}
}
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
include/net/devlink.h | 20 ++++++++++
include/uapi/linux/devlink.h | 6 +++
net/core/devlink.c | 77 +++++++++++++++++++++++++++++++++++-
3 files changed, 101 insertions(+), 2 deletions(-)
diff --git a/include/net/devlink.h b/include/net/devlink.h
index ebab2c0360d0..500c22835686 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1200,6 +1200,26 @@ struct devlink_ops {
int (*port_function_hw_addr_set)(struct devlink *devlink, struct devlink_port *port,
const u8 *hw_addr, int hw_addr_len,
struct netlink_ext_ack *extack);
+ /**
+ * @port_function_state_get: Port function's state get function.
+ *
+ * Should be used by device drivers to report the state of a function managed
+ * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
+ * function handling for a particular port.
+ */
+ int (*port_function_state_get)(struct devlink *devlink, struct devlink_port *port,
+ enum devlink_port_function_state *state,
+ struct netlink_ext_ack *extack);
+ /**
+ * @port_function_state_set: Port function's state set function.
+ *
+ * Should be used by device drivers to set the state of a function managed
+ * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
+ * function handling for a particular port.
+ */
+ int (*port_function_state_set)(struct devlink *devlink, struct devlink_port *port,
+ enum devlink_port_function_state state,
+ struct netlink_ext_ack *extack);
/**
* @port_new: Port add function.
*
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 09c41b9ce407..8e513f1cd638 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -518,9 +518,15 @@ enum devlink_resource_unit {
enum devlink_port_function_attr {
DEVLINK_PORT_FUNCTION_ATTR_UNSPEC,
DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR, /* binary */
+ DEVLINK_PORT_FUNCTION_ATTR_STATE, /* u8 */
__DEVLINK_PORT_FUNCTION_ATTR_MAX,
DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1
};
+enum devlink_port_function_state {
+ DEVLINK_PORT_FUNCTION_STATE_INACTIVE,
+ DEVLINK_PORT_FUNCTION_STATE_ACTIVE,
+};
+
#endif /* _UAPI_LINUX_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index d152489e48da..c82098cb75da 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -87,6 +87,9 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(devlink_hwerr);
static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_ATTR_MAX + 1] = {
[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR] = { .type = NLA_BINARY },
+ [DEVLINK_PORT_FUNCTION_ATTR_STATE] =
+ NLA_POLICY_RANGE(NLA_U8, DEVLINK_PORT_FUNCTION_STATE_INACTIVE,
+ DEVLINK_PORT_FUNCTION_STATE_ACTIVE),
};
static LIST_HEAD(devlink_list);
@@ -595,6 +598,40 @@ devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink
return 0;
}
+static bool devlink_port_function_state_valid(u8 state)
+{
+ return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||
+ state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;
+}
+
+static int devlink_port_function_state_fill(struct devlink *devlink, const struct devlink_ops *ops,
+ struct devlink_port *port, struct sk_buff *msg,
+ struct netlink_ext_ack *extack, bool *msg_updated)
+{
+ enum devlink_port_function_state state;
+ int err;
+
+ if (!ops->port_function_state_get)
+ return 0;
+
+ err = ops->port_function_state_get(devlink, port, &state, extack);
+ if (err) {
+ if (err == -EOPNOTSUPP)
+ return 0;
+ return err;
+ }
+ if (!devlink_port_function_state_valid(state)) {
+ WARN_ON(1);
+ NL_SET_ERR_MSG_MOD(extack, "Invalid state value read from driver");
+ return -EINVAL;
+ }
+ err = nla_put_u8(msg, DEVLINK_PORT_FUNCTION_ATTR_STATE, state);
+ if (err)
+ return err;
+ *msg_updated = true;
+ return 0;
+}
+
static int
devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *port,
struct netlink_ext_ack *extack)
@@ -611,6 +648,11 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
ops = devlink->ops;
err = devlink_port_function_hw_addr_fill(devlink, ops, port, msg, extack, &msg_updated);
+ if (err)
+ goto out;
+ err = devlink_port_function_state_fill(devlink, ops, port, msg, extack, &msg_updated);
+
+out:
if (err || !msg_updated)
nla_nest_cancel(msg, function_attr);
else
@@ -879,6 +921,28 @@ devlink_port_function_hw_addr_set(struct devlink *devlink, struct devlink_port *
return 0;
}
+static int
+devlink_port_function_state_set(struct devlink *devlink, struct devlink_port *port,
+ const struct nlattr *attr, struct netlink_ext_ack *extack)
+{
+ enum devlink_port_function_state state;
+ const struct devlink_ops *ops;
+ int err;
+
+ state = nla_get_u8(attr);
+ ops = devlink->ops;
+ if (!ops->port_function_state_set) {
+ NL_SET_ERR_MSG_MOD(extack, "Port function does not support state setting");
+ return -EOPNOTSUPP;
+ }
+ err = ops->port_function_state_set(devlink, port, state, extack);
+ if (err)
+ return err;
+
+ devlink_port_notify(port, DEVLINK_CMD_PORT_NEW);
+ return 0;
+}
+
static int
devlink_port_function_set(struct devlink *devlink, struct devlink_port *port,
const struct nlattr *attr, struct netlink_ext_ack *extack)
@@ -894,9 +958,18 @@ devlink_port_function_set(struct devlink *devlink, struct devlink_port *port,
}
attr = tb[DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR];
- if (attr)
+ if (attr) {
err = devlink_port_function_hw_addr_set(devlink, port, attr, extack);
-
+ if (err)
+ return err;
+ }
+ /* Keep this as the last function attribute set, so that when
+ * multiple port function attributes are set along with state,
+ * Those can be applied first before activating the state.
+ */
+ attr = tb[DEVLINK_PORT_FUNCTION_ATTR_STATE];
+ if (attr)
+ err = devlink_port_function_state_set(devlink, port, attr, extack);
return err;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 4/8] devlink: Support get and set state of port function
2020-09-17 17:20 ` [PATCH net-next v2 4/8] devlink: Support get and set state of port function Parav Pandit
@ 2020-09-17 20:23 ` David Ahern
2020-09-18 3:30 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-17 20:23 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/20 11:20 AM, Parav Pandit wrote:
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index d152489e48da..c82098cb75da 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -595,6 +598,40 @@ devlink_port_function_hw_addr_fill(struct devlink *devlink, const struct devlink
> return 0;
> }
>
> +static bool devlink_port_function_state_valid(u8 state)
you have a named enum so why not 'enum devlink_port_function_state state'?
> +{
> + return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||
> + state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;
> +}
> +
> +static int devlink_port_function_state_fill(struct devlink *devlink, const struct devlink_ops *ops,
> + struct devlink_port *port, struct sk_buff *msg,
> + struct netlink_ext_ack *extack, bool *msg_updated)
> +{
> + enum devlink_port_function_state state;
> + int err;
> +
> + if (!ops->port_function_state_get)
> + return 0;
> +
> + err = ops->port_function_state_get(devlink, port, &state, extack);
> + if (err) {
> + if (err == -EOPNOTSUPP)
> + return 0;
> + return err;
> + }
> + if (!devlink_port_function_state_valid(state)) {
> + WARN_ON(1);
WARN_ON_ONCE at most.
> + NL_SET_ERR_MSG_MOD(extack, "Invalid state value read from driver");
> + return -EINVAL;
> + }
> + err = nla_put_u8(msg, DEVLINK_PORT_FUNCTION_ATTR_STATE, state);
> + if (err)
> + return err;
> + *msg_updated = true;
> + return 0;
> +}
> +
> static int
> devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *port,
> struct netlink_ext_ack *extack)
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 4/8] devlink: Support get and set state of port function
2020-09-17 20:23 ` David Ahern
@ 2020-09-18 3:30 ` Parav Pandit
0 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 3:30 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: David Ahern <dsahern@gmail.com>
> Sent: Friday, September 18, 2020 1:54 AM
>
> On 9/17/20 11:20 AM, Parav Pandit wrote:
> > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > d152489e48da..c82098cb75da 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -595,6 +598,40 @@ devlink_port_function_hw_addr_fill(struct devlink
> *devlink, const struct devlink
> > return 0;
> > }
> >
> > +static bool devlink_port_function_state_valid(u8 state)
>
> you have a named enum so why not 'enum devlink_port_function_state state'?
>
Right. I should. I missed it.
Will do.
>
> > +{
> > + return state == DEVLINK_PORT_FUNCTION_STATE_INACTIVE ||
> > + state == DEVLINK_PORT_FUNCTION_STATE_ACTIVE;
> > +}
> > +
> > +static int devlink_port_function_state_fill(struct devlink *devlink, const struct
> devlink_ops *ops,
> > + struct devlink_port *port, struct
> sk_buff *msg,
> > + struct netlink_ext_ack *extack, bool
> *msg_updated) {
> > + enum devlink_port_function_state state;
> > + int err;
> > +
> > + if (!ops->port_function_state_get)
> > + return 0;
> > +
> > + err = ops->port_function_state_get(devlink, port, &state, extack);
> > + if (err) {
> > + if (err == -EOPNOTSUPP)
> > + return 0;
> > + return err;
> > + }
> > + if (!devlink_port_function_state_valid(state)) {
> > + WARN_ON(1);
>
> WARN_ON_ONCE at most.
>
Yep. I will change to WARN_ON_ONCE.
^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH net-next v2 5/8] netdevsim: Add support for add and delete of a PCI PF port
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (3 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 4/8] devlink: Support get and set state of port function Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 6/8] netdevsim: Simulate get/set hardware address of a PCI port Parav Pandit
` (3 subsequent siblings)
8 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Simulate PCI PF ports. Allow user to create one or more PCI PF ports.
Examples:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
Add and show devlink port of flavour 'pcipf' for PF number 0.
$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port show netdevsim/netdevsim10/10
netdevsim/netdevsim10/10: type eth netdev eni10npf0 flavour pcipf controller 0 pfnum 0 external false splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive
Delete newly added devlink port
$ devlink port add netdevsim/netdevsim10/10
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
Changelog:
v1->v2:
- Fixed extra semicolon at end of switch case reportec by coccinelle
---
drivers/net/netdevsim/Makefile | 3 +-
drivers/net/netdevsim/dev.c | 10 +
drivers/net/netdevsim/netdevsim.h | 19 ++
drivers/net/netdevsim/port_function.c | 337 ++++++++++++++++++++++++++
4 files changed, 368 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/netdevsim/port_function.c
diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
index ade086eed955..e69e895af62c 100644
--- a/drivers/net/netdevsim/Makefile
+++ b/drivers/net/netdevsim/Makefile
@@ -3,7 +3,8 @@
obj-$(CONFIG_NETDEVSIM) += netdevsim.o
netdevsim-objs := \
- netdev.o dev.o ethtool.o fib.o bus.o health.o udp_tunnels.o
+ netdev.o dev.o ethtool.o fib.o bus.o health.o udp_tunnels.o \
+ port_function.o
ifeq ($(CONFIG_BPF_SYSCALL),y)
netdevsim-objs += \
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 32f339fedb21..e3b81c8b5125 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -884,6 +884,8 @@ static const struct devlink_ops nsim_dev_devlink_ops = {
.trap_group_set = nsim_dev_devlink_trap_group_set,
.trap_policer_set = nsim_dev_devlink_trap_policer_set,
.trap_policer_counter_get = nsim_dev_devlink_trap_policer_counter_get,
+ .port_new = nsim_dev_devlink_port_new,
+ .port_del = nsim_dev_devlink_port_del,
};
#define NSIM_DEV_MAX_MACS_DEFAULT 32
@@ -1017,6 +1019,8 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
nsim_dev->ddir,
nsim_dev,
&nsim_dev_take_snapshot_fops);
+
+ nsim_dev_port_function_enable(nsim_dev);
return 0;
err_health_exit:
@@ -1050,6 +1054,7 @@ int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
nsim_dev->max_macs = NSIM_DEV_MAX_MACS_DEFAULT;
nsim_dev->test1 = NSIM_DEV_TEST1_DEFAULT;
spin_lock_init(&nsim_dev->fa_cookie_lock);
+ nsim_dev_port_function_init(nsim_dev);
dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
@@ -1097,6 +1102,7 @@ int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
if (err)
goto err_bpf_dev_exit;
+ nsim_dev_port_function_enable(nsim_dev);
devlink_params_publish(devlink);
devlink_reload_enable(devlink);
return 0;
@@ -1131,6 +1137,9 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
if (devlink_is_reload_failed(devlink))
return;
+
+ /* Disable and destroy any user created devlink ports */
+ nsim_dev_port_function_disable(nsim_dev);
debugfs_remove(nsim_dev->take_snapshot);
nsim_dev_port_del_all(nsim_dev);
nsim_dev_health_exit(nsim_dev);
@@ -1155,6 +1164,7 @@ void nsim_dev_remove(struct nsim_bus_dev *nsim_bus_dev)
ARRAY_SIZE(nsim_devlink_params));
devlink_unregister(devlink);
devlink_resources_unregister(devlink, NULL);
+ nsim_dev_port_function_exit(nsim_dev);
devlink_free(devlink);
}
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 0c86561e6d8d..aec3c4d5fda7 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -213,6 +213,16 @@ struct nsim_dev {
bool ipv4_only;
u32 sleep;
} udp_ports;
+ struct {
+ refcount_t refcount; /* refcount along with disable_complete serializes
+ * port operations with port function disablement
+ * during driver unload.
+ */
+ struct completion disable_complete;
+ struct list_head head;
+ struct ida ida;
+ struct ida pfnum_ida;
+ } port_functions;
};
static inline struct net *nsim_dev_net(struct nsim_dev *nsim_dev)
@@ -283,3 +293,12 @@ struct nsim_bus_dev {
int nsim_bus_init(void);
void nsim_bus_exit(void);
+
+void nsim_dev_port_function_init(struct nsim_dev *nsim_dev);
+void nsim_dev_port_function_exit(struct nsim_dev *nsim_dev);
+void nsim_dev_port_function_enable(struct nsim_dev *nsim_dev);
+void nsim_dev_port_function_disable(struct nsim_dev *nsim_dev);
+int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *attrs,
+ struct netlink_ext_ack *extack);
+int nsim_dev_devlink_port_del(struct devlink *devlink, unsigned int port_index,
+ struct netlink_ext_ack *extack);
diff --git a/drivers/net/netdevsim/port_function.c b/drivers/net/netdevsim/port_function.c
new file mode 100644
index 000000000000..4f3e9cc9489f
--- /dev/null
+++ b/drivers/net/netdevsim/port_function.c
@@ -0,0 +1,337 @@
+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
+/* Copyright (c) 2020 Mellanox Technologies Ltd. */
+
+#include <linux/etherdevice.h>
+#include <uapi/linux/devlink.h>
+
+#include "netdevsim.h"
+
+struct nsim_port_function {
+ struct devlink_port dl_port;
+ struct net_device *netdev;
+ struct list_head list;
+ unsigned int port_index;
+ enum devlink_port_flavour flavour;
+ u32 controller;
+ u16 pfnum;
+ struct nsim_port_function *pf_port; /* Valid only for SF port */
+};
+
+void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
+{
+ refcount_set(&nsim_dev->port_functions.refcount, 0);
+ INIT_LIST_HEAD(&nsim_dev->port_functions.head);
+ ida_init(&nsim_dev->port_functions.ida);
+ ida_init(&nsim_dev->port_functions.pfnum_ida);
+}
+
+void nsim_dev_port_function_exit(struct nsim_dev *nsim_dev)
+{
+ WARN_ON(!ida_is_empty(&nsim_dev->port_functions.pfnum_ida));
+ ida_destroy(&nsim_dev->port_functions.pfnum_ida);
+ WARN_ON(!ida_is_empty(&nsim_dev->port_functions.ida));
+ ida_destroy(&nsim_dev->port_functions.ida);
+ WARN_ON(!list_empty(&nsim_dev->port_functions.head));
+ WARN_ON(refcount_read(&nsim_dev->port_functions.refcount));
+}
+
+static bool nsim_dev_port_function_try_get(struct nsim_dev *nsim_dev)
+{
+ return refcount_inc_not_zero(&nsim_dev->port_functions.refcount);
+}
+
+static void nsim_dev_port_function_put(struct nsim_dev *nsim_dev)
+{
+ if (refcount_dec_and_test(&nsim_dev->port_functions.refcount))
+ complete(&nsim_dev->port_functions.disable_complete);
+}
+
+static struct devlink_port *nsim_dev_port_function_get_devlink_port(struct net_device *dev)
+{
+ struct nsim_port_function *port = netdev_priv(dev);
+
+ return &port->dl_port;
+}
+
+static netdev_tx_t nsim_dev_port_function_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+ dev_kfree_skb(skb);
+ return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops nsim_netdev_ops = {
+ .ndo_start_xmit = nsim_dev_port_function_start_xmit,
+ .ndo_get_devlink_port = nsim_dev_port_function_get_devlink_port,
+};
+
+static void nsim_port_function_ndev_setup(struct net_device *dev)
+{
+ ether_setup(dev);
+ eth_hw_addr_random(dev);
+
+ dev->tx_queue_len = 0;
+ dev->flags |= IFF_NOARP;
+ dev->flags &= ~IFF_MULTICAST;
+ dev->max_mtu = ETH_MAX_MTU;
+}
+
+static struct nsim_port_function *
+nsim_devlink_port_function_alloc(struct nsim_dev *dev, const struct devlink_port_new_attrs *attrs)
+{
+ struct nsim_bus_dev *nsim_bus_dev = dev->nsim_bus_dev;
+ struct nsim_port_function *port;
+ struct net_device *netdev;
+ int ret;
+
+ netdev = alloc_netdev(sizeof(*port), "eth%d", NET_NAME_UNKNOWN,
+ nsim_port_function_ndev_setup);
+ if (!netdev)
+ return ERR_PTR(-ENOMEM);
+
+ dev_net_set(netdev, nsim_dev_net(dev));
+ netdev->netdev_ops = &nsim_netdev_ops;
+ nsim_bus_dev = dev->nsim_bus_dev;
+ SET_NETDEV_DEV(netdev, &nsim_bus_dev->dev);
+
+ port = netdev_priv(netdev);
+ memset(port, 0, sizeof(*port));
+ port->netdev = netdev;
+ port->flavour = attrs->flavour;
+
+ if (attrs->port_index_valid)
+ ret = ida_alloc_range(&dev->port_functions.ida, attrs->port_index,
+ attrs->port_index, GFP_KERNEL);
+ else
+ ret = ida_alloc_min(&dev->port_functions.ida, nsim_bus_dev->port_count, GFP_KERNEL);
+ if (ret < 0)
+ goto port_ida_err;
+
+ port->port_index = ret;
+ port->controller = attrs->controller_valid ? attrs->controller : 0;
+
+ switch (port->flavour) {
+ case DEVLINK_PORT_FLAVOUR_PCI_PF:
+ ret = ida_alloc_range(&dev->port_functions.pfnum_ida, attrs->pfnum, attrs->pfnum,
+ GFP_KERNEL);
+ if (ret < 0)
+ goto fn_ida_err;
+ port->pfnum = ret;
+ break;
+ default:
+ break;
+ }
+ return port;
+
+fn_ida_err:
+ ida_simple_remove(&dev->port_functions.ida, port->port_index);
+port_ida_err:
+ free_netdev(netdev);
+ return ERR_PTR(ret);
+}
+
+static void nsim_devlink_port_function_free(struct nsim_dev *dev, struct nsim_port_function *port)
+{
+ switch (port->flavour) {
+ case DEVLINK_PORT_FLAVOUR_PCI_PF:
+ ida_simple_remove(&dev->port_functions.pfnum_ida, port->pfnum);
+ break;
+ default:
+ break;
+ }
+ ida_simple_remove(&dev->port_functions.ida, port->port_index);
+ free_netdev(port->netdev);
+}
+
+static bool nsim_dev_port_index_internal(struct nsim_dev *nsim_dev, unsigned int port_index)
+{
+ struct nsim_bus_dev *nsim_bus_dev = nsim_dev->nsim_bus_dev;
+
+ return (port_index < nsim_bus_dev->port_count) ? true : false;
+}
+
+static bool
+nsim_dev_port_port_exists(struct nsim_dev *nsim_dev, const struct devlink_port_new_attrs *attrs)
+{
+ struct nsim_port_function *tmp;
+
+ list_for_each_entry(tmp, &nsim_dev->port_functions.head, list) {
+ if (attrs->port_index_valid && tmp->port_index == attrs->port_index)
+ return true;
+ if (attrs->controller_valid && tmp->controller != attrs->controller)
+ continue;
+ /* If controller is provided, and if the port is for a specific controller,
+ * skip them.
+ */
+ if (!attrs->controller_valid && tmp->controller)
+ continue;
+
+ if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF &&
+ tmp->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF && tmp->pfnum == attrs->pfnum)
+ return true;
+ }
+ return false;
+}
+
+static struct nsim_port_function *
+nsim_dev_devlink_port_index_lookup(const struct nsim_dev *nsim_dev, unsigned int port_index,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_port_function *port;
+
+ list_for_each_entry(port, &nsim_dev->port_functions.head, list) {
+ if (port->port_index != port_index)
+ continue;
+ return port;
+ }
+ NL_SET_ERR_MSG_MOD(extack, "User created port not found");
+ return ERR_PTR(-ENOENT);
+}
+
+static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_dev *nsim_dev,
+ struct nsim_port_function *port,
+ struct netlink_ext_ack *extack)
+{
+ int err;
+
+ list_add(&port->list, &nsim_dev->port_functions.head);
+
+ err = devlink_port_register(devlink, &port->dl_port, port->port_index);
+ if (err)
+ goto reg_err;
+
+ err = register_netdev(port->netdev);
+ if (err)
+ goto netdev_err;
+
+ devlink_port_type_eth_set(&port->dl_port, port->netdev);
+ return 0;
+
+netdev_err:
+ devlink_port_type_clear(&port->dl_port);
+ devlink_port_unregister(&port->dl_port);
+reg_err:
+ list_del(&port->list);
+ return err;
+}
+
+static void nsim_devlink_port_function_del(struct nsim_dev *nsim_dev,
+ struct nsim_port_function *port)
+{
+ devlink_port_type_clear(&port->dl_port);
+ unregister_netdev(port->netdev);
+ devlink_port_unregister(&port->dl_port);
+ list_del(&port->list);
+}
+
+static bool nsim_dev_port_flavour_supported(const struct nsim_dev *nsim_dev,
+ const struct devlink_port_new_attrs *attrs)
+{
+ return attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF;
+}
+
+int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *attrs,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_bus_dev *nsim_bus_dev;
+ struct nsim_port_function *port;
+ int err;
+
+ nsim_bus_dev = nsim_dev->nsim_bus_dev;
+ if (attrs->port_index_valid && attrs->port_index < nsim_bus_dev->port_count) {
+ NL_SET_ERR_MSG_MOD(extack, "Port with given port index already exist");
+ return -EEXIST;
+ }
+ if (!nsim_dev_port_flavour_supported(nsim_dev, attrs)) {
+ NL_SET_ERR_MSG_MOD(extack, "Unsupported port flavour specified");
+ return -EOPNOTSUPP;
+ }
+ if (!nsim_dev_port_function_try_get(nsim_dev))
+ return -EPERM;
+ if (nsim_dev_port_port_exists(nsim_dev, attrs)) {
+ NL_SET_ERR_MSG_MOD(extack, "Port with given attributes already exists");
+ err = -EEXIST;
+ goto alloc_err;
+ }
+ port = nsim_devlink_port_function_alloc(nsim_dev, attrs);
+ if (IS_ERR(port)) {
+ NL_SET_ERR_MSG_MOD(extack, "Fail to allocate port");
+ err = PTR_ERR(port);
+ goto alloc_err;
+ }
+ memcpy(port->dl_port.attrs.switch_id.id, nsim_dev->switch_id.id,
+ nsim_dev->switch_id.id_len);
+ port->dl_port.attrs.switch_id.id_len = nsim_dev->switch_id.id_len;
+
+ devlink_port_attrs_pci_pf_set(&port->dl_port, port->controller, port->pfnum, false);
+
+ err = nsim_devlink_port_function_add(devlink, nsim_dev, port, extack);
+ if (err)
+ goto add_err;
+
+ nsim_dev_port_function_put(nsim_dev);
+ return 0;
+
+add_err:
+ nsim_devlink_port_function_free(nsim_dev, port);
+alloc_err:
+ nsim_dev_port_function_put(nsim_dev);
+ return err;
+}
+
+int nsim_dev_devlink_port_del(struct devlink *devlink, unsigned int port_index,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_port_function *port;
+
+ if (nsim_dev_port_index_internal(nsim_dev, port_index)) {
+ NL_SET_ERR_MSG_MOD(extack, "Port index doesn't belong to user created port");
+ return -EINVAL;
+ }
+
+ if (!nsim_dev_port_function_try_get(nsim_dev))
+ return -EPERM;
+
+ port = nsim_dev_devlink_port_index_lookup(nsim_dev, port_index, extack);
+ if (IS_ERR(port))
+ goto err;
+ nsim_devlink_port_function_del(nsim_dev, port);
+ nsim_devlink_port_function_free(nsim_dev, port);
+ nsim_dev_port_function_put(nsim_dev);
+ return 0;
+
+err:
+ nsim_dev_port_function_put(nsim_dev);
+ return PTR_ERR(port);
+}
+
+void nsim_dev_port_function_enable(struct nsim_dev *nsim_dev)
+{
+ init_completion(&nsim_dev->port_functions.disable_complete);
+ refcount_set(&nsim_dev->port_functions.refcount, 1);
+}
+
+void nsim_dev_port_function_disable(struct nsim_dev *nsim_dev)
+{
+ struct nsim_port_function *port;
+ struct nsim_port_function *tmp;
+
+ /* Balances with refcount_set(); drop the refcount so that
+ * any new port new/del or port function get/set commands
+ * cannot start.
+ */
+ nsim_dev_port_function_put(nsim_dev);
+ /* Wait for any ongoing commands to complete. */
+ wait_for_completion(&nsim_dev->port_functions.disable_complete);
+
+ /* At this point, no new user commands can start and any ongoing
+ * commands have completed, so it is safe to delete all user created
+ * ports.
+ */
+
+ list_for_each_entry_safe_reverse(port, tmp, &nsim_dev->port_functions.head, list) {
+ nsim_devlink_port_function_del(nsim_dev, port);
+ nsim_devlink_port_function_free(nsim_dev, port);
+ }
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH net-next v2 6/8] netdevsim: Simulate get/set hardware address of a PCI port
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (4 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 5/8] netdevsim: Add support for add and delete of a PCI PF port Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 7/8] netdevsim: Simulate port function state for " Parav Pandit
` (2 subsequent siblings)
8 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Allow users to get/set hardware address for the PCI port.
Below example creates one devlink port, queries a port, sets a
hardware address.
Example of a PCI SF port which supports a port function hw_addr set:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port show netdevsim/netdevsim10/10
netdevsim/netdevsim10/10: type eth netdev eni10npf0 flavour pcipf controller 0 pfnum 0 external false splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive
$ devlink port function set netdevsim/netdevsim10/10 hw_addr 00:11:22:33:44:55
$ devlink port show netdevsim/netdevsim10/10 -jp
{
"port": {
"netdevsim/netdevsim10/11": {
"type": "eth",
"netdev": "eni10npf0",
"flavour": "pcisf",
"controller": 0,
"pfnum": 0,
"sfnum": 44,
"external": true,
"splittable": false,
"function": {
"hw_addr": "00:11:22:33:44:55"
}
}
}
}
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
drivers/net/netdevsim/dev.c | 2 ++
drivers/net/netdevsim/netdevsim.h | 6 ++++
drivers/net/netdevsim/port_function.c | 44 +++++++++++++++++++++++++++
3 files changed, 52 insertions(+)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e3b81c8b5125..ef2e293f358b 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -886,6 +886,8 @@ static const struct devlink_ops nsim_dev_devlink_ops = {
.trap_policer_counter_get = nsim_dev_devlink_trap_policer_counter_get,
.port_new = nsim_dev_devlink_port_new,
.port_del = nsim_dev_devlink_port_del,
+ .port_function_hw_addr_get = nsim_dev_port_function_hw_addr_get,
+ .port_function_hw_addr_set = nsim_dev_port_function_hw_addr_set,
};
#define NSIM_DEV_MAX_MACS_DEFAULT 32
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index aec3c4d5fda7..8dc8f4e5dcd8 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -302,3 +302,9 @@ int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port
struct netlink_ext_ack *extack);
int nsim_dev_devlink_port_del(struct devlink *devlink, unsigned int port_index,
struct netlink_ext_ack *extack);
+int nsim_dev_port_function_hw_addr_get(struct devlink *devlink, struct devlink_port *port,
+ u8 *hw_addr, int *hw_addr_len,
+ struct netlink_ext_ack *extack);
+int nsim_dev_port_function_hw_addr_set(struct devlink *devlink, struct devlink_port *port,
+ const u8 *hw_addr, int hw_addr_len,
+ struct netlink_ext_ack *extack);
diff --git a/drivers/net/netdevsim/port_function.c b/drivers/net/netdevsim/port_function.c
index 4f3e9cc9489f..6feeeaf19ce8 100644
--- a/drivers/net/netdevsim/port_function.c
+++ b/drivers/net/netdevsim/port_function.c
@@ -15,6 +15,7 @@ struct nsim_port_function {
u32 controller;
u16 pfnum;
struct nsim_port_function *pf_port; /* Valid only for SF port */
+ u8 hw_addr[ETH_ALEN];
};
void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
@@ -335,3 +336,46 @@ void nsim_dev_port_function_disable(struct nsim_dev *nsim_dev)
nsim_devlink_port_function_free(nsim_dev, port);
}
}
+
+static struct nsim_port_function *nsim_dev_to_port_function(struct nsim_dev *nsim_dev,
+ struct devlink_port *dl_port)
+{
+ if (nsim_dev_port_index_internal(nsim_dev, dl_port->index))
+ return ERR_PTR(-EOPNOTSUPP);
+ return container_of(dl_port, struct nsim_port_function, dl_port);
+}
+
+int nsim_dev_port_function_hw_addr_get(struct devlink *devlink, struct devlink_port *dl_port,
+ u8 *hw_addr, int *hw_addr_len,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_port_function *port;
+
+ port = nsim_dev_to_port_function(nsim_dev, dl_port);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+
+ memcpy(hw_addr, port->hw_addr, ETH_ALEN);
+ *hw_addr_len = ETH_ALEN;
+ return 0;
+}
+
+int nsim_dev_port_function_hw_addr_set(struct devlink *devlink, struct devlink_port *dl_port,
+ const u8 *hw_addr, int hw_addr_len,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_port_function *port;
+
+ if (hw_addr_len != ETH_ALEN) {
+ NL_SET_ERR_MSG_MOD(extack, "Hardware address must be 6 bytes long");
+ return -EOPNOTSUPP;
+ }
+ port = nsim_dev_to_port_function(nsim_dev, dl_port);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+
+ memcpy(port->hw_addr, hw_addr, ETH_ALEN);
+ return 0;
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH net-next v2 7/8] netdevsim: Simulate port function state for a PCI port
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (5 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 6/8] netdevsim: Simulate get/set hardware address of a PCI port Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 17:20 ` [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port Parav Pandit
2020-09-18 16:52 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Jakub Kicinski
8 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Simulate port function state of a PCI port.
This enables users to get and set the state of the PCI port function.
Example of a PCI SF port which supports a port function:
Create a device with ID=10 and one physical port.
$ echo "10 1" > /sys/bus/netdevsim/new_device
$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port function set netdevsim/netdevsim10/10 hw_addr 00:11:22:33:44:55 state active
$ devlink port show netdevsim/netdevsim10/10
netdevsim/netdevsim10/10: type eth netdev eni10npf0 flavour pcipf controller 0 pfnum 0 external true splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive
$ devlink port function set netdevsim/netdevsim10/10 hw_addr 00:11:22:33:44:55 state active
$ devlink port show netdevsim/netdevsim10/10 -jp
{
"port": {
"netdevsim/netdevsim10/10": {
"type": "eth",
"netdev": "eni10npf0",
"flavour": "pcipf",
"controller": 0,
"pfnum": 0,
"external": false,
"splittable": false,
"function": {
"hw_addr": "00:11:22:33:44:55",
"state": "active"
}
}
}
}
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
drivers/net/netdevsim/dev.c | 2 ++
drivers/net/netdevsim/netdevsim.h | 6 ++++++
drivers/net/netdevsim/port_function.c | 30 +++++++++++++++++++++++++++
3 files changed, 38 insertions(+)
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index ef2e293f358b..ec1e5dc74be1 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -888,6 +888,8 @@ static const struct devlink_ops nsim_dev_devlink_ops = {
.port_del = nsim_dev_devlink_port_del,
.port_function_hw_addr_get = nsim_dev_port_function_hw_addr_get,
.port_function_hw_addr_set = nsim_dev_port_function_hw_addr_set,
+ .port_function_state_get = nsim_dev_port_function_state_get,
+ .port_function_state_set = nsim_dev_port_function_state_set,
};
#define NSIM_DEV_MAX_MACS_DEFAULT 32
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 8dc8f4e5dcd8..0ea9705eda38 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -308,3 +308,9 @@ int nsim_dev_port_function_hw_addr_get(struct devlink *devlink, struct devlink_p
int nsim_dev_port_function_hw_addr_set(struct devlink *devlink, struct devlink_port *port,
const u8 *hw_addr, int hw_addr_len,
struct netlink_ext_ack *extack);
+int nsim_dev_port_function_state_get(struct devlink *devlink, struct devlink_port *port,
+ enum devlink_port_function_state *state,
+ struct netlink_ext_ack *extack);
+int nsim_dev_port_function_state_set(struct devlink *devlink, struct devlink_port *port,
+ enum devlink_port_function_state state,
+ struct netlink_ext_ack *extack);
diff --git a/drivers/net/netdevsim/port_function.c b/drivers/net/netdevsim/port_function.c
index 6feeeaf19ce8..99581d3d15fe 100644
--- a/drivers/net/netdevsim/port_function.c
+++ b/drivers/net/netdevsim/port_function.c
@@ -16,6 +16,7 @@ struct nsim_port_function {
u16 pfnum;
struct nsim_port_function *pf_port; /* Valid only for SF port */
u8 hw_addr[ETH_ALEN];
+ u8 state; /* enum devlink_port_function_state */
};
void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
@@ -196,6 +197,7 @@ static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_d
list_add(&port->list, &nsim_dev->port_functions.head);
+ port->state = DEVLINK_PORT_FUNCTION_STATE_INACTIVE;
err = devlink_port_register(devlink, &port->dl_port, port->port_index);
if (err)
goto reg_err;
@@ -379,3 +381,31 @@ int nsim_dev_port_function_hw_addr_set(struct devlink *devlink, struct devlink_p
memcpy(port->hw_addr, hw_addr, ETH_ALEN);
return 0;
}
+
+int nsim_dev_port_function_state_get(struct devlink *devlink, struct devlink_port *dl_port,
+ enum devlink_port_function_state *state,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_port_function *port;
+
+ port = nsim_dev_to_port_function(nsim_dev, dl_port);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+ *state = port->state;
+ return 0;
+}
+
+int nsim_dev_port_function_state_set(struct devlink *devlink, struct devlink_port *dl_port,
+ enum devlink_port_function_state state,
+ struct netlink_ext_ack *extack)
+{
+ struct nsim_dev *nsim_dev = devlink_priv(devlink);
+ struct nsim_port_function *port;
+
+ port = nsim_dev_to_port_function(nsim_dev, dl_port);
+ if (IS_ERR(port))
+ return PTR_ERR(port);
+ port->state = state;
+ return 0;
+}
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (6 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 7/8] netdevsim: Simulate port function state for " Parav Pandit
@ 2020-09-17 17:20 ` Parav Pandit
2020-09-17 20:31 ` David Ahern
2020-09-18 16:52 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Jakub Kicinski
8 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-17 17:20 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: Parav Pandit, Jiri Pirko
Simulate PCI SF ports. Allow user to create one or more PCI SF ports.
Examples:
Create a PCI PF and PCI SF port.
$ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
$ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum 44
$ devlink port show netdevsim/netdevsim10/11
netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour pcisf controller 0 pfnum 0 sfnum 44 external true splittable false
function:
hw_addr 00:00:00:00:00:00 state inactive
$ devlink port function set netdevsim/netdevsim10/11 hw_addr 00:11:22:33:44:55 state active
$ devlink port show netdevsim/netdevsim10/11 -jp
{
"port": {
"netdevsim/netdevsim10/11": {
"type": "eth",
"netdev": "eni10npf0sf44",
"flavour": "pcisf",
"controller": 0,
"pfnum": 0,
"sfnum": 44,
"external": true,
"splittable": false,
"function": {
"hw_addr": "00:11:22:33:44:55",
"state": "active"
}
}
}
}
Delete newly added devlink port
$ devlink port add netdevsim/netdevsim10/11
Add devlink port of flavour 'pcisf' where port index and sfnum are
auto assigned by driver.
$ devlink port add netdevsim/netdevsim10 flavour pcisf controller 0 pfnum 0
Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
drivers/net/netdevsim/netdevsim.h | 1 +
drivers/net/netdevsim/port_function.c | 95 +++++++++++++++++++++++++--
2 files changed, 92 insertions(+), 4 deletions(-)
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 0ea9705eda38..c70782e444d5 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -222,6 +222,7 @@ struct nsim_dev {
struct list_head head;
struct ida ida;
struct ida pfnum_ida;
+ struct ida sfnum_ida;
} port_functions;
};
diff --git a/drivers/net/netdevsim/port_function.c b/drivers/net/netdevsim/port_function.c
index 99581d3d15fe..e1812acd55b4 100644
--- a/drivers/net/netdevsim/port_function.c
+++ b/drivers/net/netdevsim/port_function.c
@@ -13,10 +13,12 @@ struct nsim_port_function {
unsigned int port_index;
enum devlink_port_flavour flavour;
u32 controller;
+ u32 sfnum;
u16 pfnum;
struct nsim_port_function *pf_port; /* Valid only for SF port */
u8 hw_addr[ETH_ALEN];
u8 state; /* enum devlink_port_function_state */
+ int refcount; /* Counts how many sf ports are bound attached to this pf port. */
};
void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
@@ -25,10 +27,13 @@ void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
INIT_LIST_HEAD(&nsim_dev->port_functions.head);
ida_init(&nsim_dev->port_functions.ida);
ida_init(&nsim_dev->port_functions.pfnum_ida);
+ ida_init(&nsim_dev->port_functions.sfnum_ida);
}
void nsim_dev_port_function_exit(struct nsim_dev *nsim_dev)
{
+ WARN_ON(!ida_is_empty(&nsim_dev->port_functions.sfnum_ida));
+ ida_destroy(&nsim_dev->port_functions.sfnum_ida);
WARN_ON(!ida_is_empty(&nsim_dev->port_functions.pfnum_ida));
ida_destroy(&nsim_dev->port_functions.pfnum_ida);
WARN_ON(!ida_is_empty(&nsim_dev->port_functions.ida));
@@ -119,9 +124,24 @@ nsim_devlink_port_function_alloc(struct nsim_dev *dev, const struct devlink_port
goto fn_ida_err;
port->pfnum = ret;
break;
+ case DEVLINK_PORT_FLAVOUR_PCI_SF:
+ if (attrs->sfnum_valid)
+ ret = ida_alloc_range(&dev->port_functions.sfnum_ida, attrs->sfnum,
+ attrs->sfnum, GFP_KERNEL);
+ else
+ ret = ida_alloc(&dev->port_functions.sfnum_ida, GFP_KERNEL);
+ if (ret < 0)
+ goto fn_ida_err;
+ port->sfnum = ret;
+ port->pfnum = attrs->pfnum;
+ break;
default:
break;
}
+ /* refcount_t is not needed as port is protected by port_functions.mutex.
+ * This count is to keep track of how many SF ports are attached a PF port.
+ */
+ port->refcount = 1;
return port;
fn_ida_err:
@@ -137,6 +157,9 @@ static void nsim_devlink_port_function_free(struct nsim_dev *dev, struct nsim_po
case DEVLINK_PORT_FLAVOUR_PCI_PF:
ida_simple_remove(&dev->port_functions.pfnum_ida, port->pfnum);
break;
+ case DEVLINK_PORT_FLAVOUR_PCI_SF:
+ ida_simple_remove(&dev->port_functions.sfnum_ida, port->sfnum);
+ break;
default:
break;
}
@@ -170,6 +193,11 @@ nsim_dev_port_port_exists(struct nsim_dev *nsim_dev, const struct devlink_port_n
if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF &&
tmp->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF && tmp->pfnum == attrs->pfnum)
return true;
+
+ if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
+ tmp->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
+ tmp->sfnum == attrs->sfnum && tmp->pfnum == attrs->pfnum)
+ return true;
}
return false;
}
@@ -183,21 +211,71 @@ nsim_dev_devlink_port_index_lookup(const struct nsim_dev *nsim_dev, unsigned int
list_for_each_entry(port, &nsim_dev->port_functions.head, list) {
if (port->port_index != port_index)
continue;
+ if (port->refcount > 1) {
+ NL_SET_ERR_MSG_MOD(extack, "Port is in use");
+ return ERR_PTR(-EBUSY);
+ }
return port;
}
NL_SET_ERR_MSG_MOD(extack, "User created port not found");
return ERR_PTR(-ENOENT);
}
+static struct nsim_port_function *
+pf_port_get(struct nsim_dev *nsim_dev, struct nsim_port_function *port)
+{
+ struct nsim_port_function *tmp;
+
+ /* PF port addition doesn't need a parent. */
+ if (port->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
+ return NULL;
+
+ list_for_each_entry(tmp, &nsim_dev->port_functions.head, list) {
+ if (tmp->flavour != DEVLINK_PORT_FLAVOUR_PCI_PF || tmp->pfnum != port->pfnum)
+ continue;
+
+ if (tmp->refcount + 1 == INT_MAX)
+ return ERR_PTR(-ENOSPC);
+
+ port->pf_port = tmp;
+ tmp->refcount++;
+ return tmp;
+ }
+ return ERR_PTR(-ENOENT);
+}
+
+static void pf_port_put(struct nsim_port_function *port)
+{
+ if (port->pf_port) {
+ port->pf_port->refcount--;
+ WARN_ON(port->pf_port->refcount < 0);
+ }
+ port->refcount--;
+ WARN_ON(port->refcount != 0);
+}
+
static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_dev *nsim_dev,
struct nsim_port_function *port,
struct netlink_ext_ack *extack)
{
+ struct nsim_port_function *pf_port;
int err;
- list_add(&port->list, &nsim_dev->port_functions.head);
+ /* Keep all PF ports at the start, so that when driver is unloaded
+ * All SF ports from the end of the list can be removed first.
+ */
+ if (port->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
+ list_add(&port->list, &nsim_dev->port_functions.head);
+ else
+ list_add_tail(&port->list, &nsim_dev->port_functions.head);
+
+ pf_port = pf_port_get(nsim_dev, port);
+ if (IS_ERR(pf_port)) {
+ NL_SET_ERR_MSG_MOD(extack, "Fail to get pf port");
+ err = PTR_ERR(pf_port);
+ goto pf_err;
+ }
- port->state = DEVLINK_PORT_FUNCTION_STATE_INACTIVE;
err = devlink_port_register(devlink, &port->dl_port, port->port_index);
if (err)
goto reg_err;
@@ -213,6 +291,8 @@ static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_d
devlink_port_type_clear(&port->dl_port);
devlink_port_unregister(&port->dl_port);
reg_err:
+ pf_port_put(port);
+pf_err:
list_del(&port->list);
return err;
}
@@ -224,12 +304,14 @@ static void nsim_devlink_port_function_del(struct nsim_dev *nsim_dev,
unregister_netdev(port->netdev);
devlink_port_unregister(&port->dl_port);
list_del(&port->list);
+ pf_port_put(port);
}
static bool nsim_dev_port_flavour_supported(const struct nsim_dev *nsim_dev,
const struct devlink_port_new_attrs *attrs)
{
- return attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF;
+ return attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF ||
+ attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF;
}
int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *attrs,
@@ -266,7 +348,11 @@ int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port
nsim_dev->switch_id.id_len);
port->dl_port.attrs.switch_id.id_len = nsim_dev->switch_id.id_len;
- devlink_port_attrs_pci_pf_set(&port->dl_port, port->controller, port->pfnum, false);
+ if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
+ devlink_port_attrs_pci_pf_set(&port->dl_port, port->controller, port->pfnum, false);
+ else
+ devlink_port_attrs_pci_sf_set(&port->dl_port, port->controller, port->pfnum,
+ port->sfnum, false);
err = nsim_devlink_port_function_add(devlink, nsim_dev, port, extack);
if (err)
@@ -333,6 +419,7 @@ void nsim_dev_port_function_disable(struct nsim_dev *nsim_dev)
* ports.
*/
+ /* Remove SF ports first, followed by PF ports. */
list_for_each_entry_safe_reverse(port, tmp, &nsim_dev->port_functions.head, list) {
nsim_devlink_port_function_del(nsim_dev, port);
nsim_devlink_port_function_free(nsim_dev, port);
--
2.26.2
^ permalink raw reply related [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-17 17:20 ` [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port Parav Pandit
@ 2020-09-17 20:31 ` David Ahern
2020-09-18 3:29 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-17 20:31 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/20 11:20 AM, Parav Pandit wrote:
> Simulate PCI SF ports. Allow user to create one or more PCI SF ports.
>
> Examples:
>
> Create a PCI PF and PCI SF port.
> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
> $ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum 44
> $ devlink port show netdevsim/netdevsim10/11
> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour pcisf controller 0 pfnum 0 sfnum 44 external true splittable false
> function:
> hw_addr 00:00:00:00:00:00 state inactive
>
> $ devlink port function set netdevsim/netdevsim10/11 hw_addr 00:11:22:33:44:55 state active
>
> $ devlink port show netdevsim/netdevsim10/11 -jp
> {
> "port": {
> "netdevsim/netdevsim10/11": {
> "type": "eth",
> "netdev": "eni10npf0sf44",
I could be missing something, but it does not seem like this patch
creates the netdevice for the subfunction.
> "flavour": "pcisf",
> "controller": 0,
> "pfnum": 0,
> "sfnum": 44,
> "external": true,
> "splittable": false,
> "function": {
> "hw_addr": "00:11:22:33:44:55",
> "state": "active"
> }
> }
> }
> }
>
> Delete newly added devlink port
> $ devlink port add netdevsim/netdevsim10/11
>
> Add devlink port of flavour 'pcisf' where port index and sfnum are
> auto assigned by driver.
> $ devlink port add netdevsim/netdevsim10 flavour pcisf controller 0 pfnum 0
>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> ---
> drivers/net/netdevsim/netdevsim.h | 1 +
> drivers/net/netdevsim/port_function.c | 95 +++++++++++++++++++++++++--
> 2 files changed, 92 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 0ea9705eda38..c70782e444d5 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -222,6 +222,7 @@ struct nsim_dev {
> struct list_head head;
> struct ida ida;
> struct ida pfnum_ida;
> + struct ida sfnum_ida;
> } port_functions;
> };
>
> diff --git a/drivers/net/netdevsim/port_function.c b/drivers/net/netdevsim/port_function.c
> index 99581d3d15fe..e1812acd55b4 100644
> --- a/drivers/net/netdevsim/port_function.c
> +++ b/drivers/net/netdevsim/port_function.c
> @@ -13,10 +13,12 @@ struct nsim_port_function {
> unsigned int port_index;
> enum devlink_port_flavour flavour;
> u32 controller;
> + u32 sfnum;
> u16 pfnum;
> struct nsim_port_function *pf_port; /* Valid only for SF port */
> u8 hw_addr[ETH_ALEN];
> u8 state; /* enum devlink_port_function_state */
> + int refcount; /* Counts how many sf ports are bound attached to this pf port. */
> };
>
> void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
> @@ -25,10 +27,13 @@ void nsim_dev_port_function_init(struct nsim_dev *nsim_dev)
> INIT_LIST_HEAD(&nsim_dev->port_functions.head);
> ida_init(&nsim_dev->port_functions.ida);
> ida_init(&nsim_dev->port_functions.pfnum_ida);
> + ida_init(&nsim_dev->port_functions.sfnum_ida);
> }
>
> void nsim_dev_port_function_exit(struct nsim_dev *nsim_dev)
> {
> + WARN_ON(!ida_is_empty(&nsim_dev->port_functions.sfnum_ida));
> + ida_destroy(&nsim_dev->port_functions.sfnum_ida);
> WARN_ON(!ida_is_empty(&nsim_dev->port_functions.pfnum_ida));
> ida_destroy(&nsim_dev->port_functions.pfnum_ida);
> WARN_ON(!ida_is_empty(&nsim_dev->port_functions.ida));
> @@ -119,9 +124,24 @@ nsim_devlink_port_function_alloc(struct nsim_dev *dev, const struct devlink_port
> goto fn_ida_err;
> port->pfnum = ret;
> break;
> + case DEVLINK_PORT_FLAVOUR_PCI_SF:
> + if (attrs->sfnum_valid)
> + ret = ida_alloc_range(&dev->port_functions.sfnum_ida, attrs->sfnum,
> + attrs->sfnum, GFP_KERNEL);
> + else
> + ret = ida_alloc(&dev->port_functions.sfnum_ida, GFP_KERNEL);
> + if (ret < 0)
> + goto fn_ida_err;
> + port->sfnum = ret;
> + port->pfnum = attrs->pfnum;
> + break;
> default:
> break;
> }
> + /* refcount_t is not needed as port is protected by port_functions.mutex.
> + * This count is to keep track of how many SF ports are attached a PF port.
> + */
> + port->refcount = 1;
> return port;
>
> fn_ida_err:
> @@ -137,6 +157,9 @@ static void nsim_devlink_port_function_free(struct nsim_dev *dev, struct nsim_po
> case DEVLINK_PORT_FLAVOUR_PCI_PF:
> ida_simple_remove(&dev->port_functions.pfnum_ida, port->pfnum);
> break;
> + case DEVLINK_PORT_FLAVOUR_PCI_SF:
> + ida_simple_remove(&dev->port_functions.sfnum_ida, port->sfnum);
> + break;
> default:
> break;
> }
> @@ -170,6 +193,11 @@ nsim_dev_port_port_exists(struct nsim_dev *nsim_dev, const struct devlink_port_n
> if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF &&
> tmp->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF && tmp->pfnum == attrs->pfnum)
> return true;
> +
> + if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
> + tmp->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF &&
> + tmp->sfnum == attrs->sfnum && tmp->pfnum == attrs->pfnum)
> + return true;
> }
> return false;
> }
> @@ -183,21 +211,71 @@ nsim_dev_devlink_port_index_lookup(const struct nsim_dev *nsim_dev, unsigned int
> list_for_each_entry(port, &nsim_dev->port_functions.head, list) {
> if (port->port_index != port_index)
> continue;
> + if (port->refcount > 1) {
> + NL_SET_ERR_MSG_MOD(extack, "Port is in use");
> + return ERR_PTR(-EBUSY);
> + }
> return port;
> }
> NL_SET_ERR_MSG_MOD(extack, "User created port not found");
> return ERR_PTR(-ENOENT);
> }
>
> +static struct nsim_port_function *
> +pf_port_get(struct nsim_dev *nsim_dev, struct nsim_port_function *port)
> +{
> + struct nsim_port_function *tmp;
> +
> + /* PF port addition doesn't need a parent. */
> + if (port->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
> + return NULL;
> +
> + list_for_each_entry(tmp, &nsim_dev->port_functions.head, list) {
> + if (tmp->flavour != DEVLINK_PORT_FLAVOUR_PCI_PF || tmp->pfnum != port->pfnum)
> + continue;
> +
> + if (tmp->refcount + 1 == INT_MAX)
> + return ERR_PTR(-ENOSPC);
> +
> + port->pf_port = tmp;
> + tmp->refcount++;
> + return tmp;
> + }
> + return ERR_PTR(-ENOENT);
> +}
> +
> +static void pf_port_put(struct nsim_port_function *port)
> +{
> + if (port->pf_port) {
> + port->pf_port->refcount--;
> + WARN_ON(port->pf_port->refcount < 0);
> + }
> + port->refcount--;
> + WARN_ON(port->refcount != 0);
> +}
> +
> static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_dev *nsim_dev,
> struct nsim_port_function *port,
> struct netlink_ext_ack *extack)
> {
> + struct nsim_port_function *pf_port;
> int err;
>
> - list_add(&port->list, &nsim_dev->port_functions.head);
> + /* Keep all PF ports at the start, so that when driver is unloaded
> + * All SF ports from the end of the list can be removed first.
> + */
> + if (port->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
> + list_add(&port->list, &nsim_dev->port_functions.head);
> + else
> + list_add_tail(&port->list, &nsim_dev->port_functions.head);
> +
> + pf_port = pf_port_get(nsim_dev, port);
> + if (IS_ERR(pf_port)) {
> + NL_SET_ERR_MSG_MOD(extack, "Fail to get pf port");
> + err = PTR_ERR(pf_port);
> + goto pf_err;
> + }
>
> - port->state = DEVLINK_PORT_FUNCTION_STATE_INACTIVE;
> err = devlink_port_register(devlink, &port->dl_port, port->port_index);
> if (err)
> goto reg_err;
> @@ -213,6 +291,8 @@ static int nsim_devlink_port_function_add(struct devlink *devlink, struct nsim_d
> devlink_port_type_clear(&port->dl_port);
> devlink_port_unregister(&port->dl_port);
> reg_err:
> + pf_port_put(port);
> +pf_err:
> list_del(&port->list);
> return err;
> }
> @@ -224,12 +304,14 @@ static void nsim_devlink_port_function_del(struct nsim_dev *nsim_dev,
> unregister_netdev(port->netdev);
> devlink_port_unregister(&port->dl_port);
> list_del(&port->list);
> + pf_port_put(port);
> }
>
> static bool nsim_dev_port_flavour_supported(const struct nsim_dev *nsim_dev,
> const struct devlink_port_new_attrs *attrs)
> {
> - return attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF;
> + return attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF ||
> + attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_SF;
> }
>
> int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port_new_attrs *attrs,
> @@ -266,7 +348,11 @@ int nsim_dev_devlink_port_new(struct devlink *devlink, const struct devlink_port
> nsim_dev->switch_id.id_len);
> port->dl_port.attrs.switch_id.id_len = nsim_dev->switch_id.id_len;
>
> - devlink_port_attrs_pci_pf_set(&port->dl_port, port->controller, port->pfnum, false);
> + if (attrs->flavour == DEVLINK_PORT_FLAVOUR_PCI_PF)
> + devlink_port_attrs_pci_pf_set(&port->dl_port, port->controller, port->pfnum, false);
> + else
> + devlink_port_attrs_pci_sf_set(&port->dl_port, port->controller, port->pfnum,
> + port->sfnum, false);
>
> err = nsim_devlink_port_function_add(devlink, nsim_dev, port, extack);
> if (err)
> @@ -333,6 +419,7 @@ void nsim_dev_port_function_disable(struct nsim_dev *nsim_dev)
> * ports.
> */
>
> + /* Remove SF ports first, followed by PF ports. */
> list_for_each_entry_safe_reverse(port, tmp, &nsim_dev->port_functions.head, list) {
> nsim_devlink_port_function_del(nsim_dev, port);
> nsim_devlink_port_function_free(nsim_dev, port);
>
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-17 20:31 ` David Ahern
@ 2020-09-18 3:29 ` Parav Pandit
2020-09-18 3:38 ` David Ahern
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 3:29 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: David Ahern <dsahern@gmail.com>
> Sent: Friday, September 18, 2020 2:01 AM
>
> On 9/17/20 11:20 AM, Parav Pandit wrote:
> > Simulate PCI SF ports. Allow user to create one or more PCI SF ports.
> >
> > Examples:
> >
> > Create a PCI PF and PCI SF port.
> > $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0 $
> > devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum
> > 44 $ devlink port show netdevsim/netdevsim10/11
> > netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour pcisf
> controller 0 pfnum 0 sfnum 44 external true splittable false
> > function:
> > hw_addr 00:00:00:00:00:00 state inactive
> >
> > $ devlink port function set netdevsim/netdevsim10/11 hw_addr
> > 00:11:22:33:44:55 state active
> >
> > $ devlink port show netdevsim/netdevsim10/11 -jp {
> > "port": {
> > "netdevsim/netdevsim10/11": {
> > "type": "eth",
> > "netdev": "eni10npf0sf44",
>
> I could be missing something, but it does not seem like this patch creates the
> netdevice for the subfunction.
>
The sf port created here is the eswitch port with a valid switch id similar to PF and physical port.
So the netdev created is the representor netdevice.
It is created uniformly for subfunction and pf port flavours.
This series enables user to add PCI PF and subfunction ports.
PF port addition (and its representor netdev) is done in patch 5 [1].
This patch for SF utilizes the same ' struct nsim_port_function' for PF and SF.
Only difference between them is flavour.
[1] https://lore.kernel.org/netdev/20200917172020.26484-6-parav@nvidia.com/
>
> > "flavour": "pcisf",
> > "controller": 0,
> > "pfnum": 0,
> > "sfnum": 44,
> > "external": true,
> > "splittable": false,
> > "function": {
> > "hw_addr": "00:11:22:33:44:55",
> > "state": "active"
> > }
> > }
> > }
> > }
> >
> > Delete newly added devlink port
> > $ devlink port add netdevsim/netdevsim10/11
> >
> > Add devlink port of flavour 'pcisf' where port index and sfnum are
> > auto assigned by driver.
> > $ devlink port add netdevsim/netdevsim10 flavour pcisf controller 0
> > pfnum 0
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 3:29 ` Parav Pandit
@ 2020-09-18 3:38 ` David Ahern
2020-09-18 4:41 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-18 3:38 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/20 9:29 PM, Parav Pandit wrote:
>>> Examples:
>>>
>>> Create a PCI PF and PCI SF port.
>>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0 $
>>> devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0 sfnum
>>> 44 $ devlink port show netdevsim/netdevsim10/11
>>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour pcisf
>> controller 0 pfnum 0 sfnum 44 external true splittable false
>>> function:
>>> hw_addr 00:00:00:00:00:00 state inactive
>>>
>>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
>>> 00:11:22:33:44:55 state active
>>>
>>> $ devlink port show netdevsim/netdevsim10/11 -jp {
>>> "port": {
>>> "netdevsim/netdevsim10/11": {
>>> "type": "eth",
>>> "netdev": "eni10npf0sf44",
>>
>> I could be missing something, but it does not seem like this patch creates the
>> netdevice for the subfunction.
>>
> The sf port created here is the eswitch port with a valid switch id similar to PF and physical port.
> So the netdev created is the representor netdevice.
> It is created uniformly for subfunction and pf port flavours.
To be clear: If I run the devlink commands to create a sub-function, `ip
link show` should list a net_device that corresponds to the sub-function?
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 3:38 ` David Ahern
@ 2020-09-18 4:41 ` Parav Pandit
2020-09-18 4:53 ` Samudrala, Sridhar
2020-09-18 15:23 ` David Ahern
0 siblings, 2 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 4:41 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
Hi David,
> From: David Ahern <dsahern@gmail.com>
> Sent: Friday, September 18, 2020 9:08 AM
>
> On 9/17/20 9:29 PM, Parav Pandit wrote:
> >>> Examples:
> >>>
> >>> Create a PCI PF and PCI SF port.
> >>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0 $
> >>> devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0
> >>> sfnum
> >>> 44 $ devlink port show netdevsim/netdevsim10/11
> >>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour
> >>> pcisf
> >> controller 0 pfnum 0 sfnum 44 external true splittable false
> >>> function:
> >>> hw_addr 00:00:00:00:00:00 state inactive
> >>>
> >>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
> >>> 00:11:22:33:44:55 state active
> >>>
> >>> $ devlink port show netdevsim/netdevsim10/11 -jp {
> >>> "port": {
> >>> "netdevsim/netdevsim10/11": {
> >>> "type": "eth",
> >>> "netdev": "eni10npf0sf44",
> >>
> >> I could be missing something, but it does not seem like this patch
> >> creates the netdevice for the subfunction.
> >>
> > The sf port created here is the eswitch port with a valid switch id similar to PF
> and physical port.
> > So the netdev created is the representor netdevice.
> > It is created uniformly for subfunction and pf port flavours.
>
> To be clear: If I run the devlink commands to create a sub-function, `ip link
> show` should list a net_device that corresponds to the sub-function?
In this series only representor netdevice corresponds to sub-function will be visible in ip link show, i.e. eni10npf0sf44.
Netdevsim is only simulating the eswitch side or control path at present for pf/vf/sf ports.
So other end of this port (netdevice/rdma device/vdpa device) are not yet created.
Subfunction will be anchored on virtbus described in RFC [1], which is not yet in-kernel yet.
Grep for "every SF a device is created on virtbus" to jump to this part of the long RFC.
[1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 4:41 ` Parav Pandit
@ 2020-09-18 4:53 ` Samudrala, Sridhar
2020-09-18 5:10 ` Parav Pandit
2020-09-18 15:23 ` David Ahern
1 sibling, 1 reply; 52+ messages in thread
From: Samudrala, Sridhar @ 2020-09-18 4:53 UTC (permalink / raw)
To: Parav Pandit, David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/2020 9:41 PM, Parav Pandit wrote:
> Hi David,
>
>> From: David Ahern <dsahern@gmail.com>
>> Sent: Friday, September 18, 2020 9:08 AM
>>
>> On 9/17/20 9:29 PM, Parav Pandit wrote:
>>>>> Examples:
>>>>>
>>>>> Create a PCI PF and PCI SF port.
>>>>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0 $
>>>>> devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0
>>>>> sfnum
>>>>> 44 $ devlink port show netdevsim/netdevsim10/11
>>>>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour
>>>>> pcisf
>>>> controller 0 pfnum 0 sfnum 44 external true splittable false
>>>>> function:
>>>>> hw_addr 00:00:00:00:00:00 state inactive
>>>>>
>>>>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
>>>>> 00:11:22:33:44:55 state active
>>>>>
>>>>> $ devlink port show netdevsim/netdevsim10/11 -jp {
>>>>> "port": {
>>>>> "netdevsim/netdevsim10/11": {
>>>>> "type": "eth",
>>>>> "netdev": "eni10npf0sf44",
>>>>
>>>> I could be missing something, but it does not seem like this patch
>>>> creates the netdevice for the subfunction.
>>>>
>>> The sf port created here is the eswitch port with a valid switch id similar to PF
>> and physical port.
>>> So the netdev created is the representor netdevice.
>>> It is created uniformly for subfunction and pf port flavours.
>>
>> To be clear: If I run the devlink commands to create a sub-function, `ip link
>> show` should list a net_device that corresponds to the sub-function?
>
> In this series only representor netdevice corresponds to sub-function will be visible in ip link show, i.e. eni10npf0sf44.
This should be OK if the eSwitch mode is changed to switchdev.
But i think it should be possible to create a subfunction even in legacy
mode where representor netdev is not created.
>
> Netdevsim is only simulating the eswitch side or control path at present for pf/vf/sf ports.
> So other end of this port (netdevice/rdma device/vdpa device) are not yet created.
>
> Subfunction will be anchored on virtbus described in RFC [1], which is not yet in-kernel yet.
> Grep for "every SF a device is created on virtbus" to jump to this part of the long RFC.
>
> [1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
>
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 4:53 ` Samudrala, Sridhar
@ 2020-09-18 5:10 ` Parav Pandit
0 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 5:10 UTC (permalink / raw)
To: Samudrala, Sridhar, David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: Samudrala, Sridhar <sridhar.samudrala@intel.com>
> Sent: Friday, September 18, 2020 10:23 AM
>
>
> On 9/17/2020 9:41 PM, Parav Pandit wrote:
> > Hi David,
> >
> >> From: David Ahern <dsahern@gmail.com>
> >> Sent: Friday, September 18, 2020 9:08 AM
> >>
> >> On 9/17/20 9:29 PM, Parav Pandit wrote:
> >>>>> Examples:
> >>>>>
> >>>>> Create a PCI PF and PCI SF port.
> >>>>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
> >>>>> $ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0
> >>>>> sfnum
> >>>>> 44 $ devlink port show netdevsim/netdevsim10/11
> >>>>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour
> >>>>> pcisf
> >>>> controller 0 pfnum 0 sfnum 44 external true splittable false
> >>>>> function:
> >>>>> hw_addr 00:00:00:00:00:00 state inactive
> >>>>>
> >>>>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
> >>>>> 00:11:22:33:44:55 state active
> >>>>>
> >>>>> $ devlink port show netdevsim/netdevsim10/11 -jp {
> >>>>> "port": {
> >>>>> "netdevsim/netdevsim10/11": {
> >>>>> "type": "eth",
> >>>>> "netdev": "eni10npf0sf44",
> >>>>
> >>>> I could be missing something, but it does not seem like this patch
> >>>> creates the netdevice for the subfunction.
> >>>>
> >>> The sf port created here is the eswitch port with a valid switch id
> >>> similar to PF
> >> and physical port.
> >>> So the netdev created is the representor netdevice.
> >>> It is created uniformly for subfunction and pf port flavours.
> >>
> >> To be clear: If I run the devlink commands to create a sub-function,
> >> `ip link show` should list a net_device that corresponds to the sub-function?
> >
> > In this series only representor netdevice corresponds to sub-function will be
> visible in ip link show, i.e. eni10npf0sf44.
>
> This should be OK if the eSwitch mode is changed to switchdev.
> But i think it should be possible to create a subfunction even in legacy mode
> where representor netdev is not created.
switch_id is optional attribute of the devlink port.
It is applicable only when it is eswitch port in switchdev mode.
>
> >
> > Netdevsim is only simulating the eswitch side or control path at present for
> pf/vf/sf ports.
> > So other end of this port (netdevice/rdma device/vdpa device) are not yet
> created.
> >
> > Subfunction will be anchored on virtbus described in RFC [1], which is not yet
> in-kernel yet.
> > Grep for "every SF a device is created on virtbus" to jump to this part of the
> long RFC.
> >
> > [1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
> >
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 4:41 ` Parav Pandit
2020-09-18 4:53 ` Samudrala, Sridhar
@ 2020-09-18 15:23 ` David Ahern
2020-09-18 15:51 ` Parav Pandit
1 sibling, 1 reply; 52+ messages in thread
From: David Ahern @ 2020-09-18 15:23 UTC (permalink / raw)
To: Parav Pandit, davem, kuba, netdev; +Cc: Jiri Pirko
On 9/17/20 10:41 PM, Parav Pandit wrote:
> Hi David,
>
>> From: David Ahern <dsahern@gmail.com>
>> Sent: Friday, September 18, 2020 9:08 AM
>>
>> On 9/17/20 9:29 PM, Parav Pandit wrote:
>>>>> Examples:
>>>>>
>>>>> Create a PCI PF and PCI SF port.
>>>>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0 $
>>>>> devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0
>>>>> sfnum
>>>>> 44 $ devlink port show netdevsim/netdevsim10/11
>>>>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour
>>>>> pcisf
>>>> controller 0 pfnum 0 sfnum 44 external true splittable false
>>>>> function:
>>>>> hw_addr 00:00:00:00:00:00 state inactive
>>>>>
>>>>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
>>>>> 00:11:22:33:44:55 state active
>>>>>
>>>>> $ devlink port show netdevsim/netdevsim10/11 -jp {
>>>>> "port": {
>>>>> "netdevsim/netdevsim10/11": {
>>>>> "type": "eth",
>>>>> "netdev": "eni10npf0sf44",
>>>>
>>>> I could be missing something, but it does not seem like this patch
>>>> creates the netdevice for the subfunction.
>>>>
>>> The sf port created here is the eswitch port with a valid switch id similar to PF
>> and physical port.
>>> So the netdev created is the representor netdevice.
>>> It is created uniformly for subfunction and pf port flavours.
>>
>> To be clear: If I run the devlink commands to create a sub-function, `ip link
>> show` should list a net_device that corresponds to the sub-function?
>
> In this series only representor netdevice corresponds to sub-function will be visible in ip link show, i.e. eni10npf0sf44.
>
> Netdevsim is only simulating the eswitch side or control path at present for pf/vf/sf ports.
> So other end of this port (netdevice/rdma device/vdpa device) are not yet created.
>
> Subfunction will be anchored on virtbus described in RFC [1], which is not yet in-kernel yet.
> Grep for "every SF a device is created on virtbus" to jump to this part of the long RFC.
>
> [1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
>
Thanks for the reference. I have seen that. I am interested in this idea
of creating netdevs for 'slices' of an asic or nic, but it is not clear
to me how it connects end to end. Will you be able to create a
sub-function based netdevice, assign it limited resources from the nic
and then assign that netdevice to a container for example?
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port
2020-09-18 15:23 ` David Ahern
@ 2020-09-18 15:51 ` Parav Pandit
0 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 15:51 UTC (permalink / raw)
To: David Ahern, davem, kuba, netdev; +Cc: Jiri Pirko
> From: David Ahern <dsahern@gmail.com>
> Sent: Friday, September 18, 2020 8:54 PM
>
> On 9/17/20 10:41 PM, Parav Pandit wrote:
> > Hi David,
> >
> >> From: David Ahern <dsahern@gmail.com>
> >> Sent: Friday, September 18, 2020 9:08 AM
> >>
> >> On 9/17/20 9:29 PM, Parav Pandit wrote:
> >>>>> Examples:
> >>>>>
> >>>>> Create a PCI PF and PCI SF port.
> >>>>> $ devlink port add netdevsim/netdevsim10/10 flavour pcipf pfnum 0
> >>>>> $ devlink port add netdevsim/netdevsim10/11 flavour pcisf pfnum 0
> >>>>> sfnum
> >>>>> 44 $ devlink port show netdevsim/netdevsim10/11
> >>>>> netdevsim/netdevsim10/11: type eth netdev eni10npf0sf44 flavour
> >>>>> pcisf
> >>>> controller 0 pfnum 0 sfnum 44 external true splittable false
> >>>>> function:
> >>>>> hw_addr 00:00:00:00:00:00 state inactive
> >>>>>
> >>>>> $ devlink port function set netdevsim/netdevsim10/11 hw_addr
> >>>>> 00:11:22:33:44:55 state active
> >>>>>
> >>>>> $ devlink port show netdevsim/netdevsim10/11 -jp {
> >>>>> "port": {
> >>>>> "netdevsim/netdevsim10/11": {
> >>>>> "type": "eth",
> >>>>> "netdev": "eni10npf0sf44",
> >>>>
> >>>> I could be missing something, but it does not seem like this patch
> >>>> creates the netdevice for the subfunction.
> >>>>
> >>> The sf port created here is the eswitch port with a valid switch id
> >>> similar to PF
> >> and physical port.
> >>> So the netdev created is the representor netdevice.
> >>> It is created uniformly for subfunction and pf port flavours.
> >>
> >> To be clear: If I run the devlink commands to create a sub-function,
> >> `ip link show` should list a net_device that corresponds to the sub-
> function?
> >
> > In this series only representor netdevice corresponds to sub-function will
> be visible in ip link show, i.e. eni10npf0sf44.
> >
> > Netdevsim is only simulating the eswitch side or control path at present for
> pf/vf/sf ports.
> > So other end of this port (netdevice/rdma device/vdpa device) are not yet
> created.
> >
> > Subfunction will be anchored on virtbus described in RFC [1], which is not
> yet in-kernel yet.
> > Grep for "every SF a device is created on virtbus" to jump to this part of the
> long RFC.
> >
> > [1] https://lore.kernel.org/netdev/20200519092258.GF4655@nanopsycho/
> >
>
> Thanks for the reference. I have seen that. I am interested in this idea of
> creating netdevs for 'slices' of an asic or nic, but it is not clear to me how it
> connects end to end. Will you be able to create a sub-function based
> netdevice, assign it limited resources from the nic and then assign that
> netdevice to a container for example?
Yep. You described is precisely well.
This short series creates eswitch representor for the moment.
Once virtbus is in_kernel, will extend to create actual netdevice of the subfunction too.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-17 17:20 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Parav Pandit
` (7 preceding siblings ...)
2020-09-17 17:20 ` [PATCH net-next v2 8/8] netdevsim: Add support for add and delete PCI SF port Parav Pandit
@ 2020-09-18 16:52 ` Jakub Kicinski
2020-09-18 17:08 ` Parav Pandit
8 siblings, 1 reply; 52+ messages in thread
From: Jakub Kicinski @ 2020-09-18 16:52 UTC (permalink / raw)
To: Parav Pandit; +Cc: davem, netdev
On Thu, 17 Sep 2020 20:20:12 +0300 Parav Pandit wrote:
> Hi Dave, Jakub,
>
> Similar to PCI VF, PCI SF represents portion of the device.
> PCI SF is represented using a new devlink port flavour.
>
> This short series implements small part of the RFC described in detail at [1] and [2].
>
> It extends
> (a) devlink core to expose new devlink port flavour 'pcisf'.
> (b) Expose new user interface to add/delete devlink port.
> (c) Extends netdevsim driver to simulate PCI PF and SF ports
> (d) Add port function state attribute
Is this an RFC? It doesn't add any in-tree users.
^ permalink raw reply [flat|nested] 52+ messages in thread* RE: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 16:52 ` [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops Jakub Kicinski
@ 2020-09-18 17:08 ` Parav Pandit
2020-09-18 17:37 ` Jakub Kicinski
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 17:08 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, September 18, 2020 10:22 PM
>
> On Thu, 17 Sep 2020 20:20:12 +0300 Parav Pandit wrote:
> > Hi Dave, Jakub,
> >
> > Similar to PCI VF, PCI SF represents portion of the device.
> > PCI SF is represented using a new devlink port flavour.
> >
> > This short series implements small part of the RFC described in detail at [1]
> and [2].
> >
> > It extends
> > (a) devlink core to expose new devlink port flavour 'pcisf'.
> > (b) Expose new user interface to add/delete devlink port.
> > (c) Extends netdevsim driver to simulate PCI PF and SF ports
> > (d) Add port function state attribute
>
> Is this an RFC? It doesn't add any in-tree users.
It is not an RFC.
devlink + mlx5 + netdevsim is crossing 25+ patches on eswitch side.
So splitting it to logical piece as devlink + netdevsim.
After which mlx5 eswitch side come close to 15 + 4 patches which can run as two separate patchset.
What do you suggest?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 17:08 ` Parav Pandit
@ 2020-09-18 17:37 ` Jakub Kicinski
2020-09-18 17:47 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Kicinski @ 2020-09-18 17:37 UTC (permalink / raw)
To: Parav Pandit; +Cc: davem, netdev
On Fri, 18 Sep 2020 17:08:15 +0000 Parav Pandit wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, September 18, 2020 10:22 PM
> >
> > On Thu, 17 Sep 2020 20:20:12 +0300 Parav Pandit wrote:
> > > Hi Dave, Jakub,
> > >
> > > Similar to PCI VF, PCI SF represents portion of the device.
> > > PCI SF is represented using a new devlink port flavour.
> > >
> > > This short series implements small part of the RFC described in detail at [1]
> > and [2].
> > >
> > > It extends
> > > (a) devlink core to expose new devlink port flavour 'pcisf'.
> > > (b) Expose new user interface to add/delete devlink port.
> > > (c) Extends netdevsim driver to simulate PCI PF and SF ports
> > > (d) Add port function state attribute
> >
> > Is this an RFC? It doesn't add any in-tree users.
> It is not an RFC.
> devlink + mlx5 + netdevsim is crossing 25+ patches on eswitch side.
> So splitting it to logical piece as devlink + netdevsim.
> After which mlx5 eswitch side come close to 15 + 4 patches which can
> run as two separate patchset.
>
> What do you suggest?
Start with real patches, not netdevsim.
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 17:37 ` Jakub Kicinski
@ 2020-09-18 17:47 ` Parav Pandit
2020-09-18 18:28 ` Jakub Kicinski
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 17:47 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, September 18, 2020 11:07 PM
>
> On Fri, 18 Sep 2020 17:08:15 +0000 Parav Pandit wrote:
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Friday, September 18, 2020 10:22 PM
> > >
> > > On Thu, 17 Sep 2020 20:20:12 +0300 Parav Pandit wrote:
> > > > Hi Dave, Jakub,
> > > >
> > > > Similar to PCI VF, PCI SF represents portion of the device.
> > > > PCI SF is represented using a new devlink port flavour.
> > > >
> > > > This short series implements small part of the RFC described in
> > > > detail at [1]
> > > and [2].
> > > >
> > > > It extends
> > > > (a) devlink core to expose new devlink port flavour 'pcisf'.
> > > > (b) Expose new user interface to add/delete devlink port.
> > > > (c) Extends netdevsim driver to simulate PCI PF and SF ports
> > > > (d) Add port function state attribute
> > >
> > > Is this an RFC? It doesn't add any in-tree users.
> > It is not an RFC.
> > devlink + mlx5 + netdevsim is crossing 25+ patches on eswitch side.
> > So splitting it to logical piece as devlink + netdevsim.
> > After which mlx5 eswitch side come close to 15 + 4 patches which can
> > run as two separate patchset.
> >
> > What do you suggest?
>
> Start with real patches, not netdevsim.
Hmm. Shall I split the series below, would that be ok?
First patchset,
(a) devlink piece to add/delete port
(b) mlx5 counter part
Second patchset,
(a) devlink piece to set the state
(b) mlx5 counter part
Follow on patchset to create/delete sf's netdev on virtbus in mlx5 + devlink plumbing.
Netdevsim after that.
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 17:47 ` Parav Pandit
@ 2020-09-18 18:28 ` Jakub Kicinski
2020-09-18 20:09 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Kicinski @ 2020-09-18 18:28 UTC (permalink / raw)
To: Parav Pandit; +Cc: davem, netdev
On Fri, 18 Sep 2020 17:47:24 +0000 Parav Pandit wrote:
> > > What do you suggest?
> >
> > Start with real patches, not netdevsim.
>
> Hmm. Shall I split the series below, would that be ok?
>
> First patchset,
> (a) devlink piece to add/delete port
> (b) mlx5 counter part
>
> Second patchset,
> (a) devlink piece to set the state
> (b) mlx5 counter part
>
> Follow on patchset to create/delete sf's netdev on virtbus in mlx5 + devlink plumbing.
> Netdevsim after that.
I'd start from the virtbus part so we can see what's being created.
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 18:28 ` Jakub Kicinski
@ 2020-09-18 20:09 ` Parav Pandit
2020-09-21 22:02 ` Jakub Kicinski
0 siblings, 1 reply; 52+ messages in thread
From: Parav Pandit @ 2020-09-18 20:09 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, September 18, 2020 11:58 PM
>
> On Fri, 18 Sep 2020 17:47:24 +0000 Parav Pandit wrote:
> > > > What do you suggest?
> > >
> > > Start with real patches, not netdevsim.
> >
> > Hmm. Shall I split the series below, would that be ok?
> >
> > First patchset,
> > (a) devlink piece to add/delete port
> > (b) mlx5 counter part
> >
> > Second patchset,
> > (a) devlink piece to set the state
> > (b) mlx5 counter part
> >
> > Follow on patchset to create/delete sf's netdev on virtbus in mlx5 + devlink
> plumbing.
> > Netdevsim after that.
>
> I'd start from the virtbus part so we can see what's being created.
How do you reach there without a user interface?
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-18 20:09 ` Parav Pandit
@ 2020-09-21 22:02 ` Jakub Kicinski
2020-09-22 4:37 ` Parav Pandit
0 siblings, 1 reply; 52+ messages in thread
From: Jakub Kicinski @ 2020-09-21 22:02 UTC (permalink / raw)
To: Parav Pandit; +Cc: davem, netdev
On Fri, 18 Sep 2020 20:09:24 +0000 Parav Pandit wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Friday, September 18, 2020 11:58 PM
> >
> > On Fri, 18 Sep 2020 17:47:24 +0000 Parav Pandit wrote:
> > > > > What do you suggest?
> > > >
> > > > Start with real patches, not netdevsim.
> > >
> > > Hmm. Shall I split the series below, would that be ok?
> > >
> > > First patchset,
> > > (a) devlink piece to add/delete port
> > > (b) mlx5 counter part
> > >
> > > Second patchset,
> > > (a) devlink piece to set the state
> > > (b) mlx5 counter part
> > >
> > > Follow on patchset to create/delete sf's netdev on virtbus in mlx5 + devlink
> > plumbing.
> > > Netdevsim after that.
> >
> > I'd start from the virtbus part so we can see what's being created.
>
> How do you reach there without a user interface?
Well.. why do you have a user interface which doesn't cause anything to
happen (devices won't get created)? You're splitting the submission,
it's obvious the implementation won't be complete after the first one.
My expectation is that your implementation of the devlink commands will
just hand them off to FW, so there won't be anything interesting there
to review.
Start with the hard / risky parts - I consider the virtbus to be that,
since the conversation there includes multiple vendors, use cases and
subsystems.
^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [PATCH net-next v2 0/8] devlink: Add SF add/delete devlink ops
2020-09-21 22:02 ` Jakub Kicinski
@ 2020-09-22 4:37 ` Parav Pandit
0 siblings, 0 replies; 52+ messages in thread
From: Parav Pandit @ 2020-09-22 4:37 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, netdev
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, September 22, 2020 3:32 AM
>
> On Fri, 18 Sep 2020 20:09:24 +0000 Parav Pandit wrote:
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Friday, September 18, 2020 11:58 PM
> > >
> > > On Fri, 18 Sep 2020 17:47:24 +0000 Parav Pandit wrote:
> > > > > > What do you suggest?
> > > > >
> > > > > Start with real patches, not netdevsim.
> > > >
> > > > Hmm. Shall I split the series below, would that be ok?
> > > >
> > > > First patchset,
> > > > (a) devlink piece to add/delete port
> > > > (b) mlx5 counter part
> > > >
> > > > Second patchset,
> > > > (a) devlink piece to set the state
> > > > (b) mlx5 counter part
> > > >
> > > > Follow on patchset to create/delete sf's netdev on virtbus in mlx5
> > > > + devlink
> > > plumbing.
> > > > Netdevsim after that.
> > >
> > > I'd start from the virtbus part so we can see what's being created.
> >
> > How do you reach there without a user interface?
>
> Well.. why do you have a user interface which doesn't cause anything to happen
> (devices won't get created)? You're splitting the submission, it's obvious the
> implementation won't be complete after the first one.
>
> My expectation is that your implementation of the devlink commands will just
> hand them off to FW, so there won't be anything interesting there to review.
Correct. Since handing off to firmware and processing event from firmware is creating fairly more patches,
In first series I will just go inline where devlink command would create/remove virtbus device on state active/inactive.
This way it should be possible to have minimal working series under 20 patches.
This way in one patchset, I prefer to cover
(a) devlink plumbing for port add/del, port state
(b) mlx5 eswitch refactor and devlink callback handling
(c) devlink plumbing around
(d) virtbus device and their netdev creation
This also gives complete view from user interface to netdev device creation.
Post this series, will improve the internals of mlx5 via events etc which doesn't need multi-vendor, virtbus and devlink involvement.
Will differ netdevsim to later date.
>
> Start with the hard / risky parts - I consider the virtbus to be that, since the
> conversation there includes multiple vendors, use cases and subsystems.
^ permalink raw reply [flat|nested] 52+ messages in thread