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

On 20.08.23 12:06, Christophe JAILLET wrote:

[...]

quoted

+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.

quoted

+    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.

quoted

+}

[...]

quoted

+    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.

quoted

+    }
+    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.

quoted

+
+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