| 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: Sat, 11 Feb 2023 10:08:27 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Leonard Anderweit <leonard.anderweit@...il.com>,
linux-hwmon@...r.kernel.org
Cc: Aleksa Savic <savicaleksa83@...il.com>,
Jack Doan <me@...kdoan.com>, Jean Delvare <jdelvare@...e.com>,
Jonathan Corbet <corbet@....net>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] hwmon: (aquacomputer_d5next) Add temperature offset
control for Aquaero
On 2/11/23 08:59, Leonard Anderweit wrote:
> Support sending control reports to Aquacomputer Aquaero. This commit only
> enables control of the temperature offset as this works in the same manner as
> with already supported devices.
> Also, mention temperature offset control for Aquaero in docs.
>
> Signed-off-by: Leonard Anderweit <leonard.anderweit@...il.com>
> ---
> Documentation/hwmon/aquacomputer_d5next.rst | 4 +-
> drivers/hwmon/aquacomputer_d5next.c | 62 ++++++++++++++++-----
> 2 files changed, 50 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/hwmon/aquacomputer_d5next.rst b/Documentation/hwmon/aquacomputer_d5next.rst
> index 7d0d015b1a52..de43374940b8 100644
> --- a/Documentation/hwmon/aquacomputer_d5next.rst
> +++ b/Documentation/hwmon/aquacomputer_d5next.rst
> @@ -25,7 +25,7 @@ communicate through proprietary USB HID protocols.
>
> The Aquaero devices expose eight physical, eight virtual and four calculated
> virtual temperature sensors, as well as two flow sensors. The fans expose their
> -speed (in RPM), power, voltage and current.
> +speed (in RPM), power, voltage and current. The temperature offset can be controlled.
>
> For the D5 Next pump, available sensors are pump and fan speed, power, voltage
> and current, as well as coolant temperature and eight virtual temp sensors. Also
> @@ -75,7 +75,7 @@ Sysfs entries
>
> ================ ==============================================================
> temp[1-20]_input Physical/virtual temperature sensors (in millidegrees Celsius)
> -temp[1-4]_offset Temperature sensor correction offset (in millidegrees Celsius)
> +temp[1-8]_offset Temperature sensor correction offset (in millidegrees Celsius)
> fan[1-8]_input Pump/fan speed (in RPM) / Flow speed (in dL/h)
> fan5_pulses Quadro flow sensor pulses
> power[1-8]_input Pump/fan power (in micro Watts)
> diff --git a/drivers/hwmon/aquacomputer_d5next.c b/drivers/hwmon/aquacomputer_d5next.c
> index 03ef9e0258d2..95461e2907e1 100644
> --- a/drivers/hwmon/aquacomputer_d5next.c
> +++ b/drivers/hwmon/aquacomputer_d5next.c
> @@ -56,6 +56,7 @@ static const char *const aqc_device_names[] = {
> #define SERIAL_PART_OFFSET 2
>
> #define CTRL_REPORT_ID 0x03
> +#define AQUAERO_CTRL_REPORT_ID 0x0b
>
> /* The HID report that the official software always sends
> * after writing values, currently same for all devices
> @@ -67,6 +68,14 @@ static u8 secondary_ctrl_report[] = {
> 0x02, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x34, 0xC6
> };
>
> +/* Secondary HID report values for Aquaero */
> +#define AQUAERO_SECONDARY_CTRL_REPORT_ID 0x06
> +#define AQUAERO_SECONDARY_CTRL_REPORT_SIZE 0x07
> +
> +static u8 aquaero_secondary_ctrl_report[] = {
> + 0x06, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00
> +};
> +
> /* Report IDs for legacy devices */
> #define POWERADJUST3_STATUS_REPORT_ID 0x03
>
> @@ -94,6 +103,7 @@ static u8 secondary_ctrl_report[] = {
> #define AQUAERO_NUM_VIRTUAL_SENSORS 8
> #define AQUAERO_NUM_CALC_VIRTUAL_SENSORS 4
> #define AQUAERO_NUM_FLOW_SENSORS 2
> +#define AQUAERO_CTRL_REPORT_SIZE 0xa93
>
> /* Sensor report offsets for Aquaero fan controllers */
> #define AQUAERO_SENSOR_START 0x65
> @@ -106,6 +116,9 @@ static u8 secondary_ctrl_report[] = {
> #define AQUAERO_FAN_SPEED_OFFSET 0x00
> static u16 aquaero_sensor_fan_offsets[] = { 0x167, 0x173, 0x17f, 0x18B };
>
> +/* Control report offsets for the Aquaero fan controllers */
> +#define AQUAERO_TEMP_CTRL_OFFSET 0xdb
> +
> /* Specs of the D5 Next pump */
> #define D5NEXT_NUM_FANS 2
> #define D5NEXT_NUM_SENSORS 1
> @@ -441,6 +454,10 @@ struct aqc_data {
> const char *name;
>
> int status_report_id; /* Used for legacy devices, report is stored in buffer */
> + int ctrl_report_id;
> + int secondary_ctrl_report_id;
> + int secondary_ctrl_report_size;
> + u8 *secondary_ctrl_report;
>
> int buffer_size;
> u8 *buffer;
> @@ -513,7 +530,7 @@ static int aqc_get_ctrl_data(struct aqc_data *priv)
> int ret;
>
> memset(priv->buffer, 0x00, priv->buffer_size);
> - ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> + ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
> HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
> if (ret < 0)
> ret = -ENODATA;
> @@ -527,23 +544,27 @@ static int aqc_send_ctrl_data(struct aqc_data *priv)
> int ret;
> u16 checksum;
>
> - /* Init and xorout value for CRC-16/USB is 0xffff */
> - checksum = crc16(0xffff, priv->buffer + priv->checksum_start, priv->checksum_length);
> - checksum ^= 0xffff;
> + /* Checksum is not needed for Aquaero */
> + if (priv->kind != aquaero) {
> + /* Init and xorout value for CRC-16/USB is 0xffff */
> + checksum = crc16(0xffff, priv->buffer + priv->checksum_start,
> + priv->checksum_length);
> + checksum ^= 0xffff;
aquaero is already supported, and the checksum is so far generated
and sent. Is it ignored ? Also, is it guaranteed that _all_ aquero devices
don't need it ?
If it is not needed and ignored, does it really add value to selectively drop it ?
Either case, this change is not mentioned in the commit log, and it
violates the "one logical change per patch" rule. Please split it into
a separate patch and explain why the change is needed.
Another change to separate is the introduction of ctrl_report_id
and the secondary_ctrl_report variables, which is also done silently
and not explained. That should also be a separate patch to simplify
review.
Thanks,
Guenter
>
> - /* Place the new checksum at the end of the report */
> - put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> + /* Place the new checksum at the end of the report */
> + put_unaligned_be16(checksum, priv->buffer + priv->checksum_offset);
> + }
>
> /* Send the patched up report back to the device */
> - ret = hid_hw_raw_request(priv->hdev, CTRL_REPORT_ID, priv->buffer, priv->buffer_size,
> + ret = hid_hw_raw_request(priv->hdev, priv->ctrl_report_id, priv->buffer, priv->buffer_size,
> HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> if (ret < 0)
> return ret;
>
> /* The official software sends this report after every change, so do it here as well */
> - ret = hid_hw_raw_request(priv->hdev, SECONDARY_CTRL_REPORT_ID, secondary_ctrl_report,
> - SECONDARY_CTRL_REPORT_SIZE, HID_FEATURE_REPORT,
> - HID_REQ_SET_REPORT);
> + ret = hid_hw_raw_request(priv->hdev, priv->secondary_ctrl_report_id,
> + priv->secondary_ctrl_report, priv->secondary_ctrl_report_size,
> + HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> return ret;
> }
>
> @@ -969,10 +990,10 @@ static const struct hwmon_channel_info *aqc_info[] = {
> HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> - HWMON_T_INPUT | HWMON_T_LABEL,
> - HWMON_T_INPUT | HWMON_T_LABEL,
> - HWMON_T_INPUT | HWMON_T_LABEL,
> - HWMON_T_INPUT | HWMON_T_LABEL,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> + HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_OFFSET,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> HWMON_T_INPUT | HWMON_T_LABEL,
> @@ -1275,6 +1296,9 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
> priv->num_flow_sensors = AQUAERO_NUM_FLOW_SENSORS;
> priv->flow_sensors_start_offset = AQUAERO_FLOW_SENSORS_START;
>
> + priv->buffer_size = AQUAERO_CTRL_REPORT_SIZE;
> + priv->temp_ctrl_offset = AQUAERO_TEMP_CTRL_OFFSET;
> +
> priv->temp_label = label_temp_sensors;
> priv->virtual_temp_label = label_virtual_temp_sensors;
> priv->calc_virt_temp_label = label_aquaero_calc_temp_sensors;
> @@ -1438,6 +1462,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
> priv->firmware_version_offset = AQUAERO_FIRMWARE_VERSION;
>
> priv->fan_structure = &aqc_aquaero_fan_structure;
> +
> + priv->ctrl_report_id = AQUAERO_CTRL_REPORT_ID;
> + priv->secondary_ctrl_report_id = AQUAERO_SECONDARY_CTRL_REPORT_ID;
> + priv->secondary_ctrl_report_size = AQUAERO_SECONDARY_CTRL_REPORT_SIZE;
> + priv->secondary_ctrl_report = aquaero_secondary_ctrl_report;
> break;
> case poweradjust3:
> priv->status_report_id = POWERADJUST3_STATUS_REPORT_ID;
> @@ -1446,6 +1475,11 @@ static int aqc_probe(struct hid_device *hdev, const struct hid_device_id *id)
> priv->serial_number_start_offset = AQC_SERIAL_START;
> priv->firmware_version_offset = AQC_FIRMWARE_VERSION;
>
> + priv->ctrl_report_id = CTRL_REPORT_ID;
> + priv->secondary_ctrl_report_id = SECONDARY_CTRL_REPORT_ID;
> + priv->secondary_ctrl_report_size = SECONDARY_CTRL_REPORT_SIZE;
> + priv->secondary_ctrl_report = secondary_ctrl_report;
> +
> if (priv->kind == aquastreamult)
> priv->fan_structure = &aqc_aquastreamult_fan_structure;
> else
Powered by blists - more mailing lists