Hello, there are a couple things that I did not notice in v3, or were introduced in this version of the patch. > [...] > + > +#define max_fan_count 2 > +#define max_pwm_channel_count max_fan_count > + I think these should be all-caps as per https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl > [...] > + > +#define default_curve quiet_curve > + I am inclined to say that this should be all-caps as well. > [...] > +static int transfer_usb(struct hydro_i_pro_device *hdev, > + unsigned char *send_buf, unsigned char *recv_buf, > + int send_len, int recv_len) > +{ > + int retval; > + int wrote; > + int sndpipe = usb_sndbulkpipe(hdev->udev, hdev->bulk_out_endpointAddr); > + int rcvpipe = usb_rcvbulkpipe(hdev->udev, hdev->bulk_in_endpointAddr); > + > + retval = usb_bulk_msg(hdev->udev, sndpipe, send_buf, send_len, &wrote, > + BULK_TIMEOUT); > + if (retval) > + return retval; > + > + retval = usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote, > + BULK_TIMEOUT); > + if (retval) > + return retval; > + return 0; The preceding 5 lines could be simplified to the following: return usb_bulk_msg(hdev->udev, rcvpipe, recv_buf, recv_len, &wrote, BULK_TIMEOUT); And if you don't use 'wrote', you can simply pass 'NULL' as the 5th argument of usb_bulk_msg(). Although you should either check the value or set 'recv_buf' to all zeroes in the calling function to avoid the possibility of a failed transaction being recognized as successful. > +} > + > +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; > + unsigned char *send_buf = hdev->bulk_out_buffer; > + unsigned char *recv_buf = hdev->bulk_in_buffer; > + > + memcpy(fan_data->curve, point, sizeof(struct curve_point) * 7); > + Personally, I'd use 'sizeof(fan_data->curve)' here. And consider making that seven a named constant. Beware that even though the size is there in 'struct curve_point point[7]', you can still pass arrays that are smaller than that. Consider using 'struct curve_point point[static 7]'. > + 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 = transfer_usb(hdev, send_buf, recv_buf, 16, 4); > + if (retval) > + return retval; > + > + if (!check_succes(send_buf[0], recv_buf)) { > + dev_warn( > + &hdev->udev->dev, > + "failed setting fan pwm/temp curve for %s on channel %d %d,%d,%d\n", > + hdev->config->name, recv_buf[3], recv_buf[0], > + recv_buf[1], recv_buf[2]); > + return -EINVAL; > + } > + return 0; > +} > + > [...] > + > +static int set_fan_target_pwm(struct hydro_i_pro_device *hdev, > + struct hwmon_fan_data *fan_data, u8 val) > +{ > + int retval; > + unsigned char *send_buf = hdev->bulk_out_buffer; > + unsigned char *recv_buf = hdev->bulk_in_buffer; > + > + fan_data->fan_target = 0; > + fan_data->fan_pwm_target = val; > + > + send_buf[0] = PWM_FAN_TARGET_CMD; > + send_buf[1] = fan_data->fan_channel; > + send_buf[3] = fan_data->fan_pwm_target; > + > + retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6); > + if (retval) > + return retval; > + > + if (!check_succes(send_buf[0], recv_buf)) { Any reason why you don't check recv_buf[3] as in get_fan_current_rpm()? (same applies to set_fan_pwm_curve() and set_fan_target_rpm()) > + dev_warn( > + &hdev->udev->dev, > + "failed setting fan pwm for %s on channel %d %d,%d,%d\n", > + hdev->config->name, recv_buf[3], recv_buf[0], > + recv_buf[1], recv_buf[2]); > + return -EINVAL; > + } > + return 0; > +} > + > [...] > + > +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; > + > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_target: > + fan_data = data->channel_data[channel]; > + if (fan_data->mode != 1) > + return -EINVAL; > + > + if (val < (2 ^ 16) - 2) Did you intend to write 2 to the power of 16? Because 2^16 is not that. 2 to the power 16 may be written as '(1 << 16)' or 'BIT(16)' (you may need to include linux/bits.h for the latter) But something like this is my suggestion: if (val < 0 || 0xFFFF < val) return -EINVAL; Though, I suspect the fans won't go up to 60000 RPM or so. > + return -EINVAL; > + > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + retval = set_fan_target_rpm(hdev, fan_data, val); > + if (retval) > + goto cleanup; > + > + break; > + default: > + return -EINVAL; > + } > + break; > + case hwmon_pwm: > + switch (attr) { > + case hwmon_pwm_input: > + fan_data = data->channel_data[channel]; > + if (fan_data->mode != 1) > + return -EINVAL; > + > + if (val < (2 ^ 8) - 2) Same here, 2^8 != 2 to the power of 8. I suggest you simply do something like the following: if (val < 0 || 0xFF < val) return -EINVAL; > + return -EINVAL; > + > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + retval = set_fan_target_pwm(hdev, fan_data, val); > + if (retval) > + goto cleanup; > + > + break; > + case hwmon_pwm_enable: > + fan_data = data->channel_data[channel]; > + > + switch (val) { > + case 0: > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + retval = set_fan_pwm_curve(hdev, fan_data, > + default_curve); > + if (retval) > + goto cleanup; > + fan_data->mode = 0; > + break; > + case 1: > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + 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; > + } > + fan_data->mode = 1; > + break; > + case 2: > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + retval = set_fan_pwm_curve(hdev, fan_data, > + default_curve); > + if (retval) > + goto cleanup; > + fan_data->mode = 2; > + break; If I see it correctly, case 0 and case 2 are the basically the same, no? If so, then you could merge them. > + default: > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + > +cleanup: > + mutex_unlock(&hdev->io_mutex); > + usb_autopm_put_interface(hdev->interface); > +exit: > + return retval; > +} > + > +static int hwmon_read(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; > + > + switch (type) { > + case hwmon_fan: > + switch (attr) { > + case hwmon_fan_input: > + fan_data = data->channel_data[channel]; > + > + retval = acquire_lock(hdev); > + if (retval) > + goto exit; > + > + retval = get_fan_current_rpm(hdev, fan_data, val); > + if (retval) > + goto cleanup; > + > + goto cleanup; The preceding 3 lines can be replaced by a single 'break' given that the 'goto exit' is replaced by a 'break' after the 'switch (attr)' > + case hwmon_fan_target: > + fan_data = data->channel_data[channel]; > + if (fan_data->mode != 1) { > + *val = 0; > + goto exit; > + } > + *val = fan_data->fan_target; > + goto exit; > + case hwmon_fan_min: > + *val = 200; > + goto exit; > + > + default: > + return -EINVAL; > + } > + goto exit; > + I don't see why this goto is needed here. It has no effect on the control flow. > + case hwmon_pwm: > + switch (attr) { > + case hwmon_pwm_enable: > + fan_data = data->channel_data[channel]; > + *val = fan_data->mode; > + goto exit; > + default: > + return -EINVAL; > + } > + goto exit; > + > + default: > + return -EINVAL; > + } > + > +cleanup: > + mutex_unlock(&hdev->io_mutex); > + usb_autopm_put_interface(hdev->interface); > +exit: > + return retval; > +} > + > +static const struct hwmon_ops i_pro_ops = { > + .is_visible = hwmon_is_visible, > + .read = hwmon_read, > + .write = hwmon_write, > +}; > + > +static void hwmon_init(struct hydro_i_pro_device *hdev) > +{ > + u8 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); > + > + /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/ > + WARN_ON(hdev->config->fancount >= max_pwm_channel_count); If this warning is triggered, then that leads to the overflow of 'data->channel_data' later on. > + data->channel_count = hdev->config->fancount; > + > + /* 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 = 0; > + data->channel_data[fan_id] = fan; > + } > + > + 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, hdev->config->name, data, hwmon_info, NULL); > + dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name); > +} > + There is absolutely no error checking in hwmon_init(). > +const int USB_VENDOR_ID_CORSAIR = 0x1b1c; > +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15; I think these should be either - preferably - #define's or 'static' at least. > +/* > + * 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(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO), > + .driver_info = (kernel_ulong_t)&config_table[0] }, > + {}, > +}; > + > +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. > + */ > + retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40, > + 0xffff, 0x0000, 0, 0, 0); > + /*this always returns error*/ > + if (retval) > + ; Shouldn't you check if it's the "good" kind of error? > + > + 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); > +} > + I think you should call mutex_destroy() in astk_delete(). > +static int astk_probe(struct usb_interface *interface, > + const struct usb_device_id *id) > +{ > + struct hydro_i_pro_device *hdev = > + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL); I think this should be modifed to use 'sizeof(*ptr)' as per https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory (and everything else that uses the same pattern) > [...] > + retval = init_device(hdev->udev); > + if (retval) { > + dev_err(&interface->dev, "failed initialising %s\n", > + hdev->config->name); If you print the error code, that helps immensely with troubleshooting. > + kfree(hdev); > + goto exit; > + } > + > + hwmon_init(hdev); > + > + usb_set_intfdata(interface, hdev); > + mutex_init(&hdev->io_mutex); > +exit: > + return retval; > +} > + > [...] Barnabás Pőcze
