Hello,
I have a couple suggestions regarding the code.
> [...]
> +/*
> + * Supports following liquid coolers:
> + * H100i platinum
> + *
> + * Other products should work with this driver but no testing has been done.
> + *
> + * Note: platinum is the codename name for pro within the driver, so H100i platinum = H100i pro.
> + * But some products are actually calles platinum, these are not intended to be supported.
I think you mean "called", no?
> + *
> + * Note: fan curve control has not been implemented
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +struct device_config {
> + u16 vendor_id;
> + u16 product_id;
> + u8 fancount;
I think you could be more consistent. Later in the code you use 'uin8_t'.
I'd choose one type of fixed-width integer (either {u,s}N or [u]intN_t),
and stick with it.
> + const struct hwmon_channel_info **hwmon_info;
> +};
> +
> +struct hydro_i_pro_device {
> + struct usb_device *udev;
> +
> + const struct device_config *config;
> +
> + unsigned char *bulk_out_buffer;
> + char *bulk_in_buffer;
Any reason why these two have different sizes? You cast both to
'unsigned char*' in set_fan_target_rpm(), set_fan_pwm_curve(), etc.
As far as I see every device has two buffers for bulk in/out operations.
Isn't it possible that - when multiple processes - try to interact with
the sysfs attributes, more than one USB transactions use the same IO
buffers? I suspect that could lead to data corruption.
> + size_t bulk_out_size;
> + size_t bulk_in_size;
> + char bulk_in_endpointAddr;
> + char bulk_out_endpointAddr;
> +
> + struct usb_interface *interface; /* the interface for this device */
> + struct semaphore
> + limit_sem; /* limiting the number of writes in progress */
I think you may need a mutex here for reasons outlined in my previous comment.
> +};
> +
> +struct hwmon_data {
> + struct hydro_i_pro_device *hdev;
> + int channel_count;
> + void **channel_data;
Why is this 'void**'? As far as I see this could be 'struct hwmon_fan_data**'.
But personally I'd use the "flexible array member" feature of C to deal with this,
or even just a static array that's just big enough.
> +};
> +
> +struct curve_point {
> + uint8_t temp;
> + uint8_t pwm;
> +};
> +
> +struct hwmon_fan_data {
> + char fan_channel;
> + long fan_target;
> + unsigned char fan_pwm_target;
This is very nitpicky, but any reason why is it not 'uint8_t' like above?
This applies to other variables as well, I think some variables are too big for what they store,
or have a sign, when they could be unsigned.
> + long mode;
If the only valid modes are 0, 1, and 2, why the 'long'?
> + struct curve_point curve[7];
> +};
> [...]
> +#define SUCCES_LENGTH 3
> +#define SUCCES_CODE 0x12, 0x34
> +
> +static bool check_succes(enum opcodes command, char ret[SUCCES_LENGTH])
> +{
> + char success[SUCCES_LENGTH] = { command, SUCCES_CODE };
> + return strncmp(ret, success, SUCCES_LENGTH) == 0;
Any reason why it is not memcmp()?
> +}
> +
> +static const struct device_config *get_device_configuration(u16 vendor_id,
> + u16 product_id)
> +{
> + const struct device_config *config;
> + int i = 0;
> + int n = ARRAY_SIZE(config_table);
> + for (i = 0; i < n; i++) {
> + config = &config_table[i];
> + if (config->vendor_id == vendor_id &&
> + config->product_id == product_id) {
> + return config;
> + }
> + }
> + return config;
> +}
> +
I think this can be gotten rid of by utilizing the "driver_info" field of the
usb_device_id struct as seen in drivers/usb/serial/visor.c. That way you could
store a pointer or an index, and there would be no need to do any lookup. Like this:
{ USB_DEVICE(VID, PID), .driver_info = (kernel_ulong_t) &config_table[0] }
> +static int set_fan_pwm_curve(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data,
> + struct curve_point point[7])
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + memcpy(fan_data->curve, point, sizeof(fan_data->curve));
> +
> + send_buf[0] = PWM_FAN_CURVE_CMD;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[2] = point[0].temp;
> + send_buf[3] = point[1].temp;
> + send_buf[4] = point[2].temp;
> + send_buf[5] = point[3].temp;
> + send_buf[6] = point[4].temp;
> + send_buf[7] = point[5].temp;
> + send_buf[8] = point[6].temp;
> + send_buf[9] = point[0].pwm;
> + send_buf[10] = point[1].pwm;
> + send_buf[11] = point[2].pwm;
> + send_buf[12] = point[3].pwm;
> + send_buf[13] = point[4].pwm;
> + send_buf[14] = point[5].pwm;
> + send_buf[15] = point[6].pwm;
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 16, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 4, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_info(&hdev->udev->dev,
> + "[*] failed setting fan curve %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int set_fan_target_rpm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, long val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> +
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + fan_data->fan_target = val;
> + fan_data->fan_pwm_target = 0;
> +
> + send_buf[0] = RPM_FAN_TARGET_CMD;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[2] = (fan_data->fan_target >> 8);
> + send_buf[3] = fan_data->fan_target;
As far as I see 'fan_data->fan_target' is an unsigned 16 bit number, so anything that is
out of range should be rejected - preferably in hwmon_write() - with -EINVAL in my opinion,
since 'fan_data->fan_target' may not accurately represent the state of the hardware
if other values are allowed.
The lack of range checking applies to other parts of the code as well.
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_info(&hdev->udev->dev,
> + "[*] failed setting fan rpm %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
> +static int get_fan_current_rpm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, long *val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> +
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + send_buf[0] = PWM_GET_CURRENT_CMD;
> + send_buf[1] = fan_data->fan_channel;
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 2, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf) ||
> + recv_buf[3] != fan_data->fan_channel) {
> + dev_info(&hdev->udev->dev,
> + "[*] failed retrieving fan rmp %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return -EINVAL;
> + }
> +
> + *val = ((recv_buf[4]) << 8) + recv_buf[5];
> + return 0;
> +}
> +
> +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev,
> + struct hwmon_fan_data *fan_data, long val)
> +{
> + int retval;
> + int wrote;
> + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr);
> + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr);
> +
> + unsigned char *send_buf = hdev->bulk_out_buffer;
> + unsigned char *recv_buf = hdev->bulk_in_buffer;
> +
> + fan_data->fan_pwm_target = val;
> + fan_data->fan_target = 0;
> +
> + send_buf[0] = PWM_FAN_TARGET_CMD;
> + send_buf[1] = fan_data->fan_channel;
> + send_buf[3] = fan_data->fan_pwm_target;
> +
> + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, 4, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, 6, &wrote,
> + BULK_TIMEOUT);
> + if (retval)
> + return retval;
> +
> + if (!check_succes(send_buf[0], recv_buf)) {
> + dev_info(&hdev->udev->dev,
> + "[*] failed setting fan pwm %d,%d,%d/%d\n",
> + recv_buf[0], recv_buf[1], recv_buf[2], recv_buf[3]);
> + return -EINVAL;
> + }
> + return 0;
> +}
> +
The previous four functions are structurally more or less the same,
I think the USB related parts could be placed into their own dedicated function.
> [...]
> +static int hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + struct hwmon_data *data = dev_get_drvdata(dev);
> + struct hydro_i_pro_device *hdev = data->hdev;
> + struct hwmon_fan_data *fan_data;
> + int retval = 0;
> +
> + if (channel >= data->channel_count)
> + return -ECHRNG;
> +
I don't think it's possible that this function is called with an invalid 'channel' value.
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_target:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + retval = usb_autopm_get_interface(hdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&hdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> +
> + retval = set_fan_target_rpm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + fan_data = data->channel_data[channel];
> + if (fan_data->mode != 1)
> + return -EINVAL;
> +
> + retval = usb_autopm_get_interface(hdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&hdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> +
> + retval = set_fan_target_pwm(hdev, fan_data, val);
> + if (retval)
> + goto cleanup;
> +
> + break;
> + case hwmon_pwm_enable:
> + fan_data = data->channel_data[channel];
> +
> + retval = usb_autopm_get_interface(hdev->interface);
> + if (retval)
> + goto exit;
> +
> + if (down_trylock(&hdev->limit_sem)) {
> + retval = -EAGAIN;
> + goto cleanup_interface;
> + }
> + fan_data->mode = val;
> +
The mode is stored even if it is out of range, or if the actual "setting" of the mode
fails, hence it is possible that the value does not reflect the state of the hardware
accurately.
> + switch (val) {
> + case 0:
> + set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + break;
> + case 1:
> + if (fan_data->fan_target != 0) {
> + retval = set_fan_target_rpm(
> + hdev, fan_data,
> + fan_data->fan_target);
> + if (retval)
> + goto cleanup;
> + } else if (fan_data->fan_pwm_target != 0) {
> + retval = set_fan_target_pwm(
> + hdev, fan_data,
> + fan_data->fan_pwm_target);
> + if (retval)
> + goto cleanup;
> + }
> + break;
> + case 2:
> + set_fan_pwm_curve(hdev, fan_data,
> + default_curve);
> + break;
Shouldn't this return -EINVAL in the default branch of 'switch(val)'?
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + goto exit;
> + default:
> + return -EINVAL;
> + }
> +
> +cleanup:
> + up(&hdev->limit_sem);
> +cleanup_interface:
> + usb_autopm_put_interface(hdev->interface);
> +exit:
> + return retval;
> +}
> [...]
> +static void hwmon_init(struct hydro_i_pro_device *hdev)
> +{
> + int fan_id;
> + struct device *hwmon_dev;
> + struct hwmon_fan_data *fan;
> + struct hwmon_data *data = devm_kzalloc(
> + &hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
> + struct hwmon_chip_info *hwmon_info = devm_kzalloc(
> + &hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
Sometimes you use 'sizeof(type)', sometimes 'sizeof(*variable)', please be consistent.
> +
> + data->channel_count = hdev->config->fancount;
> + data->channel_data =
> + devm_kzalloc(&hdev->udev->dev,
> + sizeof(char *) * data->channel_count, GFP_KERNEL);
This should be 'sizeof(fan)' or 'sizeof(struct hwmon_fan_data*)', no?
> +
> + /* For each fan create a data channel a fan config entry and a pwm config entry */
> + for (fan_id = 0; fan_id <= data->channel_count; fan_id++) {
> + fan = devm_kzalloc(&hdev->udev->dev,
> + sizeof(struct hwmon_fan_data), GFP_KERNEL);
> + fan->fan_channel = fan_id;
> + fan->mode = 2;
Can't it be queried what the mode actually is?
> + data->channel_data[fan_id] = fan;
> + }
The loop overflows 'data->channel_data' by one element.
> +
> + hwmon_info->ops = &i_pro_ops;
> + hwmon_info->info = hdev->config->hwmon_info;
> +
> + data->hdev = hdev;
> + hwmon_dev = devm_hwmon_device_register_with_info(
> + &hdev->udev->dev, "driver_fan", data, hwmon_info, NULL);
Personally, I'd choose a different name, since "driver_fan" is not really
descriptive.
> + dev_info(&hdev->udev->dev, "[*] Setup hwmon\n");
> +}
> +
> +/*
> + * Devices that work with this driver.
> + * More devices should work, however none have been tested.
> + */
> +static const struct usb_device_id astk_table[] = {
> + { USB_DEVICE(0x1b1c, 0x0c15) },
Personally, I'd create constant, something like "USB_VENDOR_ID_CORSAIR" and use that here.
Possibly the same for the product id.
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(usb, astk_table);
> +
> +static int init_device(struct usb_device *udev)
> +{
> + int retval;
> +
> + /*
> + * This is needed because when running windows in a vm with proprietary driver
> + *and you switch to this driver, the device will not respond unless you run this.
^
Very nitpicky, but a space is missing there.
> + */
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
> + 0xffff, 0x0000, 0, 0, 0);
> + /*this always returns error*/
> + if (retval)
> + ;
> +
> + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0002, 0x0000, 0, 0, 0);
> + return retval;
> +}
> +
> +static int deinit_device(struct usb_device *udev)
> +{
> + return usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
> + 0x0004, 0x0000, 0, 0, 0);
> +}
> +
> +static void astk_delete(struct hydro_i_pro_device *hdev)
> +{
> + usb_put_intf(hdev->interface);
> + usb_put_dev(hdev->udev);
> + kfree(hdev->bulk_in_buffer);
> + kfree(hdev->bulk_out_buffer);
> + kfree(hdev);
> +}
> +
> +static int astk_probe(struct usb_interface *interface,
> + const struct usb_device_id *id)
> +{
> + struct hydro_i_pro_device *hdev;
> + int retval;
> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> +
> + hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
> + if (!hdev) {
> + retval = -ENOMEM;
> + goto exit;
> + }
> +
> + hdev->config = get_device_configuration(id->idVendor, id->idProduct);
> + if (hdev->config == NULL) {
> + retval = -ENOMEM;
> + goto exit;
> + }
> +
> + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in,
> + &bulk_out, NULL, NULL);
> + if (retval)
> + goto exit;
If this 'if' is taken, then 'hdev' is not freed.
> +
> + hdev->udev = usb_get_dev(interface_to_usbdev(interface));
> + hdev->interface = usb_get_intf(interface);
> +
> + /*
> + * set up the endpoint information
> + * use only the first bulk-in and bulk-out endpoints
> + */
> + hdev->bulk_in_size = usb_endpoint_maxp(bulk_in);
> + hdev->bulk_in_buffer = kmalloc(hdev->bulk_in_size, GFP_KERNEL);
> + hdev->bulk_in_endpointAddr = bulk_in->bEndpointAddress;
> + hdev->bulk_out_size = usb_endpoint_maxp(bulk_out);
> + hdev->bulk_out_buffer = kmalloc(hdev->bulk_out_size, GFP_KERNEL);
> + hdev->bulk_out_endpointAddr = bulk_out->bEndpointAddress;
> +
> + retval = init_device(hdev->udev);
> + if (retval) {
> + dev_err(&interface->dev, "failed initialising this device.\n");
> + goto exit;
Same here, 'hdev' is not freed.
> + }
> +
> + hwmon_init(hdev);
> +
> + usb_set_intfdata(interface, hdev);
> + sema_init(&hdev->limit_sem, 8);
> +exit:
> + return retval;
> +}
> +
> +static void astk_disconnect(struct usb_interface *interface)
> +{
> + struct hydro_i_pro_device *hdev = usb_get_intfdata(interface);
> +
> + dev_info(&hdev->udev->dev, "[*] DEINIT DEVICE\n");
In my opinion the style of the log messages is not consistent,
sometimes you use all-caps, sometimes it's all lowercase, sometimes
it has punctuation.
> + usb_set_intfdata(interface, NULL);
> + astk_delete(hdev);
> + deinit_device(hdev->udev);
At this point 'hdev' is already freed, and 'hdev->udev' is already "put".
Yet both are used.
> +}
> [...]
> +static int __init hydro_i_pro_init(void)
> +{
> + return usb_register(&hydro_i_pro_driver);
> +}
> +
> +static void __exit hydro_i_pro_exit(void)
> +{
> + usb_deregister(&hydro_i_pro_driver);
> +}
> +
> +module_init(hydro_i_pro_init);
> +module_exit(hydro_i_pro_exit);
Any reason for not using the module_usb_driver() macro?
> [...]
Barnabás Pőcze
