| Message ID | 20200814194202.327141-1-jaap.aarts1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V3] hwmon: add fan/pwm driver for corsair h100i platinum | expand |
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
On Sat, 15 Aug 2020 at 01:30, Barnabás Pőcze <pobrn@protonmail.com> wrote: Thanks for the quick response, I implemented most of the items you talked about. > > + * > > + * 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. Changed all the int values to be the correct size and using uN. > > + 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. I am not sure what you mean by this, I should probably make the type consistent between them, is that what you mean by "size"? > 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. Yes, I switched to using just a mutex, I saw this being used in the usb_skel driver. I was told earlier to remove the mutex and keep the semaphore, but a mutex makes more sense to me as well. Fixed 2 bugs along the way! > > +}; > > + > > +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**'. No, I will have to add something like `hwmon_pump_data` for pump information. This will also have to use fan/rpm hwmon interfaces so will share the same channel range as `hwmon_fan_data`. This way I can just use channelnr as index. If I made pump a separate array/value(it's almost always one pump) I could no longer just use channelnr as index. If you are against this then I can change it, but it is a deliberate choice to make it void and not an accident. > 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. The "flexible array member" seems to give little benefit over what I am doing right now. Since there are at most 3 fans and 1 pump in a realistic configuration a static array doesn't seem that bad actually. > > +}; > > + > > +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. I moved all the pwm values to u8, and a;ll rpm values to u16(as they can't be any bigger) I usually don't really write c code, all the different types for the same thing confuse me in which one to use. > > + long mode; > > If the only valid modes are 0, 1, and 2, why the 'long'? Also moved to u8. > > +} > > + > > +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] } That is nice! > > +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. I put range checks around all input -> fan/pwm target conversions, if you found any other places let me know. > > + > > + 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. I could, but the only part that could actually be split up is the usb_bulk_msg. If I would remove the error code that part could also be split up. Are there any conventions around what to log/not to log? Maybe it is best to remove those log messages anyways. > > + > > + 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? Irrelevant since I moved to a fixed-size array. > > + > > + /* 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? Maybe?? I don't actually know, would take some debugging/wireshark to figure out. (there were some problems with wireshark and starting the windows driver from what I remember, so might not be possible to find out) by default it uses the dafault profile present in the driver. Only if another program interferes with the device would this not match. > > + > > + 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. I just made the name be part of the device config, this way its device specific. > > + {}, > > +}; > > + > > +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. I knew I missed one of them! :( > > + } > > + > > + 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. Again I couldn't find anything on logging style within the kernel, I am leaning towards just removing them all together. If you can find any logging style guide link me to it, if not I will just remove all the logs, > > [...] > > +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? Just didn't know it existed. Once you reply to my open comments I will send in V4, if it's prefered to just add V4 not and address those comments later let me know and I will just post them. Kind regards, Jaap Aarts
> [...] > > > +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. > > I am not sure what you mean by this, I should probably make the type > consistent between them, is that what you mean by "size"? > Oops, yes, I meant type, not size. > [...] > > > +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. > > I moved all the pwm values to u8, and a;ll rpm values to u16(as they > can't be any bigger) > I usually don't really write c code, all the different types for the > same thing confuse me in > which one to use. > Well, I personally try to use unsigned and the smallest integral type that is acceptable in all the code I write - whenever possible. > [...] > > > +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. > > I could, but the only part that could actually be split up is the usb_bulk_msg. > If I would remove the error code that part could also be split up. > Are there any conventions around what to log/not to log? Maybe it is best to > remove those log messages anyways. > What I had in mind was something like this: // fill send_buf retval = usb_transaction(...); if (retval) // dev_err(...), etc. // process recv_buf if necessary And that function would include the two calls to usb_bulk_msg(), and the call to check_succes(). You could even write this function in such a way that all locking is done only in that function minimizing the possibility of locking related issues. But really, it's up to you. > [...] > > > +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. > > Again I couldn't find anything on logging style within the kernel, I am leaning > towards just removing them all together. If you can find any logging style > guide link me to it, if not I will just remove all the logs, > What I meant is that even in this single module, the style is not consistent. I don't suggest you get rid of them, just that you use a consistent style. Another thing, if you want to log a failure, then it should be dev_err() or dev_warn() depending on the severity - in my opinion. https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages talks about kernel messages. > [...] Barnabás Pőcze
> [...] > > > > +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. > > > > I moved all the pwm values to u8, and a;ll rpm values to u16(as they > > can't be any bigger) > > I usually don't really write c code, all the different types for the > > same thing confuse me in > > which one to use. > > > > Well, I personally try to use unsigned and the smallest integral type > that is acceptable in all the code I write - whenever possible. I think I kinda got the hang of it now. There are so many options like and son have very weird names like uintN_t. The naming of char also confuses me, it feels like I am using strings as byte arrays. As long as I keep using uN from now on it will be fine I think. [...] > > > > +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. > > > > I could, but the only part that could actually be split up is the usb_bulk_msg. > > If I would remove the error code that part could also be split up. > > Are there any conventions around what to log/not to log? Maybe it is best to > > remove those log messages anyways. > > > > What I had in mind was something like this: > > > // fill send_buf > > retval = usb_transaction(...); > > if (retval) > // dev_err(...), etc. > > // process recv_buf if necessary > > > And that function would include the two calls to usb_bulk_msg(), and the > call to check_succes(). You could even write this function in such a way > that all locking is done only in that function minimizing the possibility > of locking related issues. But really, it's up to you. I ended up putting everything strictly USB into this function, and everything driver protocol related outside of it, so `check_succes()` is put outside because there is nothing inherently USB about it. I also dont want to put the locking in the USB function, because some "transactions" require multiple usb messages. These are not implemented yet but from what I remember most RGB light effects need this. I don't know if I will ever implement this since the LED subsystem is very primitive for this, but I want to keep the door open. > > [...] > > > > +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. > > > > Again I couldn't find anything on logging style within the kernel, I am leaning > > towards just removing them all together. If you can find any logging style > > guide link me to it, if not I will just remove all the logs, > > > > What I meant is that even in this single module, the style is not consistent. > I don't suggest you get rid of them, just that you use a consistent style. > > Another thing, if you want to log a failure, then it should be > dev_err() or dev_warn() depending on the severity - in my opinion. > > https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages > talks about kernel messages. yeah I read that already, doesn't really talk much about what should be logged/should not. I ended up just making them more consistent, after noticing other drivers used %s I figured I could do that too, and made the messages a bit nicer too. > [...] I got everything compiling, I have to restart, then I will test and send in a new patch Kind regards, Jaap Aarts
sorry forgot to reply to the question about the buffer sizes. The reason they are different sizes (in theory) is because I get the size for each buffer from usb_endpoint_maxp. I do not know whether or not these are the same. If you have any experience with these functions and know they are always the same I am willing to make them the same size. On Sat, 15 Aug 2020 at 22:31, jaap aarts <jaap.aarts1@gmail.com> wrote: > > > [...] > > > > > +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. > > > > > > I moved all the pwm values to u8, and a;ll rpm values to u16(as they > > > can't be any bigger) > > > I usually don't really write c code, all the different types for the > > > same thing confuse me in > > > which one to use. > > > > > > > Well, I personally try to use unsigned and the smallest integral type > > that is acceptable in all the code I write - whenever possible. > > I think I kinda got the hang of it now. There are so many options like > and son have very weird names like uintN_t. The naming of char > also confuses me, it feels like I am using strings as byte arrays. > As long as I keep using uN from now on it will be fine I think. > > [...] > > > > > +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. > > > > > > I could, but the only part that could actually be split up is the usb_bulk_msg. > > > If I would remove the error code that part could also be split up. > > > Are there any conventions around what to log/not to log? Maybe it is best to > > > remove those log messages anyways. > > > > > > > What I had in mind was something like this: > > > > > > // fill send_buf > > > > retval = usb_transaction(...); > > > > if (retval) > > // dev_err(...), etc. > > > > // process recv_buf if necessary > > > > > > And that function would include the two calls to usb_bulk_msg(), and the > > call to check_succes(). You could even write this function in such a way > > that all locking is done only in that function minimizing the possibility > > of locking related issues. But really, it's up to you. > > I ended up putting everything strictly USB into this function, and everything > driver protocol related outside of it, so `check_succes()` is put outside > because there is nothing inherently USB about it. > I also dont want to put the locking in the USB function, because some > "transactions" require multiple usb messages. These are not implemented > yet but from what I remember most RGB light effects need this. > I don't know if I will ever implement this since the LED subsystem is very > primitive for this, but I want to keep the door open. > > > > [...] > > > > > +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. > > > > > > Again I couldn't find anything on logging style within the kernel, I am leaning > > > towards just removing them all together. If you can find any logging style > > > guide link me to it, if not I will just remove all the logs, > > > > > > > What I meant is that even in this single module, the style is not consistent. > > I don't suggest you get rid of them, just that you use a consistent style. > > > > Another thing, if you want to log a failure, then it should be > > dev_err() or dev_warn() depending on the severity - in my opinion. > > > > https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages > > talks about kernel messages. > > yeah I read that already, doesn't really talk much about what should > be logged/should not. > I ended up just making them more consistent, after noticing other > drivers used %s I figured > I could do that too, and made the messages a bit nicer too. > > > [...] > > I got everything compiling, I have to restart, then I will test and > send in a new patch > > Kind regards, > > Jaap Aarts
I don't recall ever asking about sizes. I said in my previous email that I meant "type", not "size". 'bulk_out_buffer' was 'unsigned char*', but 'bulk_in_buffer' was 'char*' in the patch. That's what I referred to. 2020. augusztus 15., szombat 22:44 keltezéssel, jaap aarts <jaap.aarts1@gmail.com> írta: > sorry forgot to reply to the question about the buffer sizes. > The reason they are different sizes (in theory) is because > I get the size for each buffer from usb_endpoint_maxp. > I do not know whether or not these are the same. If you have > any experience with these functions and know they are always > the same I am willing to make them the same size. > > [...]
oh oops, it must have gotten in my head differently then how I interpreted it at the time. > > I don't recall ever asking about sizes. I said in my previous email > that I meant "type", not "size". 'bulk_out_buffer' was 'unsigned char*', > but 'bulk_in_buffer' was 'char*' in the patch. That's what I referred to. > > 2020. augusztus 15., szombat 22:44 keltezéssel, jaap aarts <jaap.aarts1@gmail.com> írta: > > sorry forgot to reply to the question about the buffer sizes. > > The reason they are different sizes (in theory) is because > > I get the size for each buffer from usb_endpoint_maxp. > > I do not know whether or not these are the same. If you have > > any experience with these functions and know they are always > > the same I am willing to make them the same size. > > > > [...] > >
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..38ca2554a5c1 --- /dev/null +++ b/drivers/hwmon/corsair_hydro_i_pro.c @@ -0,0 +1,718 @@ +// 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 reverse engineered by audiohacked + * https://github.com/audiohacked/OpendriverLink + */ + +/* + * 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. + * + * 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 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; + 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 */ +}; + +struct hwmon_data { + struct hydro_i_pro_device *hdev; + int channel_count; + void **channel_data; +}; + +struct curve_point { + uint8_t temp; + uint8_t pwm; +}; + +struct hwmon_fan_data { + char fan_channel; + long fan_target; + unsigned char fan_pwm_target; + long 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, + .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 strncmp(ret, success, SUCCES_LENGTH) == 0; +} + +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; +} + +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; + + 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; +} + +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; + + if (channel >= data->channel_count) + return -ECHRNG; + + 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; + + 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; + } + 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 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; + + if (channel >= data->channel_count) + return -ECHRNG; + + switch (type) { + case hwmon_fan: + switch (attr) { + case hwmon_fan_input: + 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; + } + + 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: + up(&hdev->limit_sem); +cleanup_interface: + 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) +{ + 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); + + data->channel_count = hdev->config->fancount; + data->channel_data = + devm_kzalloc(&hdev->udev->dev, + sizeof(char *) * data->channel_count, GFP_KERNEL); + + /* 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; + 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, "driver_fan", data, hwmon_info, NULL); + 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) }, + {}, +}; + +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; + 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; + + 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; + } + + 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"); + usb_set_intfdata(interface, NULL); + astk_delete(hdev); + deinit_device(hdev->udev); +} +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, +}; + +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); + +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. temp/fan curves are not supported, you can reset to default if needed. v3: - tineout is now a #define - made all funcions static - removed all retval != 0 - removed horrible mess during hwmon_init using a device table - removed C++ style comments - added USB dependency Sorry for the long delay, I send this patch in earlier before going away, but in some haste forgot to add V3 to the subject. I would have been unavailable for comment anyways. Here is an update on the h100i platinum driver, I'm back and thus available for comment again. Probably wasnt going to make 5.8 anyways, hoping to make 5.9 :). Signed-off-by: Jaap Aarts <jaap.aarts1@gmail.com> --- drivers/hwmon/Kconfig | 7 + drivers/hwmon/Makefile | 1 + drivers/hwmon/corsair_hydro_i_pro.c | 718 ++++++++++++++++++++++++++++ 3 files changed, 726 insertions(+) create mode 100644 drivers/hwmon/corsair_hydro_i_pro.c