| Message ID | 20221126162211.93322-4-mailhol.vincent@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | can: etas_es58x: report firmware, bootloader and hardware version | expand |
> +struct es58x_sw_version { > + u8 major; > + u8 minor; > + u8 revision; > +}; > +static int es58x_devlink_info_get(struct devlink *devlink, > + struct devlink_info_req *req, > + struct netlink_ext_ack *extack) > +{ > + struct es58x_device *es58x_dev = devlink_priv(devlink); > + struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version; > + struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version; > + struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision; > + char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))]; > + int ret = 0; > + > + if (es58x_sw_version_is_set(fw_ver)) { > + snprintf(buf, sizeof(buf), "%02u.%02u.%02u", > + fw_ver->major, fw_ver->minor, fw_ver->revision); I see you have been very careful here, but i wonder if you might still get some compiler/static code analyser warnings here. As far as i remember %02u does not limit it to two characters. If the number is bigger than 99, it will take three characters. And your types are u8, so the compiler could consider these to be 3 characters each. So you end up truncating. Which you look to of done correctly, but i wonder if some over zealous checker will report it? Maybe consider "xxx.xxx.xxx"? Nice paranoid code by the way. I'm not the best at spotting potential buffer overflows, but this code looks good. The only question i had left was how well sscanf() deals with UTF-8. Andrew
Hi Andrew, Thank you for the review and the interesting comments on the parsing. On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@lunn.ch> wrote: > > +struct es58x_sw_version { > > + u8 major; > > + u8 minor; > > + u8 revision; > > +}; > > > +static int es58x_devlink_info_get(struct devlink *devlink, > > + struct devlink_info_req *req, > > + struct netlink_ext_ack *extack) > > +{ > > + struct es58x_device *es58x_dev = devlink_priv(devlink); > > + struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version; > > + struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version; > > + struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision; > > + char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))]; > > + int ret = 0; > > + > > + if (es58x_sw_version_is_set(fw_ver)) { > > + snprintf(buf, sizeof(buf), "%02u.%02u.%02u", > > + fw_ver->major, fw_ver->minor, fw_ver->revision); > > I see you have been very careful here, but i wonder if you might still > get some compiler/static code analyser warnings here. As far as i > remember %02u does not limit it to two characters. I checked, none of gcc and clang would trigger a warning even for a 'make W=12'. More generally speaking, I made sure that my driver is free of any W=12. (except from the annoying spam from GENMASK_INPUT_CHECK for which my attempts to silence it were rejected: https://lore.kernel.org/all/20220426161658.437466-1-mailhol.vincent@wanadoo.fr/ ). > If the number is > bigger than 99, it will take three characters. And your types are u8, > so the compiler could consider these to be 3 characters each. So you > end up truncating. Which you look to of done correctly, but i wonder > if some over zealous checker will report it? That zealous check is named -Wformat-truncation in gcc (I did not find it in clang). Even W=3 doesn't report it so I consider this to be fine. > Maybe consider "xxx.xxx.xxx"? If I do that, I also need to consider the maximum length of the hardware revision would be "a/xxxxx/xxxxx" because the numbers are u16. The declaration would become: char buf[max(sizeof("xxx.xxx.xxx"), sizeof("axxxxx/xxxxx"))]; Because no such warning exists in the kernel, I do not think the above line to be a good trade off. I would like to keep things as they are, it is easier to read. That said, I will add an extra check in es58x_parse_sw_version() and es58x_parse_hw_revision() to assert that the number are not bigger than 99 for the software version (and not bigger than 999 for the hardware revision). That way the code will guarantee that the truncation can never occur. > Nice paranoid code by the way. I'm not the best at spotting potential > buffer overflows, but this code looks good. The only question i had > left was how well sscanf() deals with UTF-8. It does not consider UTF-8. The %u is a _parse_integer_limit() in disguise. https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3637 https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L70 _parse_integer_limit() just check for ASCII digits so the first UTF-8 character would make the function return. https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/kstrtox.c#L65 For example, this: "ＦＷ:03.00.06" would fail parsing because sscanf() will not be able to match the first byte of the UTF-8 'Ｆ' with 'F'. Another example: "FW:０３.００.０６" would also fail parsing because _parse_integer_limit() will not recognize the first byte of UTF-8 '０' as a valid ASCII digit and return early. To finish, a very edge case: "FW:03.00.0６" would incorrectly succeed. It will parse "FW:03.00.0" successfully and will return when encountering the UTF-8 '６'. But I am not willing to cover that edge case. If the device goes into this level of perversion, I do not care any more as long as it does not result in undefined behaviour. Yours sincerely, Vincent Mailhol
On Sun. 27 Nov. 2022 at 12:42, Vincent MAILHOL <mailhol.vincent@wanadoo.fr> wrote: > Hi Andrew, > > Thank you for the review and the interesting comments on the parsing. > > On. 27 Nov. 2022 at 02:37, Andrew Lunn <andrew@lunn.ch> wrote: > > > +struct es58x_sw_version { > > > + u8 major; > > > + u8 minor; > > > + u8 revision; > > > +}; > > > > > +static int es58x_devlink_info_get(struct devlink *devlink, > > > + struct devlink_info_req *req, > > > + struct netlink_ext_ack *extack) > > > +{ > > > + struct es58x_device *es58x_dev = devlink_priv(devlink); > > > + struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version; > > > + struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version; > > > + struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision; > > > + char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))]; > > > + int ret = 0; > > > + > > > + if (es58x_sw_version_is_set(fw_ver)) { > > > + snprintf(buf, sizeof(buf), "%02u.%02u.%02u", > > > + fw_ver->major, fw_ver->minor, fw_ver->revision); > > > > I see you have been very careful here, but i wonder if you might still > > get some compiler/static code analyser warnings here. As far as i > > remember %02u does not limit it to two characters. > > I checked, none of gcc and clang would trigger a warning even for a > 'make W=12'. More generally speaking, I made sure that my driver is > free of any W=12. > (except from the annoying spam from GENMASK_INPUT_CHECK for which my > attempts to silence it were rejected: > https://lore.kernel.org/all/20220426161658.437466-1-mailhol.vincent@wanadoo.fr/ > ). > > > If the number is > > bigger than 99, it will take three characters. And your types are u8, > > so the compiler could consider these to be 3 characters each. So you > > end up truncating. Which you look to of done correctly, but i wonder > > if some over zealous checker will report it? > > That zealous check is named -Wformat-truncation in gcc (I did not find > it in clang). Even W=3 doesn't report it so I consider this to be > fine. > > > Maybe consider "xxx.xxx.xxx"? > > If I do that, I also need to consider the maximum length of the > hardware revision would be "a/xxxxx/xxxxx" because the numbers are > u16. The declaration would become: > > char buf[max(sizeof("xxx.xxx.xxx"), sizeof("axxxxx/xxxxx"))]; > > Because no such warning exists in the kernel, I do not think the above > line to be a good trade off. I would like to keep things as they are, > it is easier to read. That said, I will add an extra check in > es58x_parse_sw_version() and es58x_parse_hw_revision() to assert that > the number are not bigger than 99 for the software version (and not > bigger than 999 for the hardware revision). That way the code will > guarantee that the truncation can never occur. Never mind. I forgot that I already accounted for that. The "%2u" format in sscanf() will detect if the number is three or more digits. So I am thinking of leaving everything as-is. > > Nice paranoid code by the way. I'm not the best at spotting potential > > buffer overflows, but this code looks good. The only question i had > > left was how well sscanf() deals with UTF-8. > > It does not consider UTF-8. The %u is a _parse_integer_limit() in disguise. > https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3637 > https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L70 > > _parse_integer_limit() just check for ASCII digits so the first UTF-8 > character would make the function return. > https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/kstrtox.c#L65 > > For example, this: > "ＦＷ:03.00.06" > would fail parsing because sscanf() will not be able to match the > first byte of the UTF-8 'Ｆ' with 'F'. > > Another example: > "FW:０３.００.０６" > would also fail parsing because _parse_integer_limit() will not > recognize the first byte of UTF-8 '０' as a valid ASCII digit and > return early. > > To finish, a very edge case: > "FW:03.00.0６" > would incorrectly succeed. It will parse "FW:03.00.0" successfully and > will return when encountering the UTF-8 '６'. But I am not willing to > cover that edge case. If the device goes into this level of > perversion, I do not care any more as long as it does not result in > undefined behaviour. > > > Yours sincerely, > Vincent Mailhol
> I checked, none of gcc and clang would trigger a warning even for a > 'make W=12'. More generally speaking, I made sure that my driver is > free of any W=12. That is good enough for me. > I do not care any more as long as it does not result in > undefined behaviour. Agreed. Hopefully sscanf cannot go completely wrong and go off the end of the buffer. That i would care about. Bit i guess the USB fuzzers would of hit such problems already. Andrew
On Mon. 28 Nov. 2022 at 00:08, Andrew Lunn <andrew@lunn.ch> wrote: > > I checked, none of gcc and clang would trigger a warning even for a > > 'make W=12'. More generally speaking, I made sure that my driver is > > free of any W=12. > > That is good enough for me. > > > I do not care any more as long as it does not result in > > undefined behaviour. > > Agreed. Hopefully sscanf cannot go completely wrong and go off the end > of the buffer. That i would care about. Bit i guess the USB fuzzers > would of hit such problems already. On the surface, the sscanf() seems OK. It will break the while loop when reaching the end of the format: https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3429 or the end of the string: https://elixir.bootlin.com/linux/v6.1-rc6/source/lib/vsprintf.c#L3501 (I am skipping details here, there are other branches that will break the while loop and all of them look good). And me not being the first person using sscanf(), I hope that if a bug existed, it would have already been spotted by some static analysis/fuzzing/code review :) That said, I think I answered all your comments. Can I get your reviewed-by or ack tag? Thank you! Yours sincerely, Vincent Mailhol
> That said, I think I answered all your comments. Can I get your > reviewed-by or ack tag? Thank you! Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
> devlink does not yet have a name suited for the bootloader and so this > last piece of information is exposed to the userland for through a > custom name: "bl". Jiri, what do you think about 'bl'? Is it too short, not well known enough? It could easily be 'bootloader'. Andrew
On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote: > > devlink does not yet have a name suited for the bootloader and so this > > last piece of information is exposed to the userland for through a > > custom name: "bl". > > Jiri, what do you think about 'bl'? Is it too short, not well known > enough? It could easily be 'bootloader'. For the record, I name it "bl" by analogy with the firmware which is named "fw". My personal preference would have been to name the fields without any abbreviations: "firmware", "bootloader" and "hardware.revision" (for reference ethtool -i uses "firmware-version"). But I tried to put my personal taste aside and try to fit with the devlink trends to abbreviate things. Thus the name "bl". Yours sincerely, Vincent Mailhol
On Mon, 28 Nov 2022 23:43:19 +0900 Vincent MAILHOL wrote: > On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote: > > > devlink does not yet have a name suited for the bootloader and so this > > > last piece of information is exposed to the userland for through a > > > custom name: "bl". > > > > Jiri, what do you think about 'bl'? Is it too short, not well known > > enough? It could easily be 'bootloader'. > > For the record, I name it "bl" by analogy with the firmware which is > named "fw". My personal preference would have been to name the fields > without any abbreviations: "firmware", "bootloader" and > "hardware.revision" (for reference ethtool -i uses > "firmware-version"). But I tried to put my personal taste aside and > try to fit with the devlink trends to abbreviate things. Thus the name > "bl". Agreed, I thought "fw" is sufficiently universally understood to be used but "bl" is most definitely not :S I'd suggest "fw.bootloader". Also don't hesitate to add that to the "well known" list in devlink.h, I reckon it will be used by others sooner or later.
On Tue. 29 Nov. 2022 at 07:27, Jakub Kicinski <kuba@kernel.org> wrote: > On Mon, 28 Nov 2022 23:43:19 +0900 Vincent MAILHOL wrote: > > On Mon. 28 Nov. 2022 at 22:49, Andrew Lunn <andrew@lunn.ch> wrote: > > > > devlink does not yet have a name suited for the bootloader and so this > > > > last piece of information is exposed to the userland for through a > > > > custom name: "bl". > > > > > > Jiri, what do you think about 'bl'? Is it too short, not well known > > > enough? It could easily be 'bootloader'. > > > > For the record, I name it "bl" by analogy with the firmware which is > > named "fw". My personal preference would have been to name the fields > > without any abbreviations: "firmware", "bootloader" and > > "hardware.revision" (for reference ethtool -i uses > > "firmware-version"). But I tried to put my personal taste aside and > > try to fit with the devlink trends to abbreviate things. Thus the name > > "bl". > > Agreed, I thought "fw" is sufficiently universally understood to be used > but "bl" is most definitely not :S I'd suggest "fw.bootloader". Also > don't hesitate to add that to the "well known" list in devlink.h, > I reckon it will be used by others sooner or later. I like the "fw.bootloader" suggestion. A bootloader is technically still a firmware. I will send a separate patch to add the entry to devlink.h and only then send the v5.
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c index c6e598e4800c..d29c1bf90d73 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.c +++ b/drivers/net/can/usb/etas_es58x/es58x_core.c @@ -2250,6 +2250,7 @@ static int es58x_probe(struct usb_interface *intf, if (ret) return ret; + es58x_parse_product_info(es58x_dev); devlink_register(priv_to_devlink(es58x_dev)); for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) { diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h index bf24375580e5..9481f0764131 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_core.h +++ b/drivers/net/can/usb/etas_es58x/es58x_core.h @@ -356,6 +356,39 @@ struct es58x_operators { int (*get_timestamp)(struct es58x_device *es58x_dev); }; +/** + * struct es58x_sw_version - Version number of the firmware or the + * bootloader. + * @major: Version major number, represented on two digits. + * @minor: Version minor number, represented on two digits. + * @revision: Version revision number, represented on two digits. + * + * The firmware and the bootloader share the same format: "xx.xx.xx" + * where 'x' is a digit. Both can be retrieved from the product + * information string. + */ +struct es58x_sw_version { + u8 major; + u8 minor; + u8 revision; +}; + +/** + * struct es58x_hw_revision - Hardware revision number. + * @letter: Revision letter. + * @major: Version major number, represented on three digits. + * @minor: Version minor number, represented on three digits. + * + * The hardware revision uses its own format: "axxx/xxx" where 'a' is + * a letter and 'x' a digit. It can be retrieved from the product + * information string. + */ +struct es58x_hw_revision { + char letter; + u16 major; + u16 minor; +}; + /** * struct es58x_device - All information specific to an ES58X device. * @dev: Device information. @@ -373,6 +406,9 @@ struct es58x_operators { * queue wake/stop logic should prevent this URB from getting * empty. Please refer to es58x_get_tx_urb() for more details. * @tx_urbs_idle_cnt: number of urbs in @tx_urbs_idle. + * @firmware_version: The firmware version number. + * @bootloader_version: The bootloader version number. + * @hardware_revision: The hardware revision number. * @ktime_req_ns: kernel timestamp when es58x_set_realtime_diff_ns() * was called. * @realtime_diff_ns: difference in nanoseconds between the clocks of @@ -408,6 +444,10 @@ struct es58x_device { struct usb_anchor tx_urbs_idle; atomic_t tx_urbs_idle_cnt; + struct es58x_sw_version firmware_version; + struct es58x_sw_version bootloader_version; + struct es58x_hw_revision hardware_revision; + u64 ktime_req_ns; s64 realtime_diff_ns; @@ -420,6 +460,32 @@ struct es58x_device { union es58x_urb_cmd rx_cmd_buf; }; +/** + * es58x_sw_version_is_set() - Check if the version is a valid number. + * @sw_ver: Version number of either the firmware or the bootloader. + * + * If &es58x_sw_version.major, &es58x_sw_version.minor and + * &es58x_sw_version.revision are all zero, the product string could + * not be parsed and the version number is invalid. + */ +static inline bool es58x_sw_version_is_set(struct es58x_sw_version *sw_ver) +{ + return sw_ver->major || sw_ver->minor || sw_ver->revision; +} + +/** + * es58x_hw_revision_is_set() - Check if the revision is a valid number. + * @hw_rev: Revision number of the hardware. + * + * If &es58x_hw_revision.letter, &es58x_hw_revision.major and + * &es58x_hw_revision.minor are all zero, the product string could not + * be parsed and the hardware revision number is invalid. + */ +static inline bool es58x_hw_revision_is_set(struct es58x_hw_revision *hw_rev) +{ + return hw_rev->letter || hw_rev->major || hw_rev->minor; +} + /** * es58x_sizeof_es58x_device() - Calculate the maximum length of * struct es58x_device. @@ -693,6 +759,7 @@ int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id, const void *msg, u16 cmd_len, int channel_idx); /* es58x_devlink.c. */ +void es58x_parse_product_info(struct es58x_device *es58x_dev); extern const struct devlink_ops es58x_dl_ops; /* es581_4.c. */ diff --git a/drivers/net/can/usb/etas_es58x/es58x_devlink.c b/drivers/net/can/usb/etas_es58x/es58x_devlink.c index af6ca7ada23f..7b67682b952e 100644 --- a/drivers/net/can/usb/etas_es58x/es58x_devlink.c +++ b/drivers/net/can/usb/etas_es58x/es58x_devlink.c @@ -7,7 +7,201 @@ * Copyright (c) 2022 Vincent Mailhol <mailhol.vincent@wanadoo.fr> */ +#include <linux/ctype.h> +#include <linux/device.h> +#include <linux/usb.h> #include <net/devlink.h> +#include "es58x_core.h" + +/* USB descriptor index containing the product information string. */ +#define ES58X_PROD_INFO_IDX 6 + +/** + * es58x_parse_sw_version() - Extract boot loader or firmware version. + * @es58x_dev: ES58X device. + * @prod_info: USB custom string returned by the device. + * @prefix: Select which information should be parsed. Set it to "FW" + * to parse the firmware version or to "BL" to parse the + * bootloader version. + * + * The @prod_info string contains the firmware and the bootloader + * version number all prefixed by a magic string and concatenated with + * other numbers. Depending on the device, the firmware (bootloader) + * format is either "FW_Vxx.xx.xx" ("BL_Vxx.xx.xx") or "FW:xx.xx.xx" + * ("BL:xx.xx.xx") where 'x' represents a digit. @prod_info must + * contains the common part of those prefixes: "FW" or "BL". + * + * Parse @prod_info and store the version number in + * &es58x_dev.firmware_version or &es58x_dev.bootloader_version + * according to @prefix value. + * + * Return: zero on success, -EINVAL if @prefix contains an invalid + * value and -EBADMSG if @prod_info could not be parsed. + */ +static int es58x_parse_sw_version(struct es58x_device *es58x_dev, + const char *prod_info, const char *prefix) +{ + struct es58x_sw_version *version; + int major, minor, revision; + + if (!strcmp(prefix, "FW")) + version = &es58x_dev->firmware_version; + else if (!strcmp(prefix, "BL")) + version = &es58x_dev->bootloader_version; + else + return -EINVAL; + + /* Go to prefix */ + prod_info = strstr(prod_info, prefix); + if (!prod_info) + return -EBADMSG; + /* Go to beginning of the version number */ + while (!isdigit(*prod_info)) { + prod_info++; + if (!*prod_info) + return -EBADMSG; + } + + if (sscanf(prod_info, "%2u.%2u.%2u", &major, &minor, &revision) != 3) + return -EBADMSG; + + version->major = major; + version->minor = minor; + version->revision = revision; + + return 0; +} + +/** + * es58x_parse_hw_rev() - Extract hardware revision number. + * @es58x_dev: ES58X device. + * @prod_info: USB custom string returned by the device. + * + * @prod_info contains the hardware revision prefixed by a magic + * string and conquenated together with other numbers. Depending on + * the device, the hardware revision format is either + * "HW_VER:axxx/xxx" or "HR:axxx/xxx" where 'a' represents a letter + * and 'x' a digit. + * + * Parse @prod_info and store the hardware revision number in + * &es58x_dev.hardware_revision. + * + * Return: zero on success, -EBADMSG if @prod_info could not be + * parsed. + */ +static int es58x_parse_hw_rev(struct es58x_device *es58x_dev, + const char *prod_info) +{ + char letter; + int major, minor; + + /* The only occurrence of 'H' is in the hardware revision prefix. */ + prod_info = strchr(prod_info, 'H'); + if (!prod_info) + return -EBADMSG; + /* Go to beginning of the hardware revision */ + prod_info = strchr(prod_info, ':'); + if (!prod_info) + return -EBADMSG; + prod_info++; + + if (sscanf(prod_info, "%c%3u/%3u", &letter, &major, &minor) != 3) + return -EBADMSG; + + es58x_dev->hardware_revision.letter = letter; + es58x_dev->hardware_revision.major = major; + es58x_dev->hardware_revision.minor = minor; + + return 0; +} + +/** + * es58x_parse_product_info() - Parse the ES58x product information + * string. + * @es58x_dev: ES58X device. + * + * Retrieve the product information string and parse it to extract the + * firmware version, the bootloader version and the hardware + * revision. + * + * If the function fails, simply emit a log message and continue + * because product information is not critical for the driver to + * operate. + */ +void es58x_parse_product_info(struct es58x_device *es58x_dev) +{ + char *prod_info; + + prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX); + if (!prod_info) { + dev_warn(es58x_dev->dev, + "could not retrieve the product info string\n"); + return; + } + + if (es58x_parse_sw_version(es58x_dev, prod_info, "FW") || + es58x_parse_sw_version(es58x_dev, prod_info, "BL") || + es58x_parse_hw_rev(es58x_dev, prod_info)) + dev_info(es58x_dev->dev, + "could not parse product info: '%s'\n", prod_info); + + kfree(prod_info); +} + +/** + * es58x_devlink_info_get() - Report the product information. + * @devlink: Devlink. + * @req: skb wrapper where to put requested information. + * @extack: Unused. + * + * Report the firmware version, the bootloader version, the hardware + * revision and the serial number through netlink. + * + * Return: zero on success, errno when any error occurs. + */ +static int es58x_devlink_info_get(struct devlink *devlink, + struct devlink_info_req *req, + struct netlink_ext_ack *extack) +{ + struct es58x_device *es58x_dev = devlink_priv(devlink); + struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version; + struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version; + struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision; + char buf[max(sizeof("xx.xx.xx"), sizeof("axxx/xxx"))]; + int ret = 0; + + if (es58x_sw_version_is_set(fw_ver)) { + snprintf(buf, sizeof(buf), "%02u.%02u.%02u", + fw_ver->major, fw_ver->minor, fw_ver->revision); + ret = devlink_info_version_running_put(req, + DEVLINK_INFO_VERSION_GENERIC_FW, + buf); + if (ret) + return ret; + } + + if (es58x_sw_version_is_set(bl_ver)) { + snprintf(buf, sizeof(buf), "%02u.%02u.%02u", + bl_ver->major, bl_ver->minor, bl_ver->revision); + ret = devlink_info_version_running_put(req, "bl", buf); + if (ret) + return ret; + } + + if (es58x_hw_revision_is_set(hw_rev)) { + snprintf(buf, sizeof(buf), "%c%03u/%03u", + hw_rev->letter, hw_rev->major, hw_rev->minor); + ret = devlink_info_version_fixed_put(req, + DEVLINK_INFO_VERSION_GENERIC_BOARD_REV, + buf); + if (ret) + return ret; + } + + return devlink_info_serial_number_put(req, es58x_dev->udev->serial); +} + const struct devlink_ops es58x_dl_ops = { + .info_get = es58x_devlink_info_get, };
ES58x devices report below product information through a custom usb string: * the firmware version * the bootloader version * the hardware revision Parse this string, store the results in struct es58x_dev, export the firmware version through devlink's "fw" name and the hardware revision through devlink's "board.rev" name. devlink does not yet have a name suited for the bootloader and so this last piece of information is exposed to the userland for through a custom name: "bl". Those devlink entries are not critical to use the device, if parsing fails, print an informative log message and continue to probe the device. In addition to that, report the device serial number which is available in usb_device::serial. Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- drivers/net/can/usb/etas_es58x/es58x_core.c | 1 + drivers/net/can/usb/etas_es58x/es58x_core.h | 67 ++++++ .../net/can/usb/etas_es58x/es58x_devlink.c | 194 ++++++++++++++++++ 3 files changed, 262 insertions(+)