lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Sun, 12 May 2024 17:10:06 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Marius Zachmann <mail@...iuszachmann.de>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
 linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: (corsair-cpro) Add firmware and bootloader
 information

On 5/12/24 16:12, Marius Zachmann wrote:
> Sorry for resending. The last email had missing subsystem in the subject.
> 

You actually sent it three times unless I lost count.
Anyway, the above comment should be after "---" because it
should not be part of the description.

> This patch adds:
> - Reading the firmware and bootloader version of the device
> - Debugfs entries: firmware_version and bootloader_version
> - Updated documentation
> 

 From Documentation/process/submitting-patches.rst:

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.

With that in mind, something like

Add support for reporting firmware and bootloader version using debugfs.
Update documentation accordingly.

would be a more appropriate. After all, _reading_ the firmware and bootloader
version is implied in making the information available.

> Signed-off-by: Marius Zachmann <mail@...iuszachmann.de>
> ---
>   Documentation/hwmon/corsair-cpro.rst |  8 +++
>   drivers/hwmon/corsair-cpro.c         | 80 ++++++++++++++++++++++++++++
>   2 files changed, 88 insertions(+)
> 
> diff --git a/Documentation/hwmon/corsair-cpro.rst b/Documentation/hwmon/corsair-cpro.rst
> index 751f95476b57..11135d7ec6b9 100644
> --- a/Documentation/hwmon/corsair-cpro.rst
> +++ b/Documentation/hwmon/corsair-cpro.rst
> @@ -39,3 +39,11 @@ fan[1-6]_target		Sets fan speed target rpm.
>   pwm[1-6]		Sets the fan speed. Values from 0-255. Can only be read if pwm
>   			was set directly.
>   ======================= =====================================================================
> +
> +Debugfs entries
> +---------------
> +
> +======================= ===========================
> +firmware_version	Version of the firmware

			Firmware version

> +bootloader_version	Version of the bootloader

			Bootloader version

> +======================= ===========================
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 3e63666a61bd..4be8a98250a9 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -10,11 +10,13 @@
>   
>   #include <linux/bitops.h>
>   #include <linux/completion.h>
> +#include <linux/debugfs.h>
>   #include <linux/hid.h>
>   #include <linux/hwmon.h>
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/seq_file.h>
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   #include <linux/types.h>
> @@ -28,6 +30,8 @@
>   #define LABEL_LENGTH		11
>   #define REQ_TIMEOUT		300
>   
> +#define CTL_GET_FW_VER		0x02	/* returns the firmware version in bytes 1-3 */
> +#define CTL_GET_BL_VER		0x06	/* returns the bootloader version in bytes 1-2 */
>   #define CTL_GET_TMP_CNCT	0x10	/*
>   					 * returns in bytes 1-4 for each temp sensor:
>   					 * 0 not connected
> @@ -78,6 +82,7 @@
>   struct ccp_device {
>   	struct hid_device *hdev;
>   	struct device *hwmon_dev;
> +	struct dentry *debugfs;
>   	/* For reinitializing the completion below */
>   	spinlock_t wait_input_report_lock;
>   	struct completion wait_input_report;
> @@ -88,6 +93,8 @@ struct ccp_device {
>   	DECLARE_BITMAP(temp_cnct, NUM_TEMP_SENSORS);
>   	DECLARE_BITMAP(fan_cnct, NUM_FANS);
>   	char fan_label[6][LABEL_LENGTH];
> +	u8 firmware_ver[3];
> +	u8 bootloader_ver[2];
>   };
>   
>   /* converts response error in buffer to errno */
> @@ -496,6 +503,71 @@ static int get_temp_cnct(struct ccp_device *ccp)
>   	return 0;
>   }
>   
> +/* read firmware and bootloader version */
> +static int get_fw_version(struct ccp_device *ccp)
> +{
> +	int ret;
> +
> +	ret = send_usb_cmd(ccp, CTL_GET_FW_VER, 0, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	ccp->firmware_ver[0] = ccp->buffer[1];
> +	ccp->firmware_ver[1] = ccp->buffer[2];
> +	ccp->firmware_ver[2] = ccp->buffer[3];
> +
> +	ret = send_usb_cmd(ccp, CTL_GET_BL_VER, 0, 0, 0);
> +	if (ret)
> +		return ret;
> +
> +	ccp->bootloader_ver[0] = ccp->buffer[1];
> +	ccp->bootloader_ver[1] = ccp->buffer[2];
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_DEBUG_FS
> +

The conditional isn't needed. Yes, that may mean that the code will
be added even if CONFIG_DEBUG_FS is disabled, but that is 1) unlikely
and 2) better than not compiling the code at all and missing compile
failures.

> +static int firmware_show(struct seq_file *seqf, void *unused)
> +{
> +	struct ccp_device *ccp = seqf->private;
> +
> +	seq_printf(seqf, "%d.%d.%d\n",
> +		   ccp->firmware_ver[0],
> +		   ccp->firmware_ver[1],
> +		   ccp->firmware_ver[2]);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(firmware);
> +
> +static int bootloader_show(struct seq_file *seqf, void *unused)
> +{
> +	struct ccp_device *ccp = seqf->private;
> +
> +	seq_printf(seqf, "%d.%d\n",
> +		   ccp->bootloader_ver[0],
> +		   ccp->bootloader_ver[1]);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(bootloader);
> +
> +static void ccp_debugfs_init(struct ccp_device *ccp)
> +{
> +	ccp->debugfs = debugfs_create_dir("corsaircpro", NULL);
> +	debugfs_create_file("firmware_version", 0444, ccp->debugfs, ccp, &firmware_fops);
> +	debugfs_create_file("bootloader_version", 0444, ccp->debugfs, ccp, &bootloader_fops);
> +}
> +
> +#else
> +
> +static void ccp_debugfs_init(struct ccp_device *ccp)
> +{
> +}
> +
> +#endif
> +
>   static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   {
>   	struct ccp_device *ccp;
> @@ -542,6 +614,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   	ret = get_fan_cnct(ccp);
>   	if (ret)
>   		goto out_hw_close;
> +
> +	ret = get_fw_version(ccp);
> +	if (ret)
> +		goto out_hw_close;
> +

Why bail out ? That is only informational, and if the information isn't available
the driver would still be operational. On top of that, if debugfs isn't enabled,
the information isn't even used. That means the probe function would bail out
for no good reason.

I'd suggest to move the call to get_fw_version() into ccp_debugfs_init()
and let it fail silently. I would not mind if you add a dev_notice() message
if the call fails, but anything else seems excessive.

Thanks,
Guenter


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ