lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 16 Dec 2020 05:06:37 +0000 From: Parav Pandit <parav@...dia.com> To: Jakub Kicinski <kuba@...nel.org>, Saeed Mahameed <saeed@...nel.org> CC: "David S. Miller" <davem@...emloft.net>, Jason Gunthorpe <jgg@...dia.com>, Leon Romanovsky <leonro@...dia.com>, "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>, David Ahern <dsahern@...nel.org>, Jacob Keller <jacob.e.keller@...el.com>, Sridhar Samudrala <sridhar.samudrala@...el.com>, "david.m.ertman@...el.com" <david.m.ertman@...el.com>, "dan.j.williams@...el.com" <dan.j.williams@...el.com>, "kiran.patil@...el.com" <kiran.patil@...el.com>, "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>, Jiri Pirko <jiri@...dia.com>, Vu Pham <vuhuong@...dia.com>, Saeed Mahameed <saeedm@...dia.com> Subject: RE: [net-next v5 04/15] devlink: Support add and delete devlink port > From: Jakub Kicinski <kuba@...nel.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.
Powered by blists - more mailing lists