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 <[email protected]>
---
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);
+
+ 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;
--
2.37.4
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 <[email protected]> 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, [email protected] 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 <[email protected]>
>---
>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 <[email protected]> wrote:
> Tue, Nov 22, 2022 at 04:49:34PM CET, [email protected] 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 <[email protected]>
> >---
> >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 <[email protected]> wrote:
> Wed, Nov 23, 2022 at 12:00:44PM CET, [email protected] wrote:
> >On Wed. 23 nov. 2022 à 18:46, Jiri Pirko <[email protected]> wrote:
> >> Tue, Nov 22, 2022 at 04:49:34PM CET, [email protected] 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 <[email protected]>
> >> >---
> >> >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 <[email protected]> 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, [email protected] wrote:
>On Thu. 24 Nov. 2022 at 12:06, Jakub Kicinski <[email protected]> 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 <[email protected]> wrote:
> Wed, Nov 23, 2022 at 12:00:44PM CET, [email protected] wrote:
> >On Wed. 23 nov. 2022 à 18:46, Jiri Pirko <[email protected]> wrote:
> >> Tue, Nov 22, 2022 at 04:49:34PM CET, [email protected] 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%[email protected]/
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 <[email protected]> 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.