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

[Vincent MAILHOL]

On Wed. 23 Nov. 2022 at 13:12, Jakub Kicinski <kuba@xxxxxxxxxx> 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;
> > +     }