Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221122154934.13937-1-mailhol.vincent@wanadoo.fr> <20221122201246.0276680f@kernel.org>
In-Reply-To: <20221122201246.0276680f@kernel.org>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Wed, 23 Nov 2022 18:42:41 +0900
Message-ID: <CAMZ6RqJ8_=h1SS7WmBeEB=75wsvVUZrb-8ELCDtpZb0gSs=2+A@mail.gmail.com>
Subject: Re: [RFC PATCH] net: devlink: devlink_nl_info_fill: populate default information
To:     Jakub Kicinski <kuba@kernel.org>
Cc:     Jiri Pirko <jiri@nvidia.com>, netdev@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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