| 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
| ||
|
Message-ID: <121470f0-6c1f-418a-844c-7ec2e8a54b8e@gmail.com>
Date: Sun, 3 Dec 2023 16:00:16 +0100
From: Aleksa Savic <savicaleksa83@...il.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
linux-hwmon@...r.kernel.org
Cc: savicaleksa83@...il.com, Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>,
Jonathan Corbet <corbet@....net>,
"open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] hwmon: Add driver for Gigabyte AORUS Waterforce AIO
coolers
On 2023-12-03 15:14:05 GMT+01:00, Christophe JAILLET wrote:
> Le 03/12/2023 à 13:06, Aleksa Savic a écrit :
>> This driver exposes hardware sensors of the Gigabyte AORUS Waterforce
>> all-in-one CPU liquid coolers, which communicate through a proprietary
>> USB HID protocol. Report offsets were initially discovered in [1] and
>> confirmed by me on a Waterforce X240 by observing the sent reports from
>> the official software.
>>
>> Available sensors are pump and fan speed in RPM, as well as coolant
>> temperature. Also available through debugfs is the firmware version.
>>
>> Attaching a fan is optional and allows it to be controlled from the
>> device. If it's not connected, the fan-related sensors will report
>> zeroes.
>>
>> The addressable RGB LEDs and LCD screen are not supported in this
>> driver and should be controlled through userspace tools.
>>
>> [1]: https://github.com/liquidctl/liquidctl/issues/167
>>
>> Signed-off-by: Aleksa Savic <savicaleksa83@...il.com>
>> ---
>
> Hi,
> ...
Hi Christophe!
>
>> +/* Writes the command to the device with the rest of the report filled with zeroes */
>> +static int waterforce_write_expanded(struct waterforce_data *priv, const u8 *cmd, int cmd_length)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&priv->buffer_lock);
>> +
>> + memset(priv->buffer, 0x00, MAX_REPORT_LENGTH);
>> + memcpy(priv->buffer, cmd, cmd_length);
>
> Is memcpy_and_pad() useful here?
Looks like it is, thanks. Didn't know about that one.
>
>> + ret = hid_hw_output_report(priv->hdev, priv->buffer, MAX_REPORT_LENGTH);
>> +
>> + mutex_unlock(&priv->buffer_lock);
>> + return ret;
>> +}
>
> ...
>
>> +static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
>> + u32 attr, int channel, long *val)
>> +{
>> + int ret;
>> + struct waterforce_data *priv = dev_get_drvdata(dev);
>> +
>> + if (time_after(jiffies, priv->updated + msecs_to_jiffies(STATUS_VALIDITY))) {
>> + /* Request status on demand */
>> + ret = waterforce_get_status(priv);
>> + if (ret < 0)
>> + return -ENODATA;
>> + }
>> +
>> + switch (type) {
>> + case hwmon_temp:
>> + *val = priv->temp_input[channel];
>> + break;
>> + case hwmon_fan:
>> + *val = priv->speed_input[channel];
>> + break;
>> + case hwmon_pwm:
>> + switch (attr) {
>> + case hwmon_pwm_input:
>> + *val = DIV_ROUND_CLOSEST(priv->duty_input[channel] * 255, 100);
>> + break;
>> + default:
>> + break;
>
> Should we return an error here?
Only hwmon_pwm_input is made visible in waterforce_is_visible(),
so I'm not sure if anything else can get passed in? (Like the default case
below.) I'll add an error return here, just wondering.
>
>> + }
>> + break;
>> + default:
>> + return -EOPNOTSUPP; /* unreachable */
>> + }
>> +
>> + return 0;
>> +}
>
> ...
>
>> +static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> +{
>> + struct waterforce_data *priv;
>> + int ret;
>> +
>> + priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>> +
>> + priv->hdev = hdev;
>> + hid_set_drvdata(hdev, priv);
>> +
>> + /*
>> + * Initialize priv->updated to STATUS_VALIDITY seconds in the past, making
>> + * the initial empty data invalid for waterforce_read() without the need for
>> + * a special case there.
>> + */
>> + priv->updated = jiffies - msecs_to_jiffies(STATUS_VALIDITY);
>> +
>> + ret = hid_parse(hdev);
>> + if (ret) {
>> + hid_err(hdev, "hid parse failed with %d\n", ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * Enable hidraw so existing user-space tools can continue to work.
>> + */
>> + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> + if (ret) {
>> + hid_err(hdev, "hid hw start failed with %d\n", ret);
>> + goto fail_and_stop;
>
> Should this be 'return ret;' (the _start has failed, so why stop?)
Hm, yes.
>
>> + }
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "hid hw open failed with %d\n", ret);
>> + goto fail_and_close;
>
> Should this be 'fail_and_stop' (the _open has failed, so why close?)
Also yes... I based this part on the nzxt-kraken2 driver in the tree,
perhaps that should be investigated as well. The aquacomputer_d5next driver
seems to be doing it correctly.
>
>> + }
>> +
>> + priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
>> + if (!priv->buffer) {
>> + ret = -ENOMEM;
>> + goto fail_and_close;
>> + }
>> +
>> + mutex_init(&priv->status_report_request_mutex);
>> + mutex_init(&priv->buffer_lock);
>> + spin_lock_init(&priv->status_report_request_lock);
>> + init_completion(&priv->status_report_received);
>> + init_completion(&priv->fw_version_processed);
>> +
>> + priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "waterforce",
>> + priv, &waterforce_chip_info, NULL);
>> + if (IS_ERR(priv->hwmon_dev)) {
>> + ret = PTR_ERR(priv->hwmon_dev);
>> + hid_err(hdev, "hwmon registration failed with %d\n", ret);
>> + goto fail_and_close;
>> + }
>> +
>> + hid_device_io_start(hdev);
>> + ret = waterforce_get_fw_ver(hdev);
>> + if (ret < 0)
>> + hid_warn(hdev, "fw version request failed with %d\n", ret);
>> + hid_device_io_stop(hdev);
>> +
>> + waterforce_debugfs_init(priv);
>> +
>> + return 0;
>> +
>> +fail_and_close:
>> + hid_hw_close(hdev);
>> +fail_and_stop:
>> + hid_hw_stop(hdev);
>> + return ret;
>> +}
>> +
>> +static void waterforce_remove(struct hid_device *hdev)
>> +{
>> + struct waterforce_data *priv = hid_get_drvdata(hdev);
>> +
>> + hwmon_device_unregister(priv->hwmon_dev);
>
> Should debugfs_remove_recursive(() be called?
> (if CONFIG_DEBUG_FS is defined)
It should. Sorry, missed it.
Thanks a lot,
Aleksa
>
> CJ
>
>> +
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> +}
>
> ...
>
Powered by blists - more mailing lists