| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2b7cf5af-1d96-8650-ac93-b1243bab5d7a@wanadoo.fr>
Date: Sun, 20 Aug 2023 12:06:40 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Julius Zint <julius@...t.sh>, Lee Jones <lee@...nel.org>,
Daniel Thompson <daniel.thompson@...aro.org>,
Jingoo Han <jingoohan1@...il.com>,
Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc: Helge Deller <deller@....de>,
Thomas Weißschuh <thomas@...ch.de>,
linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
linux-input@...r.kernel.org, linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight
driver
Le 20/08/2023 à 11:41, Julius Zint a écrit :
> The HID spec defines the following Usage IDs (p. 345 ff):
>
> - Monitor Page (0x80) -> Monitor Control (0x01)
> - VESA Virtual Controls Page (0x82) -> Brightness (0x10)
>
> Apple made use of them in their Apple Studio Display and most likely on
> other external displays (LG UltraFine 5k, Pro Display XDR).
>
> The driver will work for any HID device with a report, where the
> application matches the Monitor Control Usage ID and:
>
> 1. An Input field in this report with the Brightness Usage ID (to get
> the current brightness)
> 2. A Feature field in this report with the Brightness Usage ID (to
> set the current brightness)
>
> This driver has been developed and tested with the Apple Studio Display.
> Here is a small excerpt from the decoded HID descriptor showing the
> feature field for setting the brightness:
>
> Usage Page (Monitor VESA VCP), ; Monitor VESA VPC (82h, monitor page)
> Usage (10h, Brightness),
> Logical Minimum (400),
> Logical Maximum (60000),
> Unit (Centimeter^-2 * Candela),
> Unit Exponent (14),
> Report Size (32),
> Report Count (1),
> Feature (Variable, Null State),
>
> The full HID descriptor dump is available as a comment in the source
> code.
>
> Signed-off-by: Julius Zint <julius@...t.sh>
> ---
[...]
> +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.
> + 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.
> +}
[...]
> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> + struct hid_field *input_field;
> + struct hid_field *feature_field;
> + struct hid_bl_data *data;
> + struct backlight_properties props;
> + struct backlight_device *bl;
> +
> + hid_dbg(hdev, "probe\n");
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "parse failed: %d\n", ret);
> + return ret;
> + }
> +
> + ret = hid_hw_start(hdev, HID_CONNECT_DRIVER);
> + if (ret) {
> + hid_err(hdev, "hw start failed: %d\n", ret);
> + return ret;
> + }
> +
> + input_field = hid_get_usage_field(hdev, HID_INPUT_REPORT,
> + HID_USAGE_MONITOR_CTRL,
> + HID_USAGE_VESA_VCP_BRIGHTNESS);
> + if (input_field == NULL) {
> + ret = -ENODEV;
> + goto exit_stop;
> + }
> +
> + feature_field = hid_get_usage_field(hdev, HID_FEATURE_REPORT,
> + HID_USAGE_MONITOR_CTRL,
> + HID_USAGE_VESA_VCP_BRIGHTNESS);
> + if (feature_field == NULL) {
> + ret = -ENODEV;
> + goto exit_stop;
> + }
> +
> + if (input_field->logical_minimum != feature_field->logical_minimum) {
> + hid_warn(hdev, "minimums do not match: %d / %d\n",
> + input_field->logical_minimum,
> + feature_field->logical_minimum);
> + ret = -ENODEV;
> + goto exit_stop;
> + }
> +
> + 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.
> + }
> + 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;
> +}
[...]
Powered by blists - more mailing lists