| 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: <Y+jPG4/EkYt1otap@smile.fi.intel.com>
Date: Sun, 12 Feb 2023 13:35:55 +0200
From: Andy Shevchenko <andy@...radead.org>
To: Aditya Garg <gargaditya08@...e.com>
Cc: Jiri Kosina <jikos@...nel.org>,
"jkosina@...e.cz" <jkosina@...e.cz>,
"benjamin.tissoires@...hat.com" <benjamin.tissoires@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"ronald@...ovation.ch" <ronald@...ovation.ch>,
"kekrby@...il.com" <kekrby@...il.com>,
Orlando Chamberlain <orlandoch.dev@...il.com>
Subject: Re: [PATCH 1/3] HID: apple-ibridge: Add Apple iBridge HID driver for
T1 chip.
On Fri, Feb 10, 2023 at 03:43:24AM +0000, Aditya Garg wrote:
> From: Ronald Tschalär <ronald@...ovation.ch>
>
> The iBridge device provides access to several devices, including:
> - the Touch Bar
> - the iSight webcam
> - the light sensor
> - the fingerprint sensor
>
> This driver provides the core support for managing the iBridge device
> and the access to the underlying devices. In particular, the
> functionality for the touch bar and light sensor is exposed via USB HID
> interfaces, and on devices with the T1 chip one of the HID devices is
> used for both functions. So this driver creates virtual HID devices, one
> per top-level report collection on each HID device (for a total of 3
> virtual HID devices). The sub-drivers then bind to these virtual HID
> devices.
>
> This way the Touch Bar and ALS drivers can be kept in their own modules,
> while at the same time making them look very much like as if they were
> connected to the real HID devices. And those drivers then work (mostly)
> without further changes on MacBooks with the T2 chip that don't need
> this driver.
...
> [Kerem Karabay: convert to a platform driver]
> [Kerem Karabay: fix appleib_forward_int_op]
> [Kerem Karabay: rely on HID core's parsing in appleib_add_device]
If somebody is going to update this (and update seems required for upstreaming)
the list of changes will grow. I suggest to consider Co-developed-by and move
these lines to cover-letter changelog.
> Signed-off-by: Kerem Karabay <kekrby@...il.com>
...
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
Can we keep it sorted?
> +#include "hid-ids.h"
> +#include "../hid/usbhid/usbhid.h"
+ Blank line?
> +#include "apple-ibridge.h"
...
> +static struct hid_device_id appleib_sub_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_TB) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_LINUX_FOUNDATION,
> + USB_DEVICE_ID_IBRIDGE_ALS) },
> +};
> +
> +static struct {
> + unsigned int usage;
> + struct hid_device_id *dev_id;
> +} appleib_usage_map[] = {
> + /* Default iBridge configuration, key inputs and mode settings */
> + { 0x00010006, &appleib_sub_hid_ids[0] },
> + /* OS X iBridge configuration, digitizer inputs */
> + { 0x000D0005, &appleib_sub_hid_ids[0] },
> + /* All iBridge configurations, display/DFR settings */
> + { 0xFF120001, &appleib_sub_hid_ids[0] },
> + /* All iBridge configurations, ALS */
> + { 0x00200041, &appleib_sub_hid_ids[1] },
> +};
Shouldn't be other way around, i.e. via driver_data?
...
> +struct appleib_device {
> + acpi_handle asoc_socw;
> +};
What's the point of having struct out of a single member? Can you use it directly?
(you can try and see if it's not ugly, in some cases struct can be justified)
...
> + bool sub_open[ARRAY_SIZE(appleib_sub_hid_ids)];
Why not using bitmap?
DECLARE_BITMAP(sub_open, ARRAY_SIZE(...));
...
> +static __u8 *appleib_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> + unsigned int *rsize)
Why __ types are in use? Is it part of ABI?
...
> +static int appleib_forward_int_op(struct hid_device *hdev,
> + int (*forward)(struct hid_driver *,
> + struct hid_device *, void *),
This can be on one line
> + void *args)
...
> + if (drv->suspend)
> + rc = drv->suspend(hdev, *(pm_message_t *)args);
This looks like a hack. What's going on here and why the pm_message_t is in
use? All new PM callbacks do not use it.
...
> + for (i = 0; i < ARRAY_SIZE(hdev_info->sub_hdevs); i++) {
> + /*
> + * hid_hw_open(), and hence appleib_ll_open(), is called
> + * from the driver's probe function, which in turn is called
> + * while adding the sub-hdev; but at this point we haven't yet
> + * added the sub-hdev to our list. So if we don't find the
> + * sub-hdev in our list assume it's in the process of being
> + * added and set the flag on the first unset sub-hdev.
> + */
> + if (hdev_info->sub_hdevs[i] == hdev ||
> + !hdev_info->sub_hdevs[i]) {
Unusual order of || operator arguments.
This will have a side effect, i.e. if hdev is equal to NULL it will go to the
true branch. Is it by design?
> + WRITE_ONCE(hdev_info->sub_open[i], open);
> + return 0;
> + }
> + }
...
> + while (i-- > 0)
while (i--) ?
> + hid_destroy_device(hdev_info->sub_hdevs[i]);
> + return (void *)hdev_info->sub_hdevs[i];
This casting is strange. And entire code piece. You will always return 0
element as a pointer here, why 'i'? Needs a lot of explanation.
...
> +static const struct hid_device_id appleib_hid_ids[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_IBRIDGE) },
> + { },
No comma for the terminator entry.
> +};
...
> +#ifdef CONFIG_PM
> + .suspend = appleib_hid_suspend,
> + .resume = appleib_hid_resume,
> + .reset_resume = appleib_hid_reset_resume,
> +#endif
Why not using
.driver = {
.pm = ...;
},
?
...
> + ret = hid_register_driver(&appleib_hid_driver);
> + if (ret) {
> + dev_err(&pdev->dev, "Error registering hid driver: %d\n",
> + ret);
> + return ret;
return dev_err_probe(...);
> + }
...
> +static int appleib_suspend(struct platform_device *pdev, pm_message_t message)
> +{
> + struct appleib_device *ib_dev;
> + int rc;
> + ib_dev = platform_get_drvdata(pdev);
Just unite it with the definition above.
Ditto for the similar cases here and there.
> + rc = acpi_execute_simple_method(ib_dev->asoc_socw, NULL, 0);
> + if (ACPI_FAILURE(rc))
> + dev_warn(&pdev->dev, "SOCW(0) failed: %s\n",
> + acpi_format_exception(rc));
> +
> + return 0;
> +}
...
> +static const struct acpi_device_id appleib_acpi_match[] = {
> + { "APP7777", 0 },
> + { },
No comma for terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists