| 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
| ||
|
Date: Fri, 27 Aug 2021 21:31:06 +0200
From: Aleksa Savic <savicaleksa83@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: linux-hwmon@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hwmon: add driver for Aquacomputer D5 Next
On Fri, Aug 27, 2021 at 08:47:45AM -0700, Guenter Roeck wrote:
> On Fri, Aug 27, 2021 at 10:25:05AM +0200, Aleksa Savic wrote:
> > This driver exposes hardware sensors of the Aquacomputer D5 Next
> > watercooling pump, which communicates through a proprietary USB HID
> > protocol.
> >
> > Available sensors are pump and fan speed, power, voltage and current, as
> > well as coolant temperature. Also available through debugfs are the serial
> > number, firmware version and power-on count.
> >
> > Attaching a fan is optional and allows it to be controlled using
> > temperature curves directly from the pump. If it's not connected,
> > the fan-related sensors will report zeroes.
> >
> > The pump can be configured either through software or via its physical
> > interface. Configuring the pump through this driver is not implemented,
> > as it seems to require sending it a complete configuration. That
> > includes addressable RGB LEDs, for which there is no standard sysfs
> > interface. Thus, that task is better suited for userspace tools.
> >
> > This driver has been tested on x86_64, both in-kernel and as a module.
> >
> > Signed-off-by: Aleksa Savic <savicaleksa83@...il.com>
> > ---
> > Documentation/hwmon/aquacomputer_d5next.rst | 61 ++++
> > Documentation/hwmon/index.rst | 1 +
> > MAINTAINERS | 7 +
> > drivers/hwmon/Kconfig | 10 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/aquacomputer_d5next.c | 366 ++++++++++++++++++++
> > 6 files changed, 446 insertions(+)
> > create mode 100644 Documentation/hwmon/aquacomputer_d5next.rst
> > create mode 100644 drivers/hwmon/aquacomputer_d5next.c
> >
> > diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
> > new file mode 100644
> > index 000000000000..1f4bb4ba2e4b
> > --- /dev/null
> > +++ b/Documentation/hwmon/aquacomputer_d5next.rst
> > @@ -0,0 +1,61 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver aquacomputer-d5next
> > +=================================
> > +
> > +Supported devices:
> > +
> > +* Aquacomputer D5 Next watercooling pump
> > +
> > +Author: Aleksa Savic
> > +
> > +Description
> > +-----------
> > +
> > +This driver exposes hardware sensors of the Aquacomputer D5 Next watercooling
> > +pump, which communicates through a proprietary USB HID protocol.
> > +
> > +Available sensors are pump and fan speed, power, voltage and current, as
> > +well as coolant temperature. Also available through debugfs are the serial
> > +number, firmware version and power-on count.
> > +
> > +Attaching a fan is optional and allows it to be controlled using temperature
> > +curves directly from the pump. If it's not connected, the fan-related sensors
> > +will report zeroes.
> > +
> > +The pump can be configured either through software or via its physical
> > +interface. Configuring the pump through this driver is not implemented, as it
> > +seems to require sending it a complete configuration. That includes addressable
> > +RGB LEDs, for which there is no standard sysfs interface. Thus, that task is
> > +better suited for userspace tools.
> > +
> > +Usage notes
> > +-----------
> > +
> > +The pump communicates via HID reports. The driver is loaded automatically by
> > +the kernel and supports hotswapping.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +============ =============================================
> > +temp1_input Coolant temperature (in millidegrees Celsius)
> > +fan1_input Pump speed (in RPM)
> > +fan2_input Fan speed (in RPM)
> > +power1_input Pump power (in micro Watts)
> > +power2_input Fan power (in micro Watts)
> > +in0_input Pump voltage (in milli Volts)
> > +in1_input Fan voltage (in milli Volts)
> > +in2_input +5V rail voltage (in milli Volts)
> > +curr1_input Pump current (in milli Amperes)
> > +curr2_input Fan current (in milli Amperes)
> > +============ =============================================
> > +
> > +Debugfs entries
> > +---------------
> > +
> > +================ ===============================================
> > +serial_number Serial number of the pump
> > +firmware_version Version of installed firmware
> > +power_cycles Count of how many times the pump was powered on
> > +================ ===============================================
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index bc01601ea81a..77bfb2e2e8a9 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -39,6 +39,7 @@ Hardware Monitoring Kernel Drivers
> > adt7475
> > aht10
> > amc6821
> > + aquacomputer_d5next
> > asb100
> > asc7621
> > aspeed-pwm-tacho
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d7b4f32875a9..ec0aa0dcf635 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1316,6 +1316,13 @@ L: linux-media@...r.kernel.org
> > S: Maintained
> > F: drivers/media/i2c/aptina-pll.*
> >
> > +AQUACOMPUTER D5 NEXT PUMP SENSOR DRIVER
> > +M: Aleksa Savic <savicaleksa83@...il.com>
> > +L: linux-hwmon@...r.kernel.org
> > +S: Maintained
> > +F: Documentation/hwmon/aquacomputer_d5next.rst
> > +F: drivers/hwmon/aquacomputer_d5next.c
> > +
> > AQUANTIA ETHERNET DRIVER (atlantic)
> > M: Igor Russkikh <irusskikh@...vell.com>
> > L: netdev@...r.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index e3675377bc5d..2bd563850f87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -254,6 +254,16 @@ config SENSORS_AHT10
> > This driver can also be built as a module. If so, the module
> > will be called aht10.
> >
> > +config SENSORS_AQUACOMPUTER_D5NEXT
> > + tristate "Aquacomputer D5 Next watercooling pump"
> > + depends on USB_HID
> > + help
> > + If you say yes here you get support for the Aquacomputer D5 Next
> > + watercooling pump sensors.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called aquacomputer_d5next.
> > +
> > config SENSORS_AS370
> > tristate "Synaptics AS370 SoC hardware monitoring driver"
> > help
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index d712c61c1f5e..790a611a3188 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
> > obj-$(CONFIG_SENSORS_AHT10) += aht10.o
> > obj-$(CONFIG_SENSORS_AMD_ENERGY) += amd_energy.o
> > obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
> > +obj-$(CONFIG_SENSORS_AQUACOMPUTER_D5NEXT) += aquacomputer_d5next.o
> > obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
> > obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
> > obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o
> > diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> > new file mode 100644
> > index 000000000000..0f831b0eb94c
> > --- /dev/null
> > +++ b/drivers/hwmon/aquacomputer_d5next.c
> > @@ -0,0 +1,366 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * hwmon driver for Aquacomputer D5 Next watercooling pump
> > + *
> > + * The D5 Next sends HID reports (with ID 0x01) every second to report sensor values
> > + * (coolant temperature, pump and fan speed, voltage, current and power). It responds to
> > + * Get_Report requests, but returns a dummy value of no use.
> > + *
> > + * Copyright 2021 Aleksa Savic <savicaleksa83@...il.com>
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/hid.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
>
> #include <linux/seq_file.h>
>
> > +
> > +#define DRIVER_NAME "aquacomputer-d5next"
> > +
> > +#define D5NEXT_STATUS_REPORT_ID 0x01
> > +#define D5NEXT_STATUS_UPDATE_INTERVAL 1 /* In seconds */
> > +
> > +/* Register offsets for the D5 Next pump */
> > +
> > +#define D5NEXT_SERIAL_FIRST_PART 3
> > +#define D5NEXT_SERIAL_SECOND_PART 5
> > +#define D5NEXT_FIRMWARE_VERSION 13
>
> Please always use
>
> #define<space>WHAT<tab>value
>
> with aligned values.
>
> > +#define D5NEXT_POWER_CYCLES 24
> > +
> > +#define D5NEXT_COOLANT_TEMP 87
> > +
> > +#define D5NEXT_PUMP_SPEED 116
> > +#define D5NEXT_FAN_SPEED 103
> > +
> > +#define D5NEXT_PUMP_POWER 114
> > +#define D5NEXT_FAN_POWER 101
> > +
> > +#define D5NEXT_PUMP_VOLTAGE 110
> > +#define D5NEXT_FAN_VOLTAGE 97
> > +#define D5NEXT_5V_VOLTAGE 57
> > +
> > +#define D5NEXT_PUMP_CURRENT 112
> > +#define D5NEXT_FAN_CURRENT 99
> > +
> > +/* Labels for provided values */
> > +
> > +#define L_COOLANT_TEMP "Coolant temp"
> > +
> > +#define L_PUMP_SPEED "Pump speed"
> > +#define L_FAN_SPEED "Fan speed"
> > +
> > +#define L_PUMP_POWER "Pump power"
> > +#define L_FAN_POWER "Fan power"
> > +
> > +#define L_PUMP_VOLTAGE "Pump voltage"
> > +#define L_FAN_VOLTAGE "Fan voltage"
> > +#define L_5V_VOLTAGE "+5V voltage"
> > +
> > +#define L_PUMP_CURRENT "Pump current"
> > +#define L_FAN_CURRENT "Fan current"
> > +
> > +static const char *const label_temp[] = {
> > + L_COOLANT_TEMP,
> > +};
> > +
> > +static const char *const label_speeds[] = {
> > + L_PUMP_SPEED,
> > + L_FAN_SPEED,
> > +};
> > +
> > +static const char *const label_power[] = {
> > + L_PUMP_POWER,
> > + L_FAN_POWER,
> > +};
> > +
> > +static const char *const label_voltages[] = {
> > + L_PUMP_VOLTAGE,
> > + L_FAN_VOLTAGE,
> > + L_5V_VOLTAGE,
> > +};
> > +
> > +static const char *const label_current[] = {
> > + L_PUMP_CURRENT,
> > + L_FAN_CURRENT,
> > +};
> > +
> > +struct d5next_data {
> > + struct hid_device *hdev;
> > + struct device *hwmon_dev;
> > + struct dentry *debugfs;
> > + s32 temp_input[1];
>
> This doesn't have to be an array.
>
> > + u16 speed_input[2];
> > + u32 power_input[2];
> > + u16 voltage_input[3];
> > + u16 current_input[2];
> > + u32 serial_number[2];
> > + u16 firmware_version;
> > + u32 power_cycles; /* How many times the device was powered on */
> > + unsigned long updated;
> > +};
> > +
> > +static umode_t d5next_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> > + int channel)
> > +{
> > + return 0444;
> > +}
> > +
> > +static int d5next_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> > + long *val)
> > +{
> > + struct d5next_data *priv = dev_get_drvdata(dev);
> > +
> > + if (time_after(jiffies, priv->updated + D5NEXT_STATUS_UPDATE_INTERVAL * HZ))
> > + return -ENODATA;
>
> This seems a bit strict; it results in ENODATA if a single update
> is missed or if it comes just a little late. I would suggest to relax
> it a bit.
>
> Also, D5NEXT_STATUS_UPDATE_INTERVAL is always used with "* HZ".
> I would suggest to make that part of the define.
>
Thanks for the feedback here and above! I'll send a v2 soon.
Aleksa
> > +
> > + switch (type) {
> > + case hwmon_temp:
> > + *val = priv->temp_input[channel];
> > + break;
> > + case hwmon_fan:
> > + *val = priv->speed_input[channel];
> > + break;
> > + case hwmon_power:
> > + *val = priv->power_input[channel];
> > + break;
> > + case hwmon_in:
> > + *val = priv->voltage_input[channel];
> > + break;
> > + case hwmon_curr:
> > + *val = priv->current_input[channel];
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int d5next_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> > + int channel, const char **str)
> > +{
> > + switch (type) {
> > + case hwmon_temp:
> > + *str = label_temp[channel];
> > + break;
> > + case hwmon_fan:
> > + *str = label_speeds[channel];
> > + break;
> > + case hwmon_power:
> > + *str = label_power[channel];
> > + break;
> > + case hwmon_in:
> > + *str = label_voltages[channel];
> > + break;
> > + case hwmon_curr:
> > + *str = label_current[channel];
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct hwmon_ops d5next_hwmon_ops = {
> > + .is_visible = d5next_is_visible,
> > + .read = d5next_read,
> > + .read_string = d5next_read_string,
> > +};
> > +
> > +static const struct hwmon_channel_info *d5next_info[] = {
> > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> > + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL),
> > + HWMON_CHANNEL_INFO(power, HWMON_P_INPUT | HWMON_P_LABEL, HWMON_P_INPUT | HWMON_P_LABEL),
> > + HWMON_CHANNEL_INFO(in, HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> > + HWMON_I_INPUT | HWMON_I_LABEL),
> > + HWMON_CHANNEL_INFO(curr, HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT | HWMON_C_LABEL),
> > + NULL
> > +};
> > +
> > +static const struct hwmon_chip_info d5next_chip_info = {
> > + .ops = &d5next_hwmon_ops,
> > + .info = d5next_info,
> > +};
> > +
> > +static int d5next_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data, int size)
> > +{
> > + struct d5next_data *priv;
> > +
> > + if (report->id != D5NEXT_STATUS_REPORT_ID)
> > + return 0;
> > +
> > + priv = hid_get_drvdata(hdev);
> > +
> > + /* Info provided with every report */
> > +
> > + priv->serial_number[0] = get_unaligned_be16(data + D5NEXT_SERIAL_FIRST_PART);
> > + priv->serial_number[1] = get_unaligned_be16(data + D5NEXT_SERIAL_SECOND_PART);
> > +
> > + priv->firmware_version = get_unaligned_be16(data + D5NEXT_FIRMWARE_VERSION);
> > + priv->power_cycles = get_unaligned_be32(data + D5NEXT_POWER_CYCLES);
> > +
> > + /* Sensor readings */
> > +
> > + priv->temp_input[0] = get_unaligned_be16(data + D5NEXT_COOLANT_TEMP) * 10;
> > +
> > + priv->speed_input[0] = get_unaligned_be16(data + D5NEXT_PUMP_SPEED);
> > + priv->speed_input[1] = get_unaligned_be16(data + D5NEXT_FAN_SPEED);
> > +
> > + priv->power_input[0] = get_unaligned_be16(data + D5NEXT_PUMP_POWER) * 10000;
> > + priv->power_input[1] = get_unaligned_be16(data + D5NEXT_FAN_POWER) * 10000;
> > +
> > + priv->voltage_input[0] = get_unaligned_be16(data + D5NEXT_PUMP_VOLTAGE) * 10;
> > + priv->voltage_input[1] = get_unaligned_be16(data + D5NEXT_FAN_VOLTAGE) * 10;
> > + priv->voltage_input[2] = get_unaligned_be16(data + D5NEXT_5V_VOLTAGE) * 10;
> > +
> > + priv->current_input[0] = get_unaligned_be16(data + D5NEXT_PUMP_CURRENT);
> > + priv->current_input[1] = get_unaligned_be16(data + D5NEXT_FAN_CURRENT);
> > +
> > + priv->updated = jiffies;
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +static int serial_number_show(struct seq_file *seqf, void *unused)
> > +{
> > + struct d5next_data *priv = seqf->private;
> > +
> > + seq_printf(seqf, "%05u-%05u\n", priv->serial_number[0], priv->serial_number[1]);
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(serial_number);
> > +
> > +static int firmware_version_show(struct seq_file *seqf, void *unused)
> > +{
> > + struct d5next_data *priv = seqf->private;
> > +
> > + seq_printf(seqf, "%u\n", priv->firmware_version);
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(firmware_version);
> > +
> > +static int power_cycles_show(struct seq_file *seqf, void *unused)
> > +{
> > + struct d5next_data *priv = seqf->private;
> > +
> > + seq_printf(seqf, "%u\n", priv->power_cycles);
> > +
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(power_cycles);
> > +
> > +static void d5next_debugfs_init(struct d5next_data *priv)
> > +{
> > + char name[32];
> > +
> > + scnprintf(name, sizeof(name), "%s-%s", DRIVER_NAME, dev_name(&priv->hdev->dev));
> > +
> > + priv->debugfs = debugfs_create_dir(name, NULL);
> > + debugfs_create_file("serial_number", 0444, priv->debugfs, priv, &serial_number_fops);
> > + debugfs_create_file("firmware_version", 0444, priv->debugfs, priv, &firmware_version_fops);
> > + debugfs_create_file("power_cycles", 0444, priv->debugfs, priv, &power_cycles_fops);
> > +}
> > +
> > +#else
> > +
> > +static void d5next_debugfs_init(struct d5next_data *priv)
> > +{
> > +}
> > +
> > +#endif
> > +
> > +static int d5next_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > +{
> > + struct d5next_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);
> > +
> > + priv->updated = jiffies - D5NEXT_STATUS_UPDATE_INTERVAL * HZ;
> > +
> > + ret = hid_parse(hdev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > + if (ret)
> > + return ret;
> > +
> > + ret = hid_hw_open(hdev);
> > + if (ret)
> > + goto fail_and_stop;
> > +
> > + priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "d5next", priv,
> > + &d5next_chip_info, NULL);
> > +
> > + if (IS_ERR(priv->hwmon_dev)) {
> > + ret = PTR_ERR(priv->hwmon_dev);
> > + goto fail_and_close;
> > + }
> > +
> > + d5next_debugfs_init(priv);
> > +
> > + return 0;
> > +
> > +fail_and_close:
> > + hid_hw_close(hdev);
> > +fail_and_stop:
> > + hid_hw_stop(hdev);
> > + return ret;
> > +}
> > +
> > +static void d5next_remove(struct hid_device *hdev)
> > +{
> > + struct d5next_data *priv = hid_get_drvdata(hdev);
> > +
> > + debugfs_remove_recursive(priv->debugfs);
> > + hwmon_device_unregister(priv->hwmon_dev);
> > +
> > + hid_hw_close(hdev);
> > + hid_hw_stop(hdev);
> > +}
> > +
> > +static const struct hid_device_id d5next_table[] = {
> > + { HID_USB_DEVICE(0x0c70, 0xf00e) }, /* Aquacomputer D5 Next */
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, d5next_table);
> > +
> > +static struct hid_driver d5next_driver = {
> > + .name = DRIVER_NAME,
> > + .id_table = d5next_table,
> > + .probe = d5next_probe,
> > + .remove = d5next_remove,
> > + .raw_event = d5next_raw_event,
> > +};
> > +
> > +static int __init d5next_init(void)
> > +{
> > + return hid_register_driver(&d5next_driver);
> > +}
> > +
> > +static void __exit d5next_exit(void)
> > +{
> > + hid_unregister_driver(&d5next_driver);
> > +}
> > +
> > +/* Request to initialize after the HID bus to ensure it's not being loaded before */
> > +
> > +late_initcall(d5next_init);
> > +module_exit(d5next_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Aleksa Savic <savicaleksa83@...il.com>");
> > +MODULE_DESCRIPTION("Hwmon driver for Aquacomputer D5 Next pump");
> > --
> > 2.31.1
> >
Powered by blists - more mailing lists