Message ID | 20221122154934.13937-1-mailhol.vincent@wanadoo.fr |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp2292515wrr; Tue, 22 Nov 2022 07:58:55 -0800 (PST) X-Google-Smtp-Source: AA0mqf5KDoCip+70u6/Om8Yg0O5uNPdoRLhXcYrEonInJ0+Esg46ZX3F0d6NeMG2NkI6TYHZfuIM X-Received: by 2002:a63:2705:0:b0:477:4ba4:d966 with SMTP id n5-20020a632705000000b004774ba4d966mr3975247pgn.528.1669132735238; Tue, 22 Nov 2022 07:58:55 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1669132735; cv=none; d=google.com; s=arc-20160816; b=awiG8NLCJ/TjRZoLMyFBY1LjKjFz1LPpJLGnxdJomRKA48UGfDkaYkk/pT2z6jTfXb ow7mJIQDRYGHHwgoq18NEE7D8mndf5kzrasR5uj7l9fMv0isiIyyN41lKCSeuWfqEOZu yOwI+1prCMfFJG//9QIVEoGFyerLdl0+SVGBjh5ur+7Nce0W/0D8Ngb4Vw3m+KZFRjIt ha42UtZzsTHtjUr9EwWRPaMoEecaFnYeHj9ZxdXd2e73JOi+NvGIEvfssbKjLPyfT4Xy HyqrvjAxT6xJvCqTAS1qMIf6P5LQ8IPXtWHvcDQsXCgCLEonIqvgUxlpfUfVwbrJAYG7 BXTg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:sender:dkim-signature; bh=XzoAp9Plv3egb3kfLGSXnYMVJkluSY/7fd0B6sswIds=; b=njCn4KifnctDqhBeCFRXPjwxPoOrSa9Qa6fE7HPgKvRwZJtJx0DLJmxbL1YFdyV3QN 8a9rsbLjM1iV07BRmQNRw7d77GtFhqi8NcpO5W/hDv/mIbPYHzyQg1fLY+neihssNuKa PPdiDpJD5G21JkuucEDKb9dM9Ht+2X0EBbCdfS0jwddYLVOG/3FbCj/Xa5CDMn9oUgWN uWfSePmpqxURnDQAMm27WOHn5X8vqyD15qCnNLLrZcu+L3kvy1KcKcZXxwH1NfzVMNRc 1f1T39mHIdjSW5+Rb7U74eZUCevqMFgFwNFp074jrZZOv8CfaM10kdKhHo4mo7MTOKKV zL+A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=INdoxMR4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id pv16-20020a17090b3c9000b001fd9be4fb6csi17255793pjb.39.2022.11.22.07.58.41; Tue, 22 Nov 2022 07:58:55 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20210112 header.b=INdoxMR4; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233701AbiKVPuG (ORCPT <rfc822;cjcooper78@gmail.com> + 99 others); Tue, 22 Nov 2022 10:50:06 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55888 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229449AbiKVPuE (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 22 Nov 2022 10:50:04 -0500 Received: from mail-pj1-x102a.google.com (mail-pj1-x102a.google.com [IPv6:2607:f8b0:4864:20::102a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 411BF63B84; Tue, 22 Nov 2022 07:50:03 -0800 (PST) Received: by mail-pj1-x102a.google.com with SMTP id ci10so7391955pjb.1; Tue, 22 Nov 2022 07:50:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:sender:from:to:cc:subject:date:message-id:reply-to; bh=XzoAp9Plv3egb3kfLGSXnYMVJkluSY/7fd0B6sswIds=; b=INdoxMR4pymr7XaCszMHSiwlodvYNMw0hTQ1XFdS3pyv1vqEJBHN3y8nUlf0UqgYCr rx8nNbdWgidBnRcU0ojvgzjVKCnY5F6IvNu2XgqfdXCTM7EQvPkSk9McnvmGCN0NJfLB pHMrYxvw+Q+U/FtheOSfmdsRwZ1aEdU+laEgbByMp5v/fkONQ4vzH1IzkJ1O/stgISAx AzoU2eVklfrsW9psoTB83i0NzGmPIZ45gr2yJ5y9Qm4vNIyafOowwXgr7Blo7M0KcINV Tux9ssO02KUiZJFaIf8YpNarnpL1c9BM8WmAFZW4t9P0h2nBJTcfy9w1vovvk8zdt85J tDog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:sender:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=XzoAp9Plv3egb3kfLGSXnYMVJkluSY/7fd0B6sswIds=; b=t19eMS1XRXCU5L834r2dzlLkc2ecPOIGVEfJePS7laiOTLo/MYTynqPM2qKAP9GZ+6 L8C7YWGF3YdBXObKLZRs8kayFjGx2lbga45KB9IlBIM5i8N8pCljJBWwYsVJ6CxRiWUK s8oY2QFU1zDS9+uusLHUOqsgaB+gpQCvCcm8Q9IuWacoz9PeN0bGVCLOvXj45VAcnICj 26fZVJ78TTfus7fZpe6zMwS3kTNau7rRTE+FzgjjYZlUo80z3yZ+AlT0WhtK/RrsrI97 9TK8x4j4hw/00tFUcpSXEgHyPdeLNBq4X35EDWbWvlwSnYBSSAKADJkqAv8B2V3/WD+c 3k5A== X-Gm-Message-State: ANoB5plksHtzGd+Dgc8uXbnQj4vFxHmM3H7dBQDLz6fvrSoo0LPVib8v WvLyUUcR7EXFhhM7OF5CbmA= X-Received: by 2002:a17:902:c286:b0:176:a880:6d72 with SMTP id i6-20020a170902c28600b00176a8806d72mr6034538pld.127.1669132202607; Tue, 22 Nov 2022 07:50:02 -0800 (PST) Received: from localhost.localdomain (124x33x176x97.ap124.ftth.ucom.ne.jp. [124.33.176.97]) by smtp.gmail.com with ESMTPSA id e12-20020a17090301cc00b001868981a18esm12251637plh.6.2022.11.22.07.50.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 22 Nov 2022 07:50:02 -0800 (PST) Sender: Vincent Mailhol <vincent.mailhol@gmail.com> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr> To: Jiri Pirko <jiri@nvidia.com>, netdev@vger.kernel.org Cc: "David S . Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org, Vincent Mailhol <mailhol.vincent@wanadoo.fr> Subject: [RFC PATCH] net: devlink: devlink_nl_info_fill: populate default information Date: Wed, 23 Nov 2022 00:49:34 +0900 Message-Id: <20221122154934.13937-1-mailhol.vincent@wanadoo.fr> X-Mailer: git-send-email 2.37.4 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_EF,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1750212526874005266?= X-GMAIL-MSGID: =?utf-8?q?1750212526874005266?= |
Series |
[RFC] net: devlink: devlink_nl_info_fill: populate default information
|
|
Commit Message
Vincent Mailhol
Nov. 22, 2022, 3:49 p.m. UTC
Some piece of information are common to the vast majority of the
devices. Examples are:
* the driver name.
* the serial number of a USB device.
Modify devlink_nl_info_fill() to retrieve those information so that
the drivers do not have to. Rationale: factorize code.
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
I am sending this as an RFC because I just started to study devlink.
I can see a parallel with ethtool for which the core will fill
whatever it can. c.f.:
commit f20a0a0519f3 ("ethtool: doc: clarify what drivers can implement in their get_drvinfo()")
Link: https://git.kernel.org/netdev/net-next/c/f20a0a0519f3
I think that devlink should do the same.
Right now, I identified two fields. If this RFC receive positive
feedback, I will iron it up and try to see if there is more that can
be filled by default.
Thank you for your comments.
---
net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
Comments
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 > 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? 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 > + 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; > + }
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; > > + }
Tue, Nov 22, 2022 at 04:49:34PM CET, mailhol.vincent@wanadoo.fr wrote: >Some piece of information are common to the vast majority of the >devices. Examples are: > > * the driver name. > * the serial number of a USB device. > >Modify devlink_nl_info_fill() to retrieve those information so that >the drivers do not have to. Rationale: factorize code. > >Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> >--- >I am sending this as an RFC because I just started to study devlink. > >I can see a parallel with ethtool for which the core will fill >whatever it can. c.f.: >commit f20a0a0519f3 ("ethtool: doc: clarify what drivers can implement in their get_drvinfo()") >Link: https://git.kernel.org/netdev/net-next/c/f20a0a0519f3 > >I think that devlink should do the same. > >Right now, I identified two fields. If this RFC receive positive >feedback, I will iron it up and try to see if there is more that can >be filled by default. > >Thank you for your comments. >--- > net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > >diff --git a/net/core/devlink.c b/net/core/devlink.c >index 7f789bbcbbd7..1908b360caf7 100644 >--- a/net/core/devlink.c >+++ b/net/core/devlink.c >@@ -18,6 +18,7 @@ > #include <linux/netdevice.h> > #include <linux/spinlock.h> > #include <linux/refcount.h> >+#include <linux/usb.h> > #include <linux/workqueue.h> > #include <linux/u64_stats_sync.h> > #include <linux/timekeeping.h> >@@ -6685,12 +6686,37 @@ int devlink_info_version_running_put_ext(struct devlink_info_req *req, > } > EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext); > >+static int devlink_nl_driver_info_get(struct device_driver *drv, >+ struct devlink_info_req *req) >+{ >+ if (!drv) >+ return 0; >+ >+ if (drv->name[0]) >+ return devlink_info_driver_name_put(req, drv->name); Make sure that this provides the same value for all existing drivers using devlink. >+ >+ return 0; >+} >+ >+static int devlink_nl_usb_info_get(struct usb_device *udev, >+ struct devlink_info_req *req) >+{ >+ if (!udev) >+ return 0; >+ >+ if (udev->serial[0]) >+ return devlink_info_serial_number_put(req, udev->serial); >+ >+ return 0; >+} >+ > 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); > 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; >+ >+ if (!strcmp(dev->parent->type->name, "usb_device")) { Comparing to string does not seem correct here. >+ err = devlink_nl_usb_info_get(to_usb_device(dev->parent), &req); As Jakub pointed out, you have to make sure that driver does not put the same attrs again. You have to introduce this functionality with removing the fill-ups in drivers atomically, in a single patch. >+ if (err) >+ goto err_cancel_msg; >+ } >+ > genlmsg_end(msg, hdr); > return 0; > >-- >2.37.4 >
On Wed. 23 nov. 2022 à 18:46, Jiri Pirko <jiri@nvidia.com> wrote: > Tue, Nov 22, 2022 at 04:49:34PM CET, mailhol.vincent@wanadoo.fr wrote: > >Some piece of information are common to the vast majority of the > >devices. Examples are: > > > > * the driver name. > > * the serial number of a USB device. > > > >Modify devlink_nl_info_fill() to retrieve those information so that > >the drivers do not have to. Rationale: factorize code. > > > >Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >--- > >I am sending this as an RFC because I just started to study devlink. > > > >I can see a parallel with ethtool for which the core will fill > >whatever it can. c.f.: > >commit f20a0a0519f3 ("ethtool: doc: clarify what drivers can implement in their get_drvinfo()") > >Link: https://git.kernel.org/netdev/net-next/c/f20a0a0519f3 > > > >I think that devlink should do the same. > > > >Right now, I identified two fields. If this RFC receive positive > >feedback, I will iron it up and try to see if there is more that can > >be filled by default. > > > >Thank you for your comments. > >--- > > net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > > >diff --git a/net/core/devlink.c b/net/core/devlink.c > >index 7f789bbcbbd7..1908b360caf7 100644 > >--- a/net/core/devlink.c > >+++ b/net/core/devlink.c > >@@ -18,6 +18,7 @@ > > #include <linux/netdevice.h> > > #include <linux/spinlock.h> > > #include <linux/refcount.h> > >+#include <linux/usb.h> > > #include <linux/workqueue.h> > > #include <linux/u64_stats_sync.h> > > #include <linux/timekeeping.h> > >@@ -6685,12 +6686,37 @@ int devlink_info_version_running_put_ext(struct devlink_info_req *req, > > } > > EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext); > > > >+static int devlink_nl_driver_info_get(struct device_driver *drv, > >+ struct devlink_info_req *req) > >+{ > >+ if (!drv) > >+ return 0; > >+ > >+ if (drv->name[0]) > >+ return devlink_info_driver_name_put(req, drv->name); > > Make sure that this provides the same value for all existing drivers > using devlink. There are 21 drivers so far which reports the driver name through devlink. c.f.: $ git grep "devlink_info_driver_name_put(" drivers | wc -l Out of those 21, there is only one: the mlxsw which seems to report something different than device_driver::name. Instead it reports some bus_info: https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/net/ethernet/mellanox/mlxsw/core.c#L1462 https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/net/ethernet/mellanox/mlxsw/core.h#L504 I am not sure what the bus_info is here, but it looks like a misuse of the field here. > > >+ > >+ return 0; > >+} > >+ > >+static int devlink_nl_usb_info_get(struct usb_device *udev, > >+ struct devlink_info_req *req) > >+{ > >+ if (!udev) > >+ return 0; > >+ > >+ if (udev->serial[0]) > >+ return devlink_info_serial_number_put(req, udev->serial); > >+ > >+ return 0; > >+} > >+ > > 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); > > 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; > >+ > >+ if (!strcmp(dev->parent->type->name, "usb_device")) { > > Comparing to string does not seem correct here. There is a is_usb_device() which does the check: https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/usb/core/usb.h#L152 but this macro is not exposed outside of the usb core. The string comparison was the only solution I found. Do you have any other ideas? If not and if this goes further than the RFC stage, I will ask the USB folks if there is a better way. > > >+ err = devlink_nl_usb_info_get(to_usb_device(dev->parent), &req); > > As Jakub pointed out, you have to make sure that driver does not put the > same attrs again. You have to introduce this functionality with removing > the fill-ups in drivers atomically, in a single patch. Either this, either track if the attribute is already set. I would prefer to remove all drivers fill-ups but this is not feasible for the serial number. c.f. my reply to Jacub in this thread: https://lore.kernel.org/netdev/CAMZ6RqJ8_=h1SS7WmBeEB=75wsvVUZrb-8ELCDtpZb0gSs=2+A@mail.gmail.com/ > >+ if (err) > >+ goto err_cancel_msg; > >+ } > >+ > > genlmsg_end(msg, hdr); > > return 0; > > > >-- > >2.37.4 > >
On Wed. 23 Nov. 2022 at 21:10, Jiri Pirko <jiri@nvidia.com> wrote: > Wed, Nov 23, 2022 at 12:00:44PM CET, mailhol.vincent@wanadoo.fr wrote: > >On Wed. 23 nov. 2022 à 18:46, Jiri Pirko <jiri@nvidia.com> wrote: > >> Tue, Nov 22, 2022 at 04:49:34PM CET, mailhol.vincent@wanadoo.fr wrote: > >> >Some piece of information are common to the vast majority of the > >> >devices. Examples are: > >> > > >> > * the driver name. > >> > * the serial number of a USB device. > >> > > >> >Modify devlink_nl_info_fill() to retrieve those information so that > >> >the drivers do not have to. Rationale: factorize code. > >> > > >> >Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > >> >--- > >> >I am sending this as an RFC because I just started to study devlink. > >> > > >> >I can see a parallel with ethtool for which the core will fill > >> >whatever it can. c.f.: > >> >commit f20a0a0519f3 ("ethtool: doc: clarify what drivers can implement in their get_drvinfo()") > >> >Link: https://git.kernel.org/netdev/net-next/c/f20a0a0519f3 > >> > > >> >I think that devlink should do the same. > >> > > >> >Right now, I identified two fields. If this RFC receive positive > >> >feedback, I will iron it up and try to see if there is more that can > >> >be filled by default. > >> > > >> >Thank you for your comments. > >> >--- > >> > net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++ > >> > 1 file changed, 36 insertions(+) > >> > > >> >diff --git a/net/core/devlink.c b/net/core/devlink.c > >> >index 7f789bbcbbd7..1908b360caf7 100644 > >> >--- a/net/core/devlink.c > >> >+++ b/net/core/devlink.c > >> >@@ -18,6 +18,7 @@ > >> > #include <linux/netdevice.h> > >> > #include <linux/spinlock.h> > >> > #include <linux/refcount.h> > >> >+#include <linux/usb.h> > >> > #include <linux/workqueue.h> > >> > #include <linux/u64_stats_sync.h> > >> > #include <linux/timekeeping.h> > >> >@@ -6685,12 +6686,37 @@ int devlink_info_version_running_put_ext(struct devlink_info_req *req, > >> > } > >> > EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext); > >> > > >> >+static int devlink_nl_driver_info_get(struct device_driver *drv, > >> >+ struct devlink_info_req *req) > >> >+{ > >> >+ if (!drv) > >> >+ return 0; > >> >+ > >> >+ if (drv->name[0]) > >> >+ return devlink_info_driver_name_put(req, drv->name); > >> > >> Make sure that this provides the same value for all existing drivers > >> using devlink. > > > >There are 21 drivers so far which reports the driver name through devlink. c.f.: > > $ git grep "devlink_info_driver_name_put(" drivers | wc -l > > > >Out of those 21, there is only one: the mlxsw which seems to report > >something different than device_driver::name. Instead it reports some > >bus_info: > > https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/net/ethernet/mellanox/mlxsw/core.c#L1462 > > https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/net/ethernet/mellanox/mlxsw/core.h#L504 > > > >I am not sure what the bus_info is here, but it looks like a misuse of > >the field here. > > When you are not sure, look into the code to find out :) I see no misue. > What exactly do you mean by that? I mean that device_kind, it does not sound like a field that would hold the driver name. Looking deeper in the code, I got the confirmation. bus_info::device_kind is initialized here (among other): https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/net/ethernet/mellanox/mlxsw/i2c.c#L714 and it uses ic2_client::name which indicate the type of the device (e.g. chip name): https://elixir.bootlin.com/linux/v6.1-rc1/source/include/linux/i2c.h#L317 So I confirm that this is a misuse. This driver does not report the driver's name. > >> >+ > >> >+ return 0; > >> >+} > >> >+ > >> >+static int devlink_nl_usb_info_get(struct usb_device *udev, > >> >+ struct devlink_info_req *req) > >> >+{ > >> >+ if (!udev) > >> >+ return 0; > >> >+ > >> >+ if (udev->serial[0]) > >> >+ return devlink_info_serial_number_put(req, udev->serial); > >> >+ > >> >+ return 0; > >> >+} > >> >+ > >> > 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); > >> > 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; > >> >+ > >> >+ if (!strcmp(dev->parent->type->name, "usb_device")) { > >> > >> Comparing to string does not seem correct here. > > > >There is a is_usb_device() which does the check: > > https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/usb/core/usb.h#L152 > > > >but this macro is not exposed outside of the usb core. The string > >comparison was the only solution I found. > > Find a different one. String check here is wrong. > > > >Do you have any other ideas? If not and if this goes further than the > >RFC stage, I will ask the USB folks if there is a better way. > > > >> > >> >+ err = devlink_nl_usb_info_get(to_usb_device(dev->parent), &req); > >> > >> As Jakub pointed out, you have to make sure that driver does not put the > >> same attrs again. You have to introduce this functionality with removing > >> the fill-ups in drivers atomically, in a single patch. > > > >Either this, either track if the attribute is already set. I would > >prefer to remove all drivers fill-ups but this is not feasible for the > >serial number. c.f. my reply to Jacub in this thread: > > https://lore.kernel.org/netdev/CAMZ6RqJ8_=h1SS7WmBeEB=75wsvVUZrb-8ELCDtpZb0gSs=2+A@mail.gmail.com/ > > Sure, but for the driver name it is. Let's start there. I will do a first patch only for the driver name and think again of the USB serial later on. Yours sincerely, Vincent Mailhol
On Wed, 23 Nov 2022 18:42:41 +0900 Vincent MAILHOL wrote: > 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/. I think 2/ is best because it will generalize to serial numbers while 1/ will likely not. 3/ is a smaller gain. Jiri already plumbed thru the struct devlink_info_req which is on the stack of the caller, per request, so we can add the bool / bitmap for already reported items there quite easily.
On Thu. 24 Nov. 2022 at 12:06, Jakub Kicinski <kuba@kernel.org> wrote: > On Wed, 23 Nov 2022 18:42:41 +0900 Vincent MAILHOL wrote: > > 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/. > > I think 2/ is best because it will generalize to serial numbers while > 1/ will likely not. 3/ is a smaller gain. > > Jiri already plumbed thru the struct devlink_info_req which is on the > stack of the caller, per request, so we can add the bool / bitmap for > already reported items there quite easily. Sorry, let me clarify the next actions. Are you meaning that Jiri is already working on the bitmap implementation and should I wait for his patches first? Or do you expect me to do it?
Thu, Nov 24, 2022 at 06:33:58AM CET, mailhol.vincent@wanadoo.fr wrote: >On Thu. 24 Nov. 2022 at 12:06, Jakub Kicinski <kuba@kernel.org> wrote: >> On Wed, 23 Nov 2022 18:42:41 +0900 Vincent MAILHOL wrote: >> > 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/. >> >> I think 2/ is best because it will generalize to serial numbers while >> 1/ will likely not. 3/ is a smaller gain. >> >> Jiri already plumbed thru the struct devlink_info_req which is on the >> stack of the caller, per request, so we can add the bool / bitmap for >> already reported items there quite easily. > >Sorry, let me clarify the next actions. Are you meaning that Jiri is >already working on the bitmap implementation and should I wait for his >patches first? Or do you expect me to do it? I'm not.
On Wed. 23 Nov. 2022 at 21:10, Jiri Pirko <jiri@nvidia.com> wrote: > Wed, Nov 23, 2022 at 12:00:44PM CET, mailhol.vincent@wanadoo.fr wrote: > >On Wed. 23 nov. 2022 à 18:46, Jiri Pirko <jiri@nvidia.com> wrote: > >> Tue, Nov 22, 2022 at 04:49:34PM CET, mailhol.vincent@wanadoo.fr wrote: (...) > >> >+ if (!strcmp(dev->parent->type->name, "usb_device")) { > >> > >> Comparing to string does not seem correct here. > > > >There is a is_usb_device() which does the check: > > https://elixir.bootlin.com/linux/v6.1-rc1/source/drivers/usb/core/usb.h#L152 > > > >but this macro is not exposed outside of the usb core. The string > >comparison was the only solution I found. > > Find a different one. String check here is wrong. After looking again, I found no alternative and so I asked the USB mailing list and got an answer from Greg. There is no ways to do so and the class code is not supposed to do this: https://lore.kernel.org/linux-usb/Y3+VfNdt%2FK7UtRcw@kroah.com/ So I guess that there will be no code factorization for device specific values. @Jakub, with this in mind, does it still make sense to add the bitmap? Aside from the driver name, it seems that there will be no code factorization for other types dependent values. If we only set the driver name at the core level, I would rather just clean up the existing. (Side comment, I finished implementing the bitmap just before receiving Greg's answer; I guess my code will go to the trash can...)
On Thu, 24 Nov 2022 14:33:58 +0900 Vincent MAILHOL wrote: > > I think 2/ is best because it will generalize to serial numbers while > > 1/ will likely not. 3/ is a smaller gain. > > > > Jiri already plumbed thru the struct devlink_info_req which is on the > > stack of the caller, per request, so we can add the bool / bitmap for > > already reported items there quite easily. > > Sorry, let me clarify the next actions. Are you meaning that Jiri is > already working on the bitmap implementation and should I wait for his > patches first? Or do you expect me to do it? Dunno if the question still stands but we already have struct devlink_info_req and can put the bitmap in it. All drivers use devlink helpers to add attributes.
On Tue. 29 Nov. 2022 at 03:43, Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 24 Nov 2022 14:33:58 +0900 Vincent MAILHOL wrote: > > > I think 2/ is best because it will generalize to serial numbers while > > > 1/ will likely not. 3/ is a smaller gain. > > > > > > Jiri already plumbed thru the struct devlink_info_req which is on the > > > stack of the caller, per request, so we can add the bool / bitmap for > > > already reported items there quite easily. > > > > Sorry, let me clarify the next actions. Are you meaning that Jiri is > > already working on the bitmap implementation and should I wait for his > > patches first? Or do you expect me to do it? > > Dunno if the question still stands but we already have > struct devlink_info_req and can put the bitmap in it. > All drivers use devlink helpers to add attributes. Roger that. I already wrote the code, I just need to do a bit of ironing and extra testing. I will send it after the "net: devlink: devlink_nl_info_fill: populate default information" series gets merged.
diff --git a/net/core/devlink.c b/net/core/devlink.c index 7f789bbcbbd7..1908b360caf7 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -18,6 +18,7 @@ #include <linux/netdevice.h> #include <linux/spinlock.h> #include <linux/refcount.h> +#include <linux/usb.h> #include <linux/workqueue.h> #include <linux/u64_stats_sync.h> #include <linux/timekeeping.h> @@ -6685,12 +6686,37 @@ int devlink_info_version_running_put_ext(struct devlink_info_req *req, } EXPORT_SYMBOL_GPL(devlink_info_version_running_put_ext); +static int devlink_nl_driver_info_get(struct device_driver *drv, + struct devlink_info_req *req) +{ + if (!drv) + return 0; + + if (drv->name[0]) + return devlink_info_driver_name_put(req, drv->name); + + return 0; +} + +static int devlink_nl_usb_info_get(struct usb_device *udev, + struct devlink_info_req *req) +{ + if (!udev) + return 0; + + if (udev->serial[0]) + return devlink_info_serial_number_put(req, udev->serial); + + return 0; +} + 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); 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; + + 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; + } + genlmsg_end(msg, hdr); return 0;