| Message ID | 20201215090358.240365-5-saeed@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add mlx5 subfunction support | expand |
On Tue, 15 Dec 2020 01:03:47 -0800 Saeed Mahameed wrote: > From: Parav Pandit <parav@nvidia.com> > > 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 existing devlink APIs. > This also helps to uniformly use the code for port unregistration > during driver unload and during port deletion initiated by user. > > Examples of add, show and delete commands: > $ devlink dev eswitch set pci/0000:06:00.0 mode switchdev > > $ devlink port show > pci/0000:06:00.0/65535: type eth netdev ens2f0np0 flavour physical port 0 splittable false > > $ devlink port add pci/0000:06:00.0 flavour pcisf pfnum 0 sfnum 88 > > $ devlink port show pci/0000:06:00.0/32768 > pci/0000:06:00.0/32768: type eth netdev eth0 flavour pcisf controller 0 pfnum 0 sfnum 88 external false splittable false > function: > hw_addr 00:00:00:00:88:88 state inactive opstate detached > > $ 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=enp6s0f0npf0sf88 > ID_NET_NAME_SLOT=ens2f0npf0sf88 > Unload module index > Unloaded link configuration context. > > Signed-off-by: Parav Pandit <parav@nvidia.com> > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Reviewed-by: Vu Pham <vuhuong@nvidia.com> > Signed-off-by: Saeed Mahameed <saeedm@nvidia.com> > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 5bd43f0a79a8..f8cff3e402da 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -153,6 +153,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; Oh. So you had the structure which actually gets stored in memory for the lifetime of the device in patch 3 mispacked (u32 / u16 / u32 / u8). But this one with arguments is packed. Please be consistent. > + 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; > @@ -1363,6 +1374,34 @@ 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. Add a new port of a specified flavor with optional attributes. > + * Driver should return -EOPNOTSUPP if it doesn't support port addition s/should/must/ > + * of a specified flavour or specified attributes. Driver should set > + * extack error message in case of fail to add the port. Devlink core s/fail to add the port/failure/ > + * does not hold a devlink instance lock when this callback is invoked. Called without holding the devlink instance lock. > + * Driver must ensures synchronization when adding or deleting a port. s/ensures/ensure/ but really that's pretty obvious from the previous sentence. > + * Driver must register a port with devlink core. s/must/is expected to/ Please make sure your comments and documentation are proof read by someone. > +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; > + } This is the desired port index of the new port? Or the index of the parent port? Let's make it abundantly clear since its a pass-thru argument for the driver to interpret. > + 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; Why is this check not at the beginning of the function? Also should there be an extack on it? > + return devlink->ops->port_new(devlink, &new_attrs, extack); This should return the identifier of the created port back to user space.
> From: Jakub Kicinski <kuba@kernel.org> > Sent: Wednesday, December 16, 2020 5:59 AM > > > +struct devlink_port_new_attrs { > > + enum devlink_port_flavour flavour; > > + unsigned int port_index; > > + u32 controller; > > + u32 sfnum; > > + u16 pfnum; > > Oh. So you had the structure which actually gets stored in memory for the > lifetime of the device in patch 3 mispacked (u32 / u16 / u32 / u8). > But this one with arguments is packed. Please be consistent. > Ok. I will change the packing in patch 3. > > + 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; > > @@ -1363,6 +1374,34 @@ 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. > > Add a new port of a specified flavor with optional attributes. > > > + * Driver should return -EOPNOTSUPP if it doesn't support port > > +addition > > s/should/must/ > Ack. > > + * of a specified flavour or specified attributes. Driver should set > > + * extack error message in case of fail to add the port. Devlink > > +core > > s/fail to add the port/failure/ > Ack. > > + * does not hold a devlink instance lock when this callback is invoked. > > Called without holding the devlink instance lock. > Ack. > > + * Driver must ensures synchronization when adding or deleting a > port. > > s/ensures/ensure/ but really that's pretty obvious from the previous > sentence. > It may be, but this extra clarity helps, so I am going to keep this explicit description. > > + * Driver must register a port with devlink core. > > s/must/is expected to/ > Ack. > Please make sure your comments and documentation are proof read by > someone. > Ack. > > +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; > > + } > > This is the desired port index of the new port? Yes. > Let's make it abundantly clear since its a pass-thru argument for the driver to > interpret. > Ok. Will add comment here. > > + 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; > > Why is this check not at the beginning of the function? Will move it up. > Also should there be an extack on it? > Will check, and add if required. > > + return devlink->ops->port_new(devlink, &new_attrs, extack); > > This should return the identifier of the created port back to user space. Ok. Will add.
diff --git a/include/net/devlink.h b/include/net/devlink.h index 5bd43f0a79a8..f8cff3e402da 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -153,6 +153,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; @@ -1363,6 +1374,34 @@ 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 08eac247f200..11043707f63f 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -1146,6 +1146,61 @@ 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, @@ -7604,6 +7659,10 @@ static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = { [DEVLINK_ATTR_RELOAD_ACTION] = NLA_POLICY_RANGE(NLA_U8, DEVLINK_RELOAD_ACTION_DRIVER_REINIT, DEVLINK_RELOAD_ACTION_MAX), [DEVLINK_ATTR_RELOAD_LIMITS] = NLA_POLICY_BITFIELD32(DEVLINK_RELOAD_LIMITS_VALID_MASK), + [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_small_ops devlink_nl_ops[] = { @@ -7643,6 +7702,18 @@ static const struct genl_small_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,