On Sun, 16 Aug 2020 at 00:54, Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> 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.
