| Message ID | 20221122154934.13937-1-mailhol.vincent@wanadoo.fr (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] net: devlink: devlink_nl_info_fill: populate default information | expand |
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 > >
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? > >> >> >+ >> >+ 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. > >> >+ 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
Wed, Nov 23, 2022 at 05:08:10PM CET, mailhol.vincent@wanadoo.fr wrote: >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. Okay, I think that is a bug of mlxsw_i2c implementation. You can fix it along the way. > >> >> >+ >> >> >+ 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. Yep, thanks! > > >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;
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(+)