| Message ID | 20200815211617.86565-1-jaap.aarts1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V4] hwmon: add fan/pwm driver for corsair h100i platinum | expand |
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
On 8/15/20 3:54 PM, Barnabás Pőcze wrote: > 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; > I would suggest to use clamp_val; we don't expect users to know device specific limits. Also, FTR, I won't accept any Yoda programming. Guenter > 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 >
> [...] > >> + 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; > > > > I would suggest to use clamp_val; we don't expect users to know device > specific limits. Also, FTR, I won't accept any Yoda programming. > > Guenter > [...] If you don't mind me asking, why is that? I, too, don't like Yoda conditions, but I've always thought that in this specific case - when checking if a value lies within/outside a certain range - it makes the code more easily readable if it is written like this: (lo < val && val < hi). Maybe I'm just too accustomed to it? (Don't get me wrong, I'm not trying to argue with your decision about not accepting such code, neither am I trying to convince you otherwise.) Barnabás Pőcze
On 8/15/20 5:27 PM, Barnabás Pőcze wrote: >> [...] >>>> + 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; >>> >> >> I would suggest to use clamp_val; we don't expect users to know device >> specific limits. Also, FTR, I won't accept any Yoda programming. >> >> Guenter >> [...] > > > If you don't mind me asking, why is that? I, too, don't like Yoda conditions, > but I've always thought that in this specific case - when checking if a value > lies within/outside a certain range - it makes the code more easily readable > if it is written like this: (lo < val && val < hi). > You are suggesting (val < lo || hi < val) here, which is a bit different. Anyway, good for you. If you are signing up as code reviewer for a Linux kernel subsystem, please feel free to accept and promote such code. For me, it is just confusing. Guenter > Maybe I'm just too accustomed to it? > > (Don't get me wrong, I'm not trying to argue with your decision about not > accepting such code, neither am I trying to convince you otherwise.) > > > Barnabás Pőcze >
2020. augusztus 16., vasárnap 2:51 keltezéssel, Guenter Roeck írta: > On 8/15/20 5:27 PM, Barnabás Pőcze wrote: > >> [...] > >>>> + 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; > >>> > >> > >> I would suggest to use clamp_val; we don't expect users to know device > >> specific limits. Also, FTR, I won't accept any Yoda programming. > >> > >> Guenter > >> [...] > > > > > > If you don't mind me asking, why is that? I, too, don't like Yoda conditions, > > but I've always thought that in this specific case - when checking if a value > > lies within/outside a certain range - it makes the code more easily readable > > if it is written like this: (lo < val && val < hi). > > > > You are suggesting (val < lo || hi < val) here, which is a bit different. > > Anyway, good for you. If you are signing up as code reviewer for a Linux kernel > subsystem, please feel free to accept and promote such code. For me, it is just > confusing. > > Guenter > Thank you for the answer. Barnabás Pőcze
On Sun, 16 Aug 2020 at 00:54, Barnabás Pőcze <pobrn@protonmail.com> wrote: > [...] > > +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. I ended up using `wrote`, so I still need those lines now. > > +} > > + > > +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. You told me I should be consistent with `sizeof` using variables vs types? I agree that it should be `sizeof(fan_data->curve)`, that's what I used before. > [...] > > + 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()) Yes, the device simply doesnt return them, I had the expected bytes returned wrong as well, they should be 3. > [...] > > + > > +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) I should not do things at night, I did mean 1<<16, although BIT(16) is probably nicer. I thought about casting it to u8/u16 and checking if it is still the same, but just comparing it to `BIT(16)-1` gives clearer intentions. > 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. Just tried, the device has failsafes for this :) invalid argument is returned. > [...] > > +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)' That sounds alright. > [...] > > + /* 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. I am just going to clamp this just like the rpm/pwm values. > > + 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? probably yeah, I still have no idea why the error occurs,but it is required when switching from windows driver to linux. > > + > > + 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) Aah ok, you just told me to be more consistent so I moved everything to sizeof(type) while it should have been all to sizeof(var). > [...] Ok, I fixed all of the issues, I also made the input prm/pwm clamped by min/max values I manually tested.
Hi jaap,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hwmon/hwmon-next]
[also build test WARNING on v5.8 next-20200814]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/jaap-aarts/hwmon-add-fan-pwm-driver-for-corsair-h100i-platinum/20200816-085929
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
config: nios2-randconfig-c003-20200816 (attached as .config)
compiler: nios2-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/hwmon/corsair_hydro_i_pro.c: In function 'hwmon_init':
>> drivers/hwmon/corsair_hydro_i_pro.c:529:17: warning: variable 'hwmon_dev' set but not used [-Wunused-but-set-variable]
529 | struct device *hwmon_dev;
| ^~~~~~~~~
drivers/hwmon/corsair_hydro_i_pro.c: In function 'init_device':
>> drivers/hwmon/corsair_hydro_i_pro.c:584:3: warning: suggest braces around empty body in an 'if' statement [-Wempty-body]
584 | ;
| ^
vim +/hwmon_dev +529 drivers/hwmon/corsair_hydro_i_pro.c
525
526 static void hwmon_init(struct hydro_i_pro_device *hdev)
527 {
528 u8 fan_id;
> 529 struct device *hwmon_dev;
530 struct hwmon_fan_data *fan;
531 struct hwmon_data *data = devm_kzalloc(
532 &hdev->udev->dev, sizeof(struct hwmon_data), GFP_KERNEL);
533 struct hwmon_chip_info *hwmon_info = devm_kzalloc(
534 &hdev->udev->dev, sizeof(struct hwmon_chip_info), GFP_KERNEL);
535
536 /* You did something bad!! Either adjust max_fan_count or the fancount for the config!*/
537 WARN_ON(hdev->config->fancount >= max_pwm_channel_count);
538 data->channel_count = hdev->config->fancount;
539
540 /* For each fan create a data channel a fan config entry and a pwm config entry */
541 for (fan_id = 0; fan_id < data->channel_count; fan_id++) {
542 fan = devm_kzalloc(&hdev->udev->dev,
543 sizeof(struct hwmon_fan_data), GFP_KERNEL);
544 fan->fan_channel = fan_id;
545 fan->mode = 0;
546 data->channel_data[fan_id] = fan;
547 }
548
549 hwmon_info->ops = &i_pro_ops;
550 hwmon_info->info = hdev->config->hwmon_info;
551
552 data->hdev = hdev;
553 hwmon_dev = devm_hwmon_device_register_with_info(
554 &hdev->udev->dev, hdev->config->name, data, hwmon_info, NULL);
555 dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
556 }
557
558 const int USB_VENDOR_ID_CORSAIR = 0x1b1c;
559 const int USB_PRODUCT_ID_H100I_PRO = 0x0c15;
560 /*
561 * Devices that work with this driver.
562 * More devices should work, however none have been tested.
563 */
564 static const struct usb_device_id astk_table[] = {
565 { USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_PRODUCT_ID_H100I_PRO),
566 .driver_info = (kernel_ulong_t)&config_table[0] },
567 {},
568 };
569
570 MODULE_DEVICE_TABLE(usb, astk_table);
571
572 static int init_device(struct usb_device *udev)
573 {
574 int retval;
575
576 /*
577 * This is needed because when running windows in a vm with proprietary driver
578 * and you switch to this driver, the device will not respond unless you run this.
579 */
580 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x00, 0x40,
581 0xffff, 0x0000, 0, 0, 0);
582 /*this always returns error*/
583 if (retval)
> 584 ;
585
586 retval = usb_control_msg(udev, usb_sndctrlpipe(udev, 0), 0x02, 0x40,
587 0x0002, 0x0000, 0, 0, 0);
588 return retval;
589 }
590
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index 288ae9f63588..f466b72d0f67 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -378,6 +378,13 @@ config SENSORS_ARM_SCPI and power sensors available on ARM Ltd's SCP based platforms. The actual number and type of sensors exported depend on the platform. +config SENSORS_CORSAIR_HYDRO_I_PRO + tristate "Corsair hydro HXXXi pro driver" + depends on USB + help + If you say yes here you get support for the corsair hydro HXXXi pro + range of devices. + config SENSORS_ASB100 tristate "Asus ASB100 Bach" depends on X86 && I2C diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index 3e32c21f5efe..ec63294b3ef1 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_SENSORS_W83793) += w83793.o obj-$(CONFIG_SENSORS_W83795) += w83795.o obj-$(CONFIG_SENSORS_W83781D) += w83781d.o obj-$(CONFIG_SENSORS_W83791D) += w83791d.o +obj-$(CONFIG_SENSORS_CORSAIR_HYDRO_I_PRO) += corsair_hydro_i_pro.o obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o diff --git a/drivers/hwmon/corsair_hydro_i_pro.c b/drivers/hwmon/corsair_hydro_i_pro.c new file mode 100644 index 000000000000..f4dd435be7dd --- /dev/null +++ b/drivers/hwmon/corsair_hydro_i_pro.c @@ -0,0 +1,694 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * A hwmon driver for all corsair hyxro HXXXi pro all-in-one liquid coolers. + * Copyright (c) Jaap Aarts 2020 + * + * Protocol partially reverse engineered by audiohacked + * https://github.com/audiohacked/OpendriverLink + */ + +/* + * Supports following liquid coolers: + * H100i platinum + * + * Other products should work with this driver with slight modification. + * + * Note: platinum is the codename name for pro within the driver, so H100i platinum = H100i pro. + * But some products are actually called platinum, these are not intended to be supported (yet). + * + * 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; + const char *name; + 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; + unsigned char *bulk_in_buffer; + 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 mutex io_mutex; +}; + +#define max_fan_count 2 +#define max_pwm_channel_count max_fan_count + +struct hwmon_data { + struct hydro_i_pro_device *hdev; + int channel_count; + void *channel_data[max_pwm_channel_count]; +}; + +struct curve_point { + u8 temp; + u8 pwm; +}; + +struct hwmon_fan_data { + u8 fan_channel; + u16 fan_target; + u8 fan_pwm_target; + u8 mode; + struct curve_point curve[7]; +}; + +struct curve_point quiet_curve[] = { + { + .temp = 0x1F, + .pwm = 0x15, + }, + { + .temp = 0x21, + .pwm = 0x1E, + }, + { + .temp = 0x24, + .pwm = 0x25, + }, + { + .temp = 0x27, + .pwm = 0x2D, + }, + { + .temp = 0x29, + .pwm = 0x38, + }, + { + .temp = 0x2C, + .pwm = 0x4A, + }, + { + .temp = 0x2F, + .pwm = 0x64, + }, +}; + +#define default_curve quiet_curve + +static const struct hwmon_channel_info *dual_fan[] = { + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN, + HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_MIN), + HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE, + HWMON_PWM_INPUT | HWMON_PWM_ENABLE), + + NULL +}; + +static const struct device_config config_table[] = { + { + .vendor_id = 0x1b1c, + .product_id = 0x0c15, + .fancount = 2, + .name = "corsair_H100i_pro", + .hwmon_info = dual_fan, + }, +}; + +#define BULK_TIMEOUT 100 + +enum opcodes { + PWM_FAN_CURVE_CMD = 0x40, + PWM_GET_CURRENT_CMD = 0x41, + PWM_FAN_TARGET_CMD = 0x42, + RPM_FAN_TARGET_CMD = 0x43, +}; + +#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 memcmp(ret, success, SUCCES_LENGTH) == 0; +} + +static int acquire_lock(struct hydro_i_pro_device *hdev) +{ + int retval = usb_autopm_get_interface(hdev->interface); + if (retval) + return retval; + + mutex_lock(&hdev->io_mutex); + return 0; +} + +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; +} + +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); + + 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_rpm(struct hydro_i_pro_device *hdev, + struct hwmon_fan_data *fan_data, u16 val) +{ + int retval; + 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; + + retval = transfer_usb(hdev, send_buf, recv_buf, 4, 6); + if (retval) + return retval; + + if (!check_succes(send_buf[0], recv_buf)) { + dev_warn( + &hdev->udev->dev, + "failed setting fan rpm 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 get_fan_current_rpm(struct hydro_i_pro_device *hdev, + struct hwmon_fan_data *fan_data, long *val) +{ + int retval; + 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 = transfer_usb(hdev, send_buf, recv_buf, 2, 6); + if (retval) + return retval; + + if (!check_succes(send_buf[0], recv_buf) || + recv_buf[3] != fan_data->fan_channel) { + dev_warn( + &hdev->udev->dev, + "failed retreiving 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; + } + + *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, 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)) { + 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 umode_t hwmon_is_visible(const void *d, enum hwmon_sensor_types type, + u32 attr, int channel) +{ + switch (type) { + case hwmon_fan: + switch (attr) { + case hwmon_fan_input: + return 0444; + break; + case hwmon_fan_target: + return 0644; + break; + case hwmon_fan_min: + return 0444; + break; + default: + break; + } + break; + case hwmon_pwm: + switch (attr) { + case hwmon_pwm_input: + return 0200; + break; + case hwmon_pwm_enable: + return 0644; + break; + default: + break; + } + break; + default: + break; + } + 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) + 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) + 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; + 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; + 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; + + 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); + 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); +} + +const int USB_VENDOR_ID_CORSAIR = 0x1b1c; +const int USB_PRODUCT_ID_H100I_PRO = 0x0c15; +/* + * 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) + ; + + 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 = + kzalloc(sizeof(struct hydro_i_pro_device), GFP_KERNEL); + int retval; + struct usb_endpoint_descriptor *bulk_in, *bulk_out; + + if (!hdev) { + kfree(hdev); + retval = -ENOMEM; + goto exit; + } + + hdev->config = (const struct device_config *)id->driver_info; + /* You should set the driver_info to a pointer to the correct configuration!!*/ + WARN_ON(hdev->config == NULL); + + retval = usb_find_common_endpoints(interface->cur_altsetting, &bulk_in, + &bulk_out, NULL, NULL); + if (retval) { + kfree(hdev); + goto exit; + } + + 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 %s\n", + hdev->config->name); + kfree(hdev); + goto exit; + } + + hwmon_init(hdev); + + usb_set_intfdata(interface, hdev); + mutex_init(&hdev->io_mutex); +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, "removed hwmon for %s\n", + hdev->config->name); + deinit_device(hdev->udev); + usb_set_intfdata(interface, NULL); + astk_delete(hdev); +} +static int astk_resume(struct usb_interface *intf) +{ + return 0; +} + +static int astk_suspend(struct usb_interface *intf, pm_message_t message) +{ + return 0; +} + +static struct usb_driver hydro_i_pro_driver = { + .name = "hydro_i_pro_device", + .id_table = astk_table, + .probe = astk_probe, + .disconnect = astk_disconnect, + .resume = astk_resume, + .suspend = astk_suspend, + .supports_autosuspend = 1, +}; + +module_usb_driver(hydro_i_pro_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Jaap Aarts <jaap.aarts1@gmail.com>"); +MODULE_DESCRIPTION("Corsair HXXXi pro device driver");
Adds fan/pwm support for H100i platinum. Custom temp/fan curves are not supported. v4: - fixed spelling - more consistent use of uN and other inconsistencies - moved from semaphore to mutex, fixing 2 locking bugs allong the way - moved to memcmp vs strncmp - now uses driver_info for the device configuration - check input ranges for fan rpm/pwm - fix default case - off-by-one loop range - improved naming and logging messages - fixed unfreed hdev - use module_usb_driver - use fixed-sized array for rpm/pwm channels independant of device. - use common function for the USB bulk messages. Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com> --- drivers/hwmon/Kconfig | 7 + drivers/hwmon/Makefile | 1 + drivers/hwmon/corsair_hydro_i_pro.c | 694 ++++++++++++++++++++++++++++ 3 files changed, 702 insertions(+) create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c