| Message ID | 20200917172020.26484-3-parav@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | devlink: Add SF add/delete devlink ops | expand |
> From: Jacob Keller <jacob.e.keller@intel.com> > Sent: Friday, September 18, 2020 12:13 AM > > > On 9/17/2020 10:20 AM, Parav Pandit wrote: > > 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. > > > > Ok. Seems like a good goal to be able to share code uniformly between driver > load and new port creation. > Yes. > > +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]); > > + > > Presuming that the device supports it, this could be used to allow creating other > types of ports bsides subfunctions? > This series is creating PCI PF and subfunction ports. Jiri's RFC [1] explained a possibility for VF representors to follow the similar scheme if device supports it. I am not sure creating other port flavours are useful enough such as CPU, PHYSICAL etc. I do not have enough knowledge about use case for creating CPU ports, if at all it exists. Usually physical ports are linked to a card hardware on how many physical ports present on circuit. So I find it odd if a device support physical port creation, but again its my limited view at the moment. > > + 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; > > + } > > So if the userspace doesn't provide a port index, drivers are responsible for > choosing one? Same for the other attributes I suppose? Yes. > > > + 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); } > > +
On 9/17/2020 9:25 PM, Parav Pandit wrote: >> From: Jacob Keller <jacob.e.keller@intel.com> >> Sent: Friday, September 18, 2020 12:13 AM >> >> >> On 9/17/2020 10:20 AM, Parav Pandit wrote: >>> 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. >>> >> >> Ok. Seems like a good goal to be able to share code uniformly between driver >> load and new port creation. >> > Yes. > >>> +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]); >>> + >> >> Presuming that the device supports it, this could be used to allow creating other >> types of ports bsides subfunctions? >> > This series is creating PCI PF and subfunction ports. > Jiri's RFC [1] explained a possibility for VF representors to follow the similar scheme if device supports it. > Right, VFs was the most obvious point. The ability to create VFs without needing to destroy all VFs and re-create them seems quite useful. > I am not sure creating other port flavours are useful enough such as CPU, PHYSICAL etc. > I do not have enough knowledge about use case for creating CPU ports, if at all it exists. > Usually physical ports are linked to a card hardware on how many physical ports present on circuit. > So I find it odd if a device support physical port creation, but again its my limited view at the moment. > Yea, I agree here too. I find that somewhat odd, but I suppose for everything but PHYSICAL types it's not impossible.
> From: Jacob Keller <jacob.e.keller@intel.com> > Sent: Saturday, September 19, 2020 4:37 AM > > > On 9/17/2020 9:25 PM, Parav Pandit wrote: > >> From: Jacob Keller <jacob.e.keller@intel.com> > >> Sent: Friday, September 18, 2020 12:13 AM > >> > >> > >> On 9/17/2020 10:20 AM, Parav Pandit wrote: > >>> 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. > >>> > >> > >> Ok. Seems like a good goal to be able to share code uniformly between > >> driver load and new port creation. > >> > > Yes. > > > >>> +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]); > >>> + > >> > >> Presuming that the device supports it, this could be used to allow > >> creating other types of ports bsides subfunctions? > >> > > This series is creating PCI PF and subfunction ports. > > Jiri's RFC [1] explained a possibility for VF representors to follow the similar > scheme if device supports it. > > > > Right, VFs was the most obvious point. The ability to create VFs without needing > to destroy all VFs and re-create them seems quite useful. > Yes. > > I am not sure creating other port flavours are useful enough such as CPU, > PHYSICAL etc. > > I do not have enough knowledge about use case for creating CPU ports, if at > all it exists. > > Usually physical ports are linked to a card hardware on how many physical > ports present on circuit. > > So I find it odd if a device support physical port creation, but again its my > limited view at the moment. > > > Yea, I agree here too. I find that somewhat odd, but I suppose for everything but > PHYSICAL types it's not impossible. Ok.
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,
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(+)