Re: [PATCH V5] 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 17:23, Barnabás Pőcze <pobrn@xxxxxxxxxxxxxx> wrote:
>
> Hello,
>
> I added a couple notes about the code inline.
>
>
> > [...]
> > +#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;
>
> This is a nitpick, but the 'channel_count' value comes from the 'fancount'
> of an entry of 'config_table', which has type 'u8', so I don't see any need for
> it to be an 'int'.

In practice this will indeed never go above u8, even with a pump, didn't catch
that one

> [...]
> > +static const struct device_config config_table[] = {
> > +     {
> > +             .vendor_id = 0x1b1c,
> > +             .product_id = 0x0c15,
>
> If I see it correctly, you never use 'vendor_id', nor 'product_id', right?

This was a really sloppy patch apparently. Those were only there for finding
them, so they should indeed go now.

> [...]
> > +
> > +     send_buf[0] = PWM_FAN_TARGET_CMD;
> > +     send_buf[1] = fan_data->fan_channel;
> > +     send_buf[2] = fan_data->fan_pwm_target;
> > +     dev_info(&hdev->udev->dev, "debug:%d,%d,%d", send_buf[0], send_buf[1],
> > +              send_buf[2]);
> > +     dev_info(&hdev->udev->dev, "val:%d", fan_data->fan_pwm_target);
>
> This should be dev_dbg() in my opinion if you intend to have such messages.

Those weren't even supposed to go in :(

> [...]
> > +                     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 2:
> > +                     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;
>
> This could be 'fan_data->mode = val', no?

I didnt think about that, the mode can be read from userspace and
when userspace sets it to 2 they probably expect to read 2.
I usually try to use constants as much as possible instead of using
variables when the value is known, but I missed this when merging
the statements.

> [...]
> > +static int hwmon_init(struct hydro_i_pro_device *hdev)
> > +{
> > +     u8 fan_id;
> > +     struct device *hwmon_dev;
> > +     struct hwmon_fan_data *fan;
> > +     struct hwmon_data *data;
> > +     struct hwmon_chip_info *hwmon_info;
> > +
> > +     data = devm_kzalloc(&hdev->udev->dev, sizeof(*data), GFP_KERNEL);
> > +     hwmon_info = devm_kzalloc(&hdev->udev->dev,
> > +                               sizeof(struct hwmon_chip_info), GFP_KERNEL);
>
> I think you missed this 'sizeof' when coverting to 'sizeof(*ptr)'.

Yes, I did indeed mist that one :/

>[...]
> > +             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);
> > +     if (IS_ERR(hwmon_dev))
> > +             return PTR_ERR(hwmon_dev);
> > +
> > +     dev_info(&hdev->udev->dev, "setup hwmon for %s\n", hdev->config->name);
> > +     return 0;
> > +}
>
> It is still possible that hwmon_init() leaks memory on failure.

you mean the allocated memory doesn't get deallocated until
the driver detaches? I can't free the memory myself because
when the driver does detach it will double-free.
I have looked at how other drivers handle this, none of them
deallocate any memory.
If you mean error check the allocations, I added those now.

> [...]

Kind regards,

Jaap Aarts




[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux