Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver

[Julius Zint]

On 20.08.23 12:06, Christophe JAILLET wrote:
> [...]
>
> > +static void hid_bl_remove(struct hid_device *hdev)
> > +{
> > +    struct backlight_device *bl;
> > +    struct hid_bl_data *data;
> > +
> > +    hid_dbg(hdev, "remove\n");
> > +    bl = hid_get_drvdata(hdev);
> > +    data = bl_get_data(bl);
> > +
> > +    devm_backlight_device_unregister(&hdev->dev, bl);
>
> Hi,
>
> If this need to be called here, before the hid_() calls below, there 
> seems to be no real point in using devm_backlight_device_register() / 
> devm_backlight_device_unregister().
>
> The non-devm_ version should be enough.
The non-devm_ version is marked deprecated. As for the order, I am not 
really sure. I am
concerned about someone updating the brightness while its being removed. 
So the HID device
would already have been stopped and then I would issue a request and 
wait for completion.

If hid_hw_request and hid_hw_wait can handle this situation we are fine.
> > +    hid_hw_close(hdev);
> > +    hid_hw_stop(hdev);
> > +    hid_set_drvdata(hdev, NULL);
> > +    devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will 
> be freed by the framework.
>
> > +}
>
> [...]
>
> > +    if (input_field->logical_maximum != 
> > feature_field->logical_maximum) {
> > +        hid_warn(hdev, "maximums do not match: %d / %d\n",
> > +             input_field->logical_maximum,
> > +             feature_field->logical_maximum);
> > +        ret = -ENODEV;
> > +        goto exit_stop;
> > +    }
> > +
> > +    hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
> > +
> > +    ret = hid_hw_open(hdev);
> > +    if (ret) {
> > +        hid_err(hdev, "hw open failed: %d\n", ret);
> > +        goto exit_stop;
> > +    }
> > +
> > +    data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
> > +    if (data == NULL) {
> > +        ret = -ENOMEM;
> > +        goto exit_stop;
>
> exit_free?
> in order to undo the hid_hw_open() just above.
Yes, my mistake. This does not call hid_hw_close(). I will fix it in v4.
> > +    }
> > +    data->hdev = hdev;
> > +    data->min_brightness = input_field->logical_minimum;
> > +    data->max_brightness = input_field->logical_maximum;
> > +    data->input_field = input_field;
> > +    data->feature_field = feature_field;
> > +
> > +    memset(&props, 0, sizeof(props));
> > +    props.type = BACKLIGHT_RAW;
> > +    props.max_brightness = data->max_brightness - data->min_brightness;
> > +
> > +    bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
> > +                        &hdev->dev, data,
> > +                        &hid_bl_ops,
> > +                        &props);
> > +    if (IS_ERR(bl)) {
> > +        ret = PTR_ERR(bl);
> > +        hid_err(hdev, "failed to register backlight: %d\n", ret);
> > +        goto exit_free;
> > +    }
> > +
> > +    hid_set_drvdata(hdev, bl);
> > +
> > +    return 0;
> > +
> > +exit_free:
> > +    hid_hw_close(hdev);
> > +    devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will 
> be freed by the framework.
>
> > +
> > +exit_stop:
> > +    hid_hw_stop(hdev);
> > +    return ret;
> > +}
>
> [...]
I will remove all of the explicit calls to devm_kfree (and others) in v4 
(and test it thoroughly).

Thank you for the helpful review.

Julius