Re: [RFC PATCH] net: devlink: devlink_nl_info_fill: populate default information

On Wed. 23 Nov. 2022 at 13:12, Jakub Kicinski [off-list ref] wrote:

On Wed, 23 Nov 2022 00:49:34 +0900 Vincent Mailhol wrote:

quoted

 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.

quoted

      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?

quoted

+     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;
+     }