Re: [PATCH V4] hwmon: add fan/pwm driver for corsair h100i platinum

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux