ES58x devices report below information in their usb product info
string:
* the firmware version
* the bootloader version
* the hardware revision
Parse this string, store the results in struct es58x_dev and create
three new sysfs entries.
Those sysfs entries are not critical to use the device, if parsing
fails, print an informative log message and continue to probe the
device.
Now that these value are available in sysfs, no more need to print
them in the kernel log so remove es58x_get_product_info().
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/usb/etas_es58x/Makefile | 2 +-
drivers/net/can/usb/etas_es58x/es58x_core.c | 49 +---
drivers/net/can/usb/etas_es58x/es58x_core.h | 51 ++++
drivers/net/can/usb/etas_es58x/es58x_sysfs.c | 231 +++++++++++++++++++
4 files changed, 286 insertions(+), 47 deletions(-)
create mode 100644 drivers/net/can/usb/etas_es58x/es58x_sysfs.c
diff --git a/drivers/net/can/usb/etas_es58x/Makefile b/drivers/net/can/usb/etas_es58x/Makefile
index a129b4aa0215..8f37aa3e32d3 100644
--- a/drivers/net/can/usb/etas_es58x/Makefile
+++ b/drivers/net/can/usb/etas_es58x/Makefile
@@ -1,3 +1,3 @@
# SPDX-License-Identifier: GPL-2.0
obj-$(CONFIG_CAN_ETAS_ES58X) += etas_es58x.o
-etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o
+etas_es58x-y = es58x_core.o es581_4.o es58x_fd.o es58x_sysfs.o
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 25f863b4f5f0..c5109117f8e6 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -2119,48 +2119,6 @@ static void es58x_free_netdevs(struct es58x_device *es58x_dev)
}
}
-/**
- * es58x_get_product_info() - Get the product information and print them.
- * @es58x_dev: ES58X device.
- *
- * Do a synchronous call to get the product information.
- *
- * Return: zero on success, errno when any error occurs.
- */
-static int es58x_get_product_info(struct es58x_device *es58x_dev)
-{
- struct usb_device *udev = es58x_dev->udev;
- const int es58x_prod_info_idx = 6;
- /* Empirical tests show a prod_info length of maximum 83,
- * below should be more than enough.
- */
- const size_t prod_info_len = 127;
- char *prod_info;
- int ret;
-
- prod_info = kmalloc(prod_info_len, GFP_KERNEL);
- if (!prod_info)
- return -ENOMEM;
-
- ret = usb_string(udev, es58x_prod_info_idx, prod_info, prod_info_len);
- if (ret < 0) {
- dev_err(es58x_dev->dev,
- "%s: Could not read the product info: %pe\n",
- __func__, ERR_PTR(ret));
- goto out_free;
- }
- if (ret >= prod_info_len - 1) {
- dev_warn(es58x_dev->dev,
- "%s: Buffer is too small, result might be truncated\n",
- __func__);
- }
- dev_info(es58x_dev->dev, "Product info: %s\n", prod_info);
-
- out_free:
- kfree(prod_info);
- return ret < 0 ? ret : 0;
-}
-
/**
* es58x_init_es58x_dev() - Initialize the ES58X device.
* @intf: USB interface.
@@ -2243,10 +2201,6 @@ static int es58x_probe(struct usb_interface *intf,
if (IS_ERR(es58x_dev))
return PTR_ERR(es58x_dev);
- ret = es58x_get_product_info(es58x_dev);
- if (ret)
- return ret;
-
for (ch_idx = 0; ch_idx < es58x_dev->num_can_ch; ch_idx++) {
ret = es58x_init_netdev(es58x_dev, ch_idx);
if (ret) {
@@ -2255,6 +2209,8 @@ static int es58x_probe(struct usb_interface *intf,
}
}
+ es58x_create_file(es58x_dev->dev);
+
return ret;
}
@@ -2272,6 +2228,7 @@ static void es58x_disconnect(struct usb_interface *intf)
dev_info(&intf->dev, "Disconnecting %s %s\n",
es58x_dev->udev->manufacturer, es58x_dev->udev->product);
+ es58x_remove_file(es58x_dev->dev);
es58x_free_netdevs(es58x_dev);
es58x_free_urbs(es58x_dev);
usb_set_intfdata(intf, NULL);
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.h b/drivers/net/can/usb/etas_es58x/es58x_core.h
index 640fe0a1df63..d3756ff07da5 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.h
@@ -356,6 +356,45 @@ 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.
+ *
+ * If @major, @minor and @revision are all zero, the product string
+ * could not be parsed and the version number is invalid.
+ */
+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.
+ *
+ * If @letter, @major and @minor are all zero, the product string
+ * could not be parsed and the hardware revision number is invalid.
+ */
+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 +412,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 +450,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;
@@ -674,6 +720,7 @@ static inline enum es58x_flag es58x_get_flags(const struct sk_buff *skb)
return es58x_flags;
}
+/* Prototypes from es58x_core.c. */
int es58x_can_get_echo_skb(struct net_device *netdev, u32 packet_idx,
u64 *tstamps, unsigned int pkts);
int es58x_tx_ack_msg(struct net_device *netdev, u16 tx_free_entries,
@@ -691,6 +738,10 @@ int es58x_rx_cmd_ret_u32(struct net_device *netdev,
int es58x_send_msg(struct es58x_device *es58x_dev, u8 cmd_type, u8 cmd_id,
const void *msg, u16 cmd_len, int channel_idx);
+/* Prototypes from es58x_sysfs.c. */
+void es58x_create_file(struct device *dev);
+void es58x_remove_file(struct device *dev);
+
extern const struct es58x_parameters es581_4_param;
extern const struct es58x_operators es581_4_ops;
diff --git a/drivers/net/can/usb/etas_es58x/es58x_sysfs.c b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
new file mode 100644
index 000000000000..db1d57b6cda5
--- /dev/null
+++ b/drivers/net/can/usb/etas_es58x/es58x_sysfs.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/* Driver for ETAS GmbH ES58X USB CAN(-FD) Bus Interfaces.
+ *
+ * File es58x_sysfs.c: parse the product information string and
+ * populate sysfs.
+ *
+ * Copyright (c) 2022 Vincent Mailhol <[email protected]>
+ */
+
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/string.h>
+#include <linux/usb.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;
+}
+
+/**
+ * firmware_version_show() - Emit the firmware version in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the firmware version.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t firmware_version_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+ struct es58x_sw_version *fw_ver = &es58x_dev->firmware_version;
+
+ return sysfs_emit(buf, "%02u.%02u.%02u\n",
+ fw_ver->major, fw_ver->minor, fw_ver->revision);
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+/**
+ * bootloader_version_show() - Emit the bootloader version in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the bootloader version.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t bootloader_version_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+ struct es58x_sw_version *bl_ver = &es58x_dev->bootloader_version;
+
+ return sysfs_emit(buf, "%02u.%02u.%02u\n",
+ bl_ver->major, bl_ver->minor, bl_ver->revision);
+}
+static DEVICE_ATTR_RO(bootloader_version);
+
+/**
+ * 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;
+}
+
+/**
+ * hardware_revision_show() - Emit the hardware revision in sysfs.
+ * @dev: Device.
+ * @attr: Device attribute, not used.
+ * @buf: Where to write the hardware revision.
+ *
+ * Return: number of bytes written.
+ */
+static ssize_t hardware_revision_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+ struct es58x_hw_revision *hw_rev = &es58x_dev->hardware_revision;
+
+ return sysfs_emit(buf, "%c%03u/%03u\n",
+ hw_rev->letter, hw_rev->major, hw_rev->minor);
+}
+static DEVICE_ATTR_RO(hardware_revision);
+
+/**
+ * es58x_create_file() - Expose the product information through sysfs.
+ * @dev: Device.
+ *
+ * Retrieve the product information string and parse it to extract the
+ * firmware version, the bootloader version and the hardware
+ * revision. Then create three sysfs entries.
+ *
+ * If the parsing fails, simply emit an informative log message and
+ * continue because these files are not critical for the driver to
+ * operate.
+ */
+void es58x_create_file(struct device *dev)
+{
+ struct es58x_device *es58x_dev = dev_get_drvdata(dev);
+ char *prod_info;
+
+ prod_info = usb_cache_string(es58x_dev->udev, ES58X_PROD_INFO_IDX);
+ if (!prod_info) {
+ dev_warn(dev, "could not retrieve the product info string\n");
+ return;
+ }
+
+ if (!es58x_parse_sw_version(es58x_dev, prod_info, "FW"))
+ device_create_file(dev, &dev_attr_firmware_version);
+ else
+ dev_info(dev, "parsing error: can not create firmware_version sysfs entry\n");
+
+ if (!es58x_parse_sw_version(es58x_dev, prod_info, "BL"))
+ device_create_file(dev, &dev_attr_bootloader_version);
+ else
+ dev_info(dev, "parsing error: can not create bootloader_version sysfs entry\n");
+
+ if (!es58x_parse_hw_rev(es58x_dev, prod_info))
+ device_create_file(dev, &dev_attr_hardware_revision);
+ else
+ dev_info(dev, "parsing error: can not create hardware_revision sysfs entry\n");
+
+ kfree(prod_info);
+}
+
+/**
+ * es58x_remove_file() - Remove sysfs files created by es58x_create_file().
+ * @dev: Device.
+ *
+ * device_remove_file() checks if the sysfs file exists before trying
+ * to remove it so it is safe to call it even if these files were not
+ * created by es58x_create_file() due of a parsing error.
+ */
+void es58x_remove_file(struct device *dev)
+{
+ device_remove_file(dev, &dev_attr_firmware_version);
+ device_remove_file(dev, &dev_attr_bootloader_version);
+ device_remove_file(dev, &dev_attr_hardware_revision);
+}
--
2.37.4
On 13 Nov 13:01, Vincent Mailhol wrote:
>ES58x devices report below information in their usb product info
>string:
>
> * the firmware version
> * the bootloader version
> * the hardware revision
>
>Parse this string, store the results in struct es58x_dev and create
>three new sysfs entries.
>
will this be the /sys/class/net/XXX sysfs ?
We try to avoid adding device specific entries in there,
Couldn't you just squeeze the firmware and hw version into the
ethtool->drvinfo->fw_version
something like:
fw_version: %3u.%3u.%3u (%c.%3u.%3u)
and bootloader into ethtool->drvinfo->erom_version:
* @erom_version: Expansion ROM version string; may be an empty string
On Wed. 16 Nov. 2022 at 07:50, Saeed Mahameed <[email protected]> wrote:
> On 13 Nov 13:01, Vincent Mailhol wrote:
> >ES58x devices report below information in their usb product info
> >string:
> >
> > * the firmware version
> > * the bootloader version
> > * the hardware revision
> >
> >Parse this string, store the results in struct es58x_dev and create
> >three new sysfs entries.
> >
>
> will this be the /sys/class/net/XXX sysfs ?
I am dropping the idea of using sysfs and I am now considering using
devlink following Andrew's message:
https://lore.kernel.org/linux-can/[email protected]/
> We try to avoid adding device specific entries in there,
>
> Couldn't you just squeeze the firmware and hw version into the
> ethtool->drvinfo->fw_version
>
> something like:
> fw_version: %3u.%3u.%3u (%c.%3u.%3u)
This looks like a hack. There is no way for the end user to know, just
from the ethtool output, what these in brackets values would mean.
> and bootloader into ethtool->drvinfo->erom_version:
> * @erom_version: Expansion ROM version string; may be an empty string
Same. I considered doing this in the early draft of this series and
dropped the idea because an expansion ROM and a boot loader are two
things different.
I will continue to study devlink and only use the drvinfo only for the
firmware version.
Yours sincerely,
Vincent Mailhol
On 16 Nov 09:36, Vincent MAILHOL wrote:
>On Wed. 16 Nov. 2022 at 07:50, Saeed Mahameed <[email protected]> wrote:
>> On 13 Nov 13:01, Vincent Mailhol wrote:
>> >ES58x devices report below information in their usb product info
>> >string:
>> >
>> > * the firmware version
>> > * the bootloader version
>> > * the hardware revision
>> >
>> >Parse this string, store the results in struct es58x_dev and create
>> >three new sysfs entries.
>> >
>>
>> will this be the /sys/class/net/XXX sysfs ?
>
>I am dropping the idea of using sysfs and I am now considering using
>devlink following Andrew's message:
>https://lore.kernel.org/linux-can/[email protected]/
>
+1
>> We try to avoid adding device specific entries in there,
>>
>> Couldn't you just squeeze the firmware and hw version into the
>> ethtool->drvinfo->fw_version
>>
>> something like:
>> fw_version: %3u.%3u.%3u (%c.%3u.%3u)
>
>This looks like a hack. There is no way for the end user to know, just
>from the ethtool output, what these in brackets values would mean.
it's not, there is no well defined format for what to put in the version,
as long as it clearly describes what FW is currently running.
at the end of the day, it's just a text you copy&paste when you contact
customer support.
>
>> and bootloader into ethtool->drvinfo->erom_version:
>> * @erom_version: Expansion ROM version string; may be an empty string
>
>Same. I considered doing this in the early draft of this series and
>dropped the idea because an expansion ROM and a boot loader are two
>things different.
>
>I will continue to study devlink and only use the drvinfo only for the
>firmware version.
>
100% devlink is a great options.