Messages in this thread | | | From | Vincent MAILHOL <> | Date | Wed, 23 Nov 2022 18:42:41 +0900 | Subject | Re: [RFC PATCH] net: devlink: devlink_nl_info_fill: populate default information |
| |
On Wed. 23 Nov. 2022 at 13:12, Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 23 Nov 2022 00:49:34 +0900 Vincent Mailhol wrote: > > static int > > devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink, > > enum devlink_command cmd, u32 portid, > > u32 seq, int flags, struct netlink_ext_ack *extack) > > { > > struct devlink_info_req req = {}; > > + struct device *dev = devlink_to_dev(devlink); > > nit: longest to shortest lines
I was not aware of this convention. Thanks for the hint.
> > void *hdr; > > int err; > > > > @@ -6707,6 +6733,16 @@ devlink_nl_info_fill(struct sk_buff *msg, struct devlink *devlink, > > if (err) > > goto err_cancel_msg; > > > > + err = devlink_nl_driver_info_get(dev->driver, &req); > > + if (err) > > + goto err_cancel_msg; > > won't this result in repeated attributes, potentially?
You are right. It will because nla_put() doesn't check if an attribute already exists and unconditionally reserves new space: https://elixir.bootlin.com/linux/v6.0/source/lib/nlattr.c#L993
> Unlike ethtool which copies data into a struct devlink > adds an attribute each time you request. It does not override. > So we need to extend req with some tracking of whether driver > already put in the info in question
I see three solutions:
1/ Do it in the core, clean up all drivers using devlink_info_driver_name_put() and make the function static (i.e. forbid the drivers to set the driver name themselves). N.B. This first solution does not work for devlink_info_serial_number_put() because the core will not always be able to provide a default value (e.g. my code only covers USB devices).
2/ Keep track of which attribute is already set (as you suggested).
3/ Do a function devlink_nl_info_fill_default() and let the drivers choose to either call that function or set the attributes themselves.
I would tend to go with a mix of 1/ and 2/.
What do you think?
> > + if (!strcmp(dev->parent->type->name, "usb_device")) { > > + err = devlink_nl_usb_info_get(to_usb_device(dev->parent), &req); > > + if (err) > > + goto err_cancel_msg; > > + }
| |