* Re: [PATCH RFC v5 4/6] ARM: pxa: Convert reset driver to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-06 7:07 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231004-pxa-gpio-v5-4-d99ae6fceea8@skole.hr>
On Wed, Oct 4, 2023 at 4:56 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> The PXA reset driver still uses the legacy GPIO interface for
> configuring and asserting the reset pin.
>
> Convert it to use the GPIO descriptor interface.
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
> arch/arm/mach-pxa/reset.c | 39 +++++++++++++--------------------------
> arch/arm/mach-pxa/reset.h | 3 +--
> arch/arm/mach-pxa/spitz.c | 6 +++++-
> 3 files changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/mach-pxa/reset.c b/arch/arm/mach-pxa/reset.c
> index 27293549f8ad..08bc104b9411 100644
> --- a/arch/arm/mach-pxa/reset.c
> +++ b/arch/arm/mach-pxa/reset.c
> @@ -2,7 +2,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/io.h>
> #include <asm/proc-fns.h>
> #include <asm/system_misc.h>
> @@ -14,33 +14,20 @@
>
> static void do_hw_reset(void);
>
> -static int reset_gpio = -1;
> +static struct gpio_desc *reset_gpio;
>
> -int init_gpio_reset(int gpio, int output, int level)
> +int init_gpio_reset(int output, int level)
> {
> - int rc;
> -
> - rc = gpio_request(gpio, "reset generator");
> - if (rc) {
> - printk(KERN_ERR "Can't request reset_gpio\n");
> - goto out;
> + reset_gpio = gpiod_get(NULL, "reset", GPIOD_ASIS);
> + if (IS_ERR(reset_gpio)) {
> + pr_err("Can't request reset_gpio: %pe\n", reset_gpio);
> + return PTR_ERR(reset_gpio);
> }
>
> if (output)
> - rc = gpio_direction_output(gpio, level);
> + return gpiod_direction_output(reset_gpio, level);
> else
> - rc = gpio_direction_input(gpio);
> - if (rc) {
> - printk(KERN_ERR "Can't configure reset_gpio\n");
> - gpio_free(gpio);
> - goto out;
> - }
> -
> -out:
> - if (!rc)
> - reset_gpio = gpio;
> -
> - return rc;
> + return gpiod_direction_input(reset_gpio);
> }
>
> /*
> @@ -50,16 +37,16 @@ int init_gpio_reset(int gpio, int output, int level)
> */
> static void do_gpio_reset(void)
> {
> - BUG_ON(reset_gpio == -1);
> + BUG_ON(IS_ERR(reset_gpio));
Crashing the kernel on a GPIO error? I guess it just keeps the old
behavior but still...
>
> /* drive it low */
> - gpio_direction_output(reset_gpio, 0);
> + gpiod_direction_output(reset_gpio, 0);
> mdelay(2);
> /* rising edge or drive high */
> - gpio_set_value(reset_gpio, 1);
> + gpiod_set_value(reset_gpio, 1);
> mdelay(2);
> /* falling edge */
> - gpio_set_value(reset_gpio, 0);
> + gpiod_set_value(reset_gpio, 0);
>
> /* give it some time */
> mdelay(10);
> diff --git a/arch/arm/mach-pxa/reset.h b/arch/arm/mach-pxa/reset.h
> index 963dd190bc13..5864f61a0e94 100644
> --- a/arch/arm/mach-pxa/reset.h
> +++ b/arch/arm/mach-pxa/reset.h
> @@ -13,10 +13,9 @@ extern void pxa_register_wdt(unsigned int reset_status);
>
> /**
> * init_gpio_reset() - register GPIO as reset generator
> - * @gpio: gpio nr
> * @output: set gpio as output instead of input during normal work
> * @level: output level
> */
> -extern int init_gpio_reset(int gpio, int output, int level);
> +extern int init_gpio_reset(int output, int level);
>
> #endif /* __ASM_ARCH_RESET_H */
> diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
> index 965354e64c68..26ec29c9cd1b 100644
> --- a/arch/arm/mach-pxa/spitz.c
> +++ b/arch/arm/mach-pxa/spitz.c
> @@ -1024,9 +1024,13 @@ static void spitz_restart(enum reboot_mode mode, const char *cmd)
> spitz_poweroff();
> }
>
> +GPIO_LOOKUP_SINGLE(spitz_reset_gpio_table, NULL, "pxa-gpio",
> + SPITZ_GPIO_ON_RESET, "reset", GPIO_ACTIVE_HIGH);
> +
> static void __init spitz_init(void)
> {
> - init_gpio_reset(SPITZ_GPIO_ON_RESET, 1, 0);
> + gpiod_add_lookup_table(&spitz_reset_gpio_table);
> + init_gpio_reset(1, 0);
> pm_power_off = spitz_poweroff;
>
> PMCR = 0x00;
>
> --
> 2.42.0
>
>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH RFC v5 5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
From: Bartosz Golaszewski @ 2023-10-06 7:10 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231004-pxa-gpio-v5-5-d99ae6fceea8@skole.hr>
On Wed, Oct 4, 2023 at 4:56 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
> device.
>
> Convert it to use the GPIO descriptor interface.
>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH RFC v5 6/6] input: ads7846: Move wait_for_sync() logic to driver
From: Bartosz Golaszewski @ 2023-10-06 7:11 UTC (permalink / raw)
To: Duje Mihanović
Cc: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij, Andy Shevchenko,
Dmitry Torokhov, Mark Brown, linux-arm-kernel, linux-kernel,
linux-usb, linux-gpio, linux-input, linux-spi
In-Reply-To: <20231004-pxa-gpio-v5-6-d99ae6fceea8@skole.hr>
On Wed, Oct 4, 2023 at 4:56 PM Duje Mihanović <duje.mihanovic@skole.hr> wrote:
>
> If this code is left in the board file, the sync GPIO would have to be
> separated into another lookup table during conversion to the GPIO
> descriptor API (which is also done in this patch).
>
> The only user of this code (Sharp Spitz) is also converted in this
> patch.
>
> Suggested-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Acked-by: Mark Brown <broonie@kernel.org>
> Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
> ---
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Benjamin Tissoires @ 2023-10-06 7:29 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <20231005182638.3776-1-hdegoede@redhat.com>
Hi Hans,
On Oct 05 2023, Hans de Goede wrote:
> hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
> races when it races with itself.
>
> hidpp_connect_event() primarily runs from a workqueue but it also runs
> on probe() and if a "device-connected" packet is received by the hw
> when the thread running hidpp_connect_event() from probe() is waiting on
> the hw, then a second thread running hidpp_connect_event() will be
> started from the workqueue.
>
> This opens the following races (note the below code is simplified):
>
> 1. Retrieving + printing the protocol (harmless race):
>
> if (!hidpp->protocol_major) {
> hidpp_root_get_protocol_version()
> hidpp->protocol_major = response.rap.params[0];
> }
>
> We can actually see this race hit in the dmesg in the abrt output
> attached to rhbz#2227968:
>
> [ 3064.624215] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> [ 3064.658184] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
>
> Testing with extra logging added has shown that after this the 2 threads
> take turn grabbing the hw access mutex (send_mutex) so they ping-pong
> through all the other TOCTOU cases managing to hit all of them:
>
> 2. Updating the name to the HIDPP name (harmless race):
>
> if (hidpp->name == hdev->name) {
> ...
> hidpp->name = new_name;
> }
>
> 3. Initializing the power_supply class for the battery (problematic!):
>
> hidpp_initialize_battery()
> {
> if (hidpp->battery.ps)
> return 0;
>
> probe_battery(); /* Blocks, threads take turns executing this */
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
> }
>
> 4. Creating delayed input_device (potentially problematic):
>
> if (hidpp->delayed_input)
> return;
>
> hidpp->delayed_input = hidpp_allocate_input(hdev);
>
> The really big problem here is 3. Hitting the race leads to the following
> sequence:
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
>
> ...
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
>
> So now we have registered 2 power supplies for the same battery,
> which looks a bit weird from userspace's pov but this is not even
> the really big problem.
>
> Notice how:
>
> 1. This is all devm-maganaged
> 2. The hidpp->battery.desc struct is shared between the 2 power supplies
> 3. hidpp->battery.desc points to the result from the second devm_kmemdup()
>
> This causes a use after free scenario on USB disconnect of the receiver:
> 1. The last registered power supply class device gets unregistered
> 2. The memory from the last devm_kmemdup() call gets freed,
> hidpp->battery.desc.properties now points to freed memory
> 3. The first registered power supply class device gets unregistered,
> this involves sending a remove uevent to userspace which invokes
> power_supply_uevent() to fill the uevent data
> 4. power_supply_uevent() uses hidpp->battery.desc.properties which
> now points to freed memory leading to backtraces like this one:
>
> Sep 22 20:01:35 eric kernel: BUG: unable to handle page fault for address: ffffb2140e017f08
> ...
> Sep 22 20:01:35 eric kernel: Workqueue: usb_hub_wq hub_event
> Sep 22 20:01:35 eric kernel: RIP: 0010:power_supply_uevent+0xee/0x1d0
> ...
> Sep 22 20:01:35 eric kernel: ? asm_exc_page_fault+0x26/0x30
> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0xee/0x1d0
> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0x10d/0x1d0
> Sep 22 20:01:35 eric kernel: dev_uevent+0x10f/0x2d0
> Sep 22 20:01:35 eric kernel: kobject_uevent_env+0x291/0x680
> Sep 22 20:01:35 eric kernel: power_supply_unregister+0x8e/0xa0
> Sep 22 20:01:35 eric kernel: release_nodes+0x3d/0xb0
> Sep 22 20:01:35 eric kernel: devres_release_group+0xfc/0x130
> Sep 22 20:01:35 eric kernel: hid_device_remove+0x56/0xa0
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> Sep 22 20:01:35 eric kernel: logi_dj_remove+0x9a/0x100 [hid_logitech_dj 5c91534a0ead2b65e04dd799a0437e3b99b21bc4]
> Sep 22 20:01:35 eric kernel: hid_device_remove+0x44/0xa0
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> Sep 22 20:01:35 eric kernel: usbhid_disconnect+0x47/0x60 [usbhid 727dcc1c0b94e6b4418727a468398ac3bca492f3]
> Sep 22 20:01:35 eric kernel: usb_unbind_interface+0x90/0x270
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? kobject_put+0xa0/0x1d0
> Sep 22 20:01:35 eric kernel: usb_disable_device+0xcd/0x1e0
> Sep 22 20:01:35 eric kernel: usb_disconnect+0xde/0x2c0
> Sep 22 20:01:35 eric kernel: usb_disconnect+0xc3/0x2c0
> Sep 22 20:01:35 eric kernel: hub_event+0xe80/0x1c10
>
> There have been quite a few bug reports (see Link tags) about this crash.
>
> Fix all the TOCTOU issues, including the really bad power-supply related
> system crash on USB disconnect, by making probe() use the workqueue for
> running hidpp_connect_event() too, so that it can never run more then once.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227221
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2242189
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412#c58
Many thanks for finding out the root cause of this. And sorry that you
had to do it :(
Out of curiosity, do you have an idea on when this was introduced?
From these logs it seem that the symptoms started to appear in July in
distributions, but I can not quickly reproduce it locally and so I'm a
little bit puzzled.
Anyway, with or without a Fixes tag, I think I'll apply it today and
send this and the other patch from Johan to Linus ASAP.
Cheers,
Benjamin
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ff077df0babf..a209d51bd247 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4515,7 +4515,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto hid_hw_init_fail;
> }
>
> - hidpp_connect_event(hidpp);
> + schedule_work(&hidpp->work);
> + flush_work(&hidpp->work);
>
> if (will_restart) {
> /* Reset the HID node state */
> --
> 2.41.0
>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Hans de Goede @ 2023-10-06 7:59 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <krfqpnfzrubfiweaokbibvz7cfcnd6wi6ub6xazpgfou3uwdds@z6th2ckoqb52>
Hi Benjamin,
On 10/6/23 09:29, Benjamin Tissoires wrote:
> Hi Hans,
>
> On Oct 05 2023, Hans de Goede wrote:
>> hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
>> races when it races with itself.
>>
>> hidpp_connect_event() primarily runs from a workqueue but it also runs
>> on probe() and if a "device-connected" packet is received by the hw
>> when the thread running hidpp_connect_event() from probe() is waiting on
>> the hw, then a second thread running hidpp_connect_event() will be
>> started from the workqueue.
>>
>> This opens the following races (note the below code is simplified):
>>
>> 1. Retrieving + printing the protocol (harmless race):
>>
>> if (!hidpp->protocol_major) {
>> hidpp_root_get_protocol_version()
>> hidpp->protocol_major = response.rap.params[0];
>> }
>>
>> We can actually see this race hit in the dmesg in the abrt output
>> attached to rhbz#2227968:
>>
>> [ 3064.624215] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
>> [ 3064.658184] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
>>
>> Testing with extra logging added has shown that after this the 2 threads
>> take turn grabbing the hw access mutex (send_mutex) so they ping-pong
>> through all the other TOCTOU cases managing to hit all of them:
>>
>> 2. Updating the name to the HIDPP name (harmless race):
>>
>> if (hidpp->name == hdev->name) {
>> ...
>> hidpp->name = new_name;
>> }
>>
>> 3. Initializing the power_supply class for the battery (problematic!):
>>
>> hidpp_initialize_battery()
>> {
>> if (hidpp->battery.ps)
>> return 0;
>>
>> probe_battery(); /* Blocks, threads take turns executing this */
>>
>> hidpp->battery.desc.properties =
>> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>>
>> hidpp->battery.ps =
>> devm_power_supply_register(&hidpp->hid_dev->dev,
>> &hidpp->battery.desc, cfg);
>> }
>>
>> 4. Creating delayed input_device (potentially problematic):
>>
>> if (hidpp->delayed_input)
>> return;
>>
>> hidpp->delayed_input = hidpp_allocate_input(hdev);
>>
>> The really big problem here is 3. Hitting the race leads to the following
>> sequence:
>>
>> hidpp->battery.desc.properties =
>> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>>
>> hidpp->battery.ps =
>> devm_power_supply_register(&hidpp->hid_dev->dev,
>> &hidpp->battery.desc, cfg);
>>
>> ...
>>
>> hidpp->battery.desc.properties =
>> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>>
>> hidpp->battery.ps =
>> devm_power_supply_register(&hidpp->hid_dev->dev,
>> &hidpp->battery.desc, cfg);
>>
>> So now we have registered 2 power supplies for the same battery,
>> which looks a bit weird from userspace's pov but this is not even
>> the really big problem.
>>
>> Notice how:
>>
>> 1. This is all devm-maganaged
>> 2. The hidpp->battery.desc struct is shared between the 2 power supplies
>> 3. hidpp->battery.desc points to the result from the second devm_kmemdup()
>>
>> This causes a use after free scenario on USB disconnect of the receiver:
>> 1. The last registered power supply class device gets unregistered
>> 2. The memory from the last devm_kmemdup() call gets freed,
>> hidpp->battery.desc.properties now points to freed memory
>> 3. The first registered power supply class device gets unregistered,
>> this involves sending a remove uevent to userspace which invokes
>> power_supply_uevent() to fill the uevent data
>> 4. power_supply_uevent() uses hidpp->battery.desc.properties which
>> now points to freed memory leading to backtraces like this one:
>>
>> Sep 22 20:01:35 eric kernel: BUG: unable to handle page fault for address: ffffb2140e017f08
>> ...
>> Sep 22 20:01:35 eric kernel: Workqueue: usb_hub_wq hub_event
>> Sep 22 20:01:35 eric kernel: RIP: 0010:power_supply_uevent+0xee/0x1d0
>> ...
>> Sep 22 20:01:35 eric kernel: ? asm_exc_page_fault+0x26/0x30
>> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0xee/0x1d0
>> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0x10d/0x1d0
>> Sep 22 20:01:35 eric kernel: dev_uevent+0x10f/0x2d0
>> Sep 22 20:01:35 eric kernel: kobject_uevent_env+0x291/0x680
>> Sep 22 20:01:35 eric kernel: power_supply_unregister+0x8e/0xa0
>> Sep 22 20:01:35 eric kernel: release_nodes+0x3d/0xb0
>> Sep 22 20:01:35 eric kernel: devres_release_group+0xfc/0x130
>> Sep 22 20:01:35 eric kernel: hid_device_remove+0x56/0xa0
>> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
>> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
>> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
>> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
>> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
>> Sep 22 20:01:35 eric kernel: logi_dj_remove+0x9a/0x100 [hid_logitech_dj 5c91534a0ead2b65e04dd799a0437e3b99b21bc4]
>> Sep 22 20:01:35 eric kernel: hid_device_remove+0x44/0xa0
>> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
>> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
>> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
>> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
>> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
>> Sep 22 20:01:35 eric kernel: usbhid_disconnect+0x47/0x60 [usbhid 727dcc1c0b94e6b4418727a468398ac3bca492f3]
>> Sep 22 20:01:35 eric kernel: usb_unbind_interface+0x90/0x270
>> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
>> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
>> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
>> Sep 22 20:01:35 eric kernel: ? kobject_put+0xa0/0x1d0
>> Sep 22 20:01:35 eric kernel: usb_disable_device+0xcd/0x1e0
>> Sep 22 20:01:35 eric kernel: usb_disconnect+0xde/0x2c0
>> Sep 22 20:01:35 eric kernel: usb_disconnect+0xc3/0x2c0
>> Sep 22 20:01:35 eric kernel: hub_event+0xe80/0x1c10
>>
>> There have been quite a few bug reports (see Link tags) about this crash.
>>
>> Fix all the TOCTOU issues, including the really bad power-supply related
>> system crash on USB disconnect, by making probe() use the workqueue for
>> running hidpp_connect_event() too, so that it can never run more then once.
>>
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227221
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2242189
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412#c58
>
> Many thanks for finding out the root cause of this. And sorry that you
> had to do it :(
>
> Out of curiosity, do you have an idea on when this was introduced?
> From these logs it seem that the symptoms started to appear in July in
> distributions, but I can not quickly reproduce it locally and so I'm a
> little bit puzzled.
I think this may be triggered more easily after commit 699fb50d99039
from 2023-07-20, but the root cause of this seems to be have been
with us all along ...
> Anyway, with or without a Fixes tag, I think I'll apply it today and
> send this and the other patch from Johan to Linus ASAP.
Note I realized this morning that there is another small race between
probe() and hidpp_connect_event() which should not cause a crahs but which
may cause functional issues..
So I'm preparing a follow up patch. I actually just finished testing
the follow up patch. Please also include that patch (assuming you agree
with the fix from it).
Regards,
Hans
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/hid/hid-logitech-hidpp.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> index ff077df0babf..a209d51bd247 100644
>> --- a/drivers/hid/hid-logitech-hidpp.c
>> +++ b/drivers/hid/hid-logitech-hidpp.c
>> @@ -4515,7 +4515,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> goto hid_hw_init_fail;
>> }
>>
>> - hidpp_connect_event(hidpp);
>> + schedule_work(&hidpp->work);
>> + flush_work(&hidpp->work);
>>
>> if (will_restart) {
>> /* Reset the HID node state */
>> --
>> 2.41.0
>>
>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Benjamin Tissoires @ 2023-10-06 8:09 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <3ebf4abd-ab66-4dfb-7cbb-1248c1f449fd@redhat.com>
On Oct 06 2023, Hans de Goede wrote:
> Hi Benjamin,
>
> On 10/6/23 09:29, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Oct 05 2023, Hans de Goede wrote:
> >> hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
> >> races when it races with itself.
> >>
> >> hidpp_connect_event() primarily runs from a workqueue but it also runs
> >> on probe() and if a "device-connected" packet is received by the hw
> >> when the thread running hidpp_connect_event() from probe() is waiting on
> >> the hw, then a second thread running hidpp_connect_event() will be
> >> started from the workqueue.
> >>
> >> This opens the following races (note the below code is simplified):
> >>
> >> 1. Retrieving + printing the protocol (harmless race):
> >>
> >> if (!hidpp->protocol_major) {
> >> hidpp_root_get_protocol_version()
> >> hidpp->protocol_major = response.rap.params[0];
> >> }
> >>
> >> We can actually see this race hit in the dmesg in the abrt output
> >> attached to rhbz#2227968:
> >>
> >> [ 3064.624215] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> >> [ 3064.658184] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> >>
> >> Testing with extra logging added has shown that after this the 2 threads
> >> take turn grabbing the hw access mutex (send_mutex) so they ping-pong
> >> through all the other TOCTOU cases managing to hit all of them:
> >>
> >> 2. Updating the name to the HIDPP name (harmless race):
> >>
> >> if (hidpp->name == hdev->name) {
> >> ...
> >> hidpp->name = new_name;
> >> }
> >>
> >> 3. Initializing the power_supply class for the battery (problematic!):
> >>
> >> hidpp_initialize_battery()
> >> {
> >> if (hidpp->battery.ps)
> >> return 0;
> >>
> >> probe_battery(); /* Blocks, threads take turns executing this */
> >>
> >> hidpp->battery.desc.properties =
> >> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >>
> >> hidpp->battery.ps =
> >> devm_power_supply_register(&hidpp->hid_dev->dev,
> >> &hidpp->battery.desc, cfg);
> >> }
> >>
> >> 4. Creating delayed input_device (potentially problematic):
> >>
> >> if (hidpp->delayed_input)
> >> return;
> >>
> >> hidpp->delayed_input = hidpp_allocate_input(hdev);
> >>
> >> The really big problem here is 3. Hitting the race leads to the following
> >> sequence:
> >>
> >> hidpp->battery.desc.properties =
> >> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >>
> >> hidpp->battery.ps =
> >> devm_power_supply_register(&hidpp->hid_dev->dev,
> >> &hidpp->battery.desc, cfg);
> >>
> >> ...
> >>
> >> hidpp->battery.desc.properties =
> >> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >>
> >> hidpp->battery.ps =
> >> devm_power_supply_register(&hidpp->hid_dev->dev,
> >> &hidpp->battery.desc, cfg);
> >>
> >> So now we have registered 2 power supplies for the same battery,
> >> which looks a bit weird from userspace's pov but this is not even
> >> the really big problem.
> >>
> >> Notice how:
> >>
> >> 1. This is all devm-maganaged
> >> 2. The hidpp->battery.desc struct is shared between the 2 power supplies
> >> 3. hidpp->battery.desc points to the result from the second devm_kmemdup()
> >>
> >> This causes a use after free scenario on USB disconnect of the receiver:
> >> 1. The last registered power supply class device gets unregistered
> >> 2. The memory from the last devm_kmemdup() call gets freed,
> >> hidpp->battery.desc.properties now points to freed memory
> >> 3. The first registered power supply class device gets unregistered,
> >> this involves sending a remove uevent to userspace which invokes
> >> power_supply_uevent() to fill the uevent data
> >> 4. power_supply_uevent() uses hidpp->battery.desc.properties which
> >> now points to freed memory leading to backtraces like this one:
> >>
> >> Sep 22 20:01:35 eric kernel: BUG: unable to handle page fault for address: ffffb2140e017f08
> >> ...
> >> Sep 22 20:01:35 eric kernel: Workqueue: usb_hub_wq hub_event
> >> Sep 22 20:01:35 eric kernel: RIP: 0010:power_supply_uevent+0xee/0x1d0
> >> ...
> >> Sep 22 20:01:35 eric kernel: ? asm_exc_page_fault+0x26/0x30
> >> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0xee/0x1d0
> >> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0x10d/0x1d0
> >> Sep 22 20:01:35 eric kernel: dev_uevent+0x10f/0x2d0
> >> Sep 22 20:01:35 eric kernel: kobject_uevent_env+0x291/0x680
> >> Sep 22 20:01:35 eric kernel: power_supply_unregister+0x8e/0xa0
> >> Sep 22 20:01:35 eric kernel: release_nodes+0x3d/0xb0
> >> Sep 22 20:01:35 eric kernel: devres_release_group+0xfc/0x130
> >> Sep 22 20:01:35 eric kernel: hid_device_remove+0x56/0xa0
> >> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> >> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> >> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> >> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> >> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> >> Sep 22 20:01:35 eric kernel: logi_dj_remove+0x9a/0x100 [hid_logitech_dj 5c91534a0ead2b65e04dd799a0437e3b99b21bc4]
> >> Sep 22 20:01:35 eric kernel: hid_device_remove+0x44/0xa0
> >> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> >> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> >> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> >> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> >> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> >> Sep 22 20:01:35 eric kernel: usbhid_disconnect+0x47/0x60 [usbhid 727dcc1c0b94e6b4418727a468398ac3bca492f3]
> >> Sep 22 20:01:35 eric kernel: usb_unbind_interface+0x90/0x270
> >> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> >> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> >> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> >> Sep 22 20:01:35 eric kernel: ? kobject_put+0xa0/0x1d0
> >> Sep 22 20:01:35 eric kernel: usb_disable_device+0xcd/0x1e0
> >> Sep 22 20:01:35 eric kernel: usb_disconnect+0xde/0x2c0
> >> Sep 22 20:01:35 eric kernel: usb_disconnect+0xc3/0x2c0
> >> Sep 22 20:01:35 eric kernel: hub_event+0xe80/0x1c10
> >>
> >> There have been quite a few bug reports (see Link tags) about this crash.
> >>
> >> Fix all the TOCTOU issues, including the really bad power-supply related
> >> system crash on USB disconnect, by making probe() use the workqueue for
> >> running hidpp_connect_event() too, so that it can never run more then once.
> >>
> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227221
> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> >> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2242189
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412#c58
> >
> > Many thanks for finding out the root cause of this. And sorry that you
> > had to do it :(
> >
> > Out of curiosity, do you have an idea on when this was introduced?
> > From these logs it seem that the symptoms started to appear in July in
> > distributions, but I can not quickly reproduce it locally and so I'm a
> > little bit puzzled.
>
> I think this may be triggered more easily after commit 699fb50d99039
> from 2023-07-20, but the root cause of this seems to be have been
> with us all along ...
Yeah, that's what I feared :/
>
> > Anyway, with or without a Fixes tag, I think I'll apply it today and
> > send this and the other patch from Johan to Linus ASAP.
>
> Note I realized this morning that there is another small race between
> probe() and hidpp_connect_event() which should not cause a crahs but which
> may cause functional issues..
>
> So I'm preparing a follow up patch. I actually just finished testing
> the follow up patch. Please also include that patch (assuming you agree
> with the fix from it).
Sure. Jiri told me to wait a couple of days before sending the PR to
Linus so the bots came come in and do their jobs. So I will send those
early next week which should give us the time to prepare your other
patch.
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
>
>
>
>
>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> drivers/hid/hid-logitech-hidpp.c | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> >> index ff077df0babf..a209d51bd247 100644
> >> --- a/drivers/hid/hid-logitech-hidpp.c
> >> +++ b/drivers/hid/hid-logitech-hidpp.c
> >> @@ -4515,7 +4515,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >> goto hid_hw_init_fail;
> >> }
> >>
> >> - hidpp_connect_event(hidpp);
> >> + schedule_work(&hidpp->work);
> >> + flush_work(&hidpp->work);
> >>
> >> if (will_restart) {
> >> /* Reset the HID node state */
> >> --
> >> 2.41.0
> >>
> >
>
^ permalink raw reply
* [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 8:18 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input, stable
In-Reply-To: <20231006081858.17677-1-hdegoede@redhat.com>
hidpp_probe() restarts IO after setting things up, if we get a connect
event just before hidpp_probe() stops all IO then hidpp_connect_event()
will see IO errors causing it to fail to setup the connected device.
Add a new io_mutex which hidpp_probe() locks while restarting IO and
which is also taken by hidpp_connect_event() to avoid these 2 things
from racing.
Hopefully this will help with the occasional connect errors leading to
e.g. missing battery monitoring.
Cc: stable@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 35 ++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index a209d51bd247..33f9cd98485a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -181,6 +181,7 @@ struct hidpp_scroll_counter {
struct hidpp_device {
struct hid_device *hid_dev;
struct input_dev *input;
+ struct mutex io_mutex;
struct mutex send_mutex;
void *send_receive_buf;
char *name; /* will never be NULL and should not be freed */
@@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
return;
}
+ /* Avoid probe() restarting IO */
+ mutex_lock(&hidpp->io_mutex);
+
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_connect(hdev, connected);
if (ret)
- return;
+ goto out_unlock;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
ret = m560_send_config_command(hdev, connected);
if (ret)
- return;
+ goto out_unlock;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
ret = k400_connect(hdev, connected);
if (ret)
- return;
+ goto out_unlock;
}
if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
ret = hidpp10_wheel_connect(hidpp);
if (ret)
- return;
+ goto out_unlock;
}
if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS) {
ret = hidpp10_extra_mouse_buttons_connect(hidpp);
if (ret)
- return;
+ goto out_unlock;
}
if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
ret = hidpp10_consumer_keys_connect(hidpp);
if (ret)
- return;
+ goto out_unlock;
}
/* the device is already connected, we can ask for its name and
@@ -4245,7 +4249,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
ret = hidpp_root_get_protocol_version(hidpp);
if (ret) {
hid_err(hdev, "Can not get the protocol version.\n");
- return;
+ goto out_unlock;
}
}
@@ -4256,7 +4260,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
"%s", name);
kfree(name);
if (!devm_name)
- return;
+ goto out_unlock;
hidpp->name = devm_name;
}
@@ -4291,12 +4295,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
/* if the input nodes are already created, we can stop now */
- return;
+ goto out_unlock;
input = hidpp_allocate_input(hdev);
if (!input) {
hid_err(hdev, "cannot allocate new input device: %d\n", ret);
- return;
+ goto out_unlock;
}
hidpp_populate_input(hidpp, input);
@@ -4304,10 +4308,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
ret = input_register_device(input);
if (ret) {
input_free_device(input);
- return;
+ goto out_unlock;
}
hidpp->delayed_input = input;
+out_unlock:
+ mutex_unlock(&hidpp->io_mutex);
}
static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
@@ -4450,6 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
will_restart = true;
INIT_WORK(&hidpp->work, delayed_work_cb);
+ mutex_init(&hidpp->io_mutex);
mutex_init(&hidpp->send_mutex);
init_waitqueue_head(&hidpp->wait);
@@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
flush_work(&hidpp->work);
if (will_restart) {
+ /* Avoid hidpp_connect_event() running while restarting */
+ mutex_lock(&hidpp->io_mutex);
+
/* Reset the HID node state */
hid_device_io_stop(hdev);
hid_hw_close(hdev);
@@ -4529,6 +4539,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/* Now export the actual inputs and hidraw nodes to the world */
ret = hid_hw_start(hdev, connect_mask);
+ mutex_unlock(&hidpp->io_mutex);
if (ret) {
hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
goto hid_hw_start_fail;
@@ -4553,6 +4564,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
+ mutex_destroy(&hidpp->io_mutex);
return ret;
}
@@ -4568,6 +4580,7 @@ static void hidpp_remove(struct hid_device *hdev)
hid_hw_stop(hdev);
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
+ mutex_destroy(&hidpp->io_mutex);
}
#define LDJ_DEVICE(product) \
--
2.41.0
^ permalink raw reply related
* [PATCH 0/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 8:18 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
Hi Benjamin, et al.,
Here are 2 follow-up patches to my logitech-hidpp fix from yesterday:
https://lore.kernel.org/linux-input/20231005182638.3776-1-hdegoede@redhat.com/
The first patch is another race bugfix and should ideally go to stable
assuming you agree,
The second patch is just some code cleanup.
Regards,
Hans
Hans de Goede (2):
HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
restarts IO
HID: logitech-hidpp: Drop delayed_work_cb()
drivers/hid/hid-logitech-hidpp.c | 49 ++++++++++++++++++--------------
1 file changed, 27 insertions(+), 22 deletions(-)
--
2.41.0
^ permalink raw reply
* [PATCH 2/2] HID: logitech-hidpp: Drop delayed_work_cb()
From: Hans de Goede @ 2023-10-06 8:18 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231006081858.17677-1-hdegoede@redhat.com>
Drop delayed_work_cb() instead make hidpp_connect_event() the workqueue
function itself.
Besides resulting in a small cleanup this will hopefully also make
it clearer that going forward hidpp_connect_event() should only
be run from a workqueue and not be directly involved.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 33f9cd98485a..15c36112902b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -236,8 +236,6 @@ struct hidpp_device {
#define HIDPP20_ERROR_UNSUPPORTED 0x09
#define HIDPP20_ERROR 0xff
-static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
-
static int __hidpp_send_report(struct hid_device *hdev,
struct hidpp_report *hidpp_report)
{
@@ -451,13 +449,6 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
return ret;
}
-static void delayed_work_cb(struct work_struct *work)
-{
- struct hidpp_device *hidpp = container_of(work, struct hidpp_device,
- work);
- hidpp_connect_event(hidpp);
-}
-
static inline bool hidpp_match_answer(struct hidpp_report *question,
struct hidpp_report *answer)
{
@@ -4190,8 +4181,9 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
return input_dev;
}
-static void hidpp_connect_event(struct hidpp_device *hidpp)
+static void hidpp_connect_event(struct work_struct *work)
{
+ struct hidpp_device *hidpp = container_of(work, struct hidpp_device, work);
struct hid_device *hdev = hidpp->hid_dev;
int ret = 0;
bool connected = atomic_read(&hidpp->connected);
@@ -4455,7 +4447,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hidpp->quirks & HIDPP_QUIRK_UNIFYING)
will_restart = true;
- INIT_WORK(&hidpp->work, delayed_work_cb);
+ INIT_WORK(&hidpp->work, hidpp_connect_event);
mutex_init(&hidpp->io_mutex);
mutex_init(&hidpp->send_mutex);
init_waitqueue_head(&hidpp->wait);
--
2.41.0
^ permalink raw reply related
* Re: [PATCH 2/2] HID: logitech-hidpp: Drop delayed_work_cb()
From: Hans de Goede @ 2023-10-06 8:21 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input
In-Reply-To: <20231006081858.17677-3-hdegoede@redhat.com>
Hi,
On 10/6/23 10:18, Hans de Goede wrote:
> Drop delayed_work_cb() instead make hidpp_connect_event() the workqueue
> function itself.
>
> Besides resulting in a small cleanup this will hopefully also make
> it clearer that going forward hidpp_connect_event() should only
> be run from a workqueue and not be directly involved.
Typo: s/involved/invoked/
Regards,
Hans
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 33f9cd98485a..15c36112902b 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -236,8 +236,6 @@ struct hidpp_device {
> #define HIDPP20_ERROR_UNSUPPORTED 0x09
> #define HIDPP20_ERROR 0xff
>
> -static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
> -
> static int __hidpp_send_report(struct hid_device *hdev,
> struct hidpp_report *hidpp_report)
> {
> @@ -451,13 +449,6 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
> return ret;
> }
>
> -static void delayed_work_cb(struct work_struct *work)
> -{
> - struct hidpp_device *hidpp = container_of(work, struct hidpp_device,
> - work);
> - hidpp_connect_event(hidpp);
> -}
> -
> static inline bool hidpp_match_answer(struct hidpp_report *question,
> struct hidpp_report *answer)
> {
> @@ -4190,8 +4181,9 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
> return input_dev;
> }
>
> -static void hidpp_connect_event(struct hidpp_device *hidpp)
> +static void hidpp_connect_event(struct work_struct *work)
> {
> + struct hidpp_device *hidpp = container_of(work, struct hidpp_device, work);
> struct hid_device *hdev = hidpp->hid_dev;
> int ret = 0;
> bool connected = atomic_read(&hidpp->connected);
> @@ -4455,7 +4447,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> hidpp->quirks & HIDPP_QUIRK_UNIFYING)
> will_restart = true;
>
> - INIT_WORK(&hidpp->work, delayed_work_cb);
> + INIT_WORK(&hidpp->work, hidpp_connect_event);
> mutex_init(&hidpp->io_mutex);
> mutex_init(&hidpp->send_mutex);
> init_waitqueue_head(&hidpp->wait);
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Hans de Goede @ 2023-10-06 8:50 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input, stable
In-Reply-To: <20231005182638.3776-1-hdegoede@redhat.com>
Hi,
On 10/5/23 20:26, Hans de Goede wrote:
> hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
> races when it races with itself.
>
> hidpp_connect_event() primarily runs from a workqueue but it also runs
> on probe() and if a "device-connected" packet is received by the hw
> when the thread running hidpp_connect_event() from probe() is waiting on
> the hw, then a second thread running hidpp_connect_event() will be
> started from the workqueue.
>
> This opens the following races (note the below code is simplified):
>
> 1. Retrieving + printing the protocol (harmless race):
>
> if (!hidpp->protocol_major) {
> hidpp_root_get_protocol_version()
> hidpp->protocol_major = response.rap.params[0];
> }
>
> We can actually see this race hit in the dmesg in the abrt output
> attached to rhbz#2227968:
>
> [ 3064.624215] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> [ 3064.658184] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
>
> Testing with extra logging added has shown that after this the 2 threads
> take turn grabbing the hw access mutex (send_mutex) so they ping-pong
> through all the other TOCTOU cases managing to hit all of them:
>
> 2. Updating the name to the HIDPP name (harmless race):
>
> if (hidpp->name == hdev->name) {
> ...
> hidpp->name = new_name;
> }
>
> 3. Initializing the power_supply class for the battery (problematic!):
>
> hidpp_initialize_battery()
> {
> if (hidpp->battery.ps)
> return 0;
>
> probe_battery(); /* Blocks, threads take turns executing this */
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
> }
>
> 4. Creating delayed input_device (potentially problematic):
>
> if (hidpp->delayed_input)
> return;
>
> hidpp->delayed_input = hidpp_allocate_input(hdev);
>
> The really big problem here is 3. Hitting the race leads to the following
> sequence:
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
>
> ...
>
> hidpp->battery.desc.properties =
> devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
>
> hidpp->battery.ps =
> devm_power_supply_register(&hidpp->hid_dev->dev,
> &hidpp->battery.desc, cfg);
>
> So now we have registered 2 power supplies for the same battery,
> which looks a bit weird from userspace's pov but this is not even
> the really big problem.
>
> Notice how:
>
> 1. This is all devm-maganaged
> 2. The hidpp->battery.desc struct is shared between the 2 power supplies
> 3. hidpp->battery.desc points to the result from the second devm_kmemdup()
Small error in the commit msg here, please squash in a fix if possible, this
should read:
3. hidpp->battery.desc.properties points to the result from the second devm_kmemdup()
Regards,
Hans
> This causes a use after free scenario on USB disconnect of the receiver:
> 1. The last registered power supply class device gets unregistered
> 2. The memory from the last devm_kmemdup() call gets freed,
> hidpp->battery.desc.properties now points to freed memory
> 3. The first registered power supply class device gets unregistered,
> this involves sending a remove uevent to userspace which invokes
> power_supply_uevent() to fill the uevent data
> 4. power_supply_uevent() uses hidpp->battery.desc.properties which
> now points to freed memory leading to backtraces like this one:
>
> Sep 22 20:01:35 eric kernel: BUG: unable to handle page fault for address: ffffb2140e017f08
> ...
> Sep 22 20:01:35 eric kernel: Workqueue: usb_hub_wq hub_event
> Sep 22 20:01:35 eric kernel: RIP: 0010:power_supply_uevent+0xee/0x1d0
> ...
> Sep 22 20:01:35 eric kernel: ? asm_exc_page_fault+0x26/0x30
> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0xee/0x1d0
> Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0x10d/0x1d0
> Sep 22 20:01:35 eric kernel: dev_uevent+0x10f/0x2d0
> Sep 22 20:01:35 eric kernel: kobject_uevent_env+0x291/0x680
> Sep 22 20:01:35 eric kernel: power_supply_unregister+0x8e/0xa0
> Sep 22 20:01:35 eric kernel: release_nodes+0x3d/0xb0
> Sep 22 20:01:35 eric kernel: devres_release_group+0xfc/0x130
> Sep 22 20:01:35 eric kernel: hid_device_remove+0x56/0xa0
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> Sep 22 20:01:35 eric kernel: logi_dj_remove+0x9a/0x100 [hid_logitech_dj 5c91534a0ead2b65e04dd799a0437e3b99b21bc4]
> Sep 22 20:01:35 eric kernel: hid_device_remove+0x44/0xa0
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> Sep 22 20:01:35 eric kernel: usbhid_disconnect+0x47/0x60 [usbhid 727dcc1c0b94e6b4418727a468398ac3bca492f3]
> Sep 22 20:01:35 eric kernel: usb_unbind_interface+0x90/0x270
> Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> Sep 22 20:01:35 eric kernel: ? kobject_put+0xa0/0x1d0
> Sep 22 20:01:35 eric kernel: usb_disable_device+0xcd/0x1e0
> Sep 22 20:01:35 eric kernel: usb_disconnect+0xde/0x2c0
> Sep 22 20:01:35 eric kernel: usb_disconnect+0xc3/0x2c0
> Sep 22 20:01:35 eric kernel: hub_event+0xe80/0x1c10
>
> There have been quite a few bug reports (see Link tags) about this crash.
>
> Fix all the TOCTOU issues, including the really bad power-supply related
> system crash on USB disconnect, by making probe() use the workqueue for
> running hidpp_connect_event() too, so that it can never run more then once.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227221
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2242189
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412#c58
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ff077df0babf..a209d51bd247 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4515,7 +4515,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> goto hid_hw_init_fail;
> }
>
> - hidpp_connect_event(hidpp);
> + schedule_work(&hidpp->work);
> + flush_work(&hidpp->work);
>
> if (will_restart) {
> /* Reset the HID node state */
^ permalink raw reply
* [PATCH v2] Input: max77693-haptic - add device-tree compatible strings
From: Marek Szyprowski @ 2023-10-06 10:03 UTC (permalink / raw)
To: linux-input, linux-kernel
Cc: Marek Szyprowski, Dmitry Torokhov, Krzysztof Kozlowski
In-Reply-To: <CGME20231006100330eucas1p2c874f582336ed1de4dc1cd759c452ce2@eucas1p2.samsung.com>
Add the needed device-tree compatible strings to the MAX77693 haptic
driver, so it can be automatically loaded when compiled as a kernel
module and given device-tree contains separate (i.e. 'motor-driver') node
under the main PMIC node. When device is instantiated from device-tree,
the driver data cannot be read via platform_get_device_id(), so get
device type from the parent MFD device instead, what works for both
cases.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
drivers/input/misc/max77693-haptic.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/max77693-haptic.c b/drivers/input/misc/max77693-haptic.c
index 80f4416ffe2f..0e646f1b257b 100644
--- a/drivers/input/misc/max77693-haptic.c
+++ b/drivers/input/misc/max77693-haptic.c
@@ -307,7 +307,7 @@ static int max77693_haptic_probe(struct platform_device *pdev)
haptic->suspend_state = false;
/* Variant-specific init */
- haptic->dev_type = platform_get_device_id(pdev)->driver_data;
+ haptic->dev_type = max77693->type;
switch (haptic->dev_type) {
case TYPE_MAX77693:
haptic->regmap_haptic = max77693->regmap_haptic;
@@ -406,16 +406,24 @@ static DEFINE_SIMPLE_DEV_PM_OPS(max77693_haptic_pm_ops,
max77693_haptic_resume);
static const struct platform_device_id max77693_haptic_id[] = {
- { "max77693-haptic", TYPE_MAX77693 },
- { "max77843-haptic", TYPE_MAX77843 },
+ { "max77693-haptic", },
+ { "max77843-haptic", },
{},
};
MODULE_DEVICE_TABLE(platform, max77693_haptic_id);
+static const struct of_device_id of_max77693_haptic_dt_match[] = {
+ { .compatible = "maxim,max77693-haptic", },
+ { .compatible = "maxim,max77843-haptic", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, of_max77693_haptic_dt_match);
+
static struct platform_driver max77693_haptic_driver = {
.driver = {
.name = "max77693-haptic",
.pm = pm_sleep_ptr(&max77693_haptic_pm_ops),
+ .of_match_table = of_max77693_haptic_dt_match,
},
.probe = max77693_haptic_probe,
.id_table = max77693_haptic_id,
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2] Input: max77693-haptic - add device-tree compatible strings
From: Krzysztof Kozlowski @ 2023-10-06 10:04 UTC (permalink / raw)
To: Marek Szyprowski, linux-input, linux-kernel; +Cc: Dmitry Torokhov
In-Reply-To: <20231006100320.2908210-1-m.szyprowski@samsung.com>
On 06/10/2023 12:03, Marek Szyprowski wrote:
> Add the needed device-tree compatible strings to the MAX77693 haptic
> driver, so it can be automatically loaded when compiled as a kernel
> module and given device-tree contains separate (i.e. 'motor-driver') node
> under the main PMIC node. When device is instantiated from device-tree,
> the driver data cannot be read via platform_get_device_id(), so get
> device type from the parent MFD device instead, what works for both
> cases.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/input/misc/max77693-haptic.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH RFT v6 2/6] ARM: pxa: Convert Spitz LEDs to GPIO descriptors
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
Sharp's Spitz board still uses the legacy GPIO interface for configuring
its two onboard LEDs.
Convert them to use the GPIO descriptor interface.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 535e2b2e997b..1fb4102ea39e 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -452,16 +452,25 @@ static inline void spitz_keys_init(void) {}
* LEDs
******************************************************************************/
#if defined(CONFIG_LEDS_GPIO) || defined(CONFIG_LEDS_GPIO_MODULE)
+static struct gpiod_lookup_table spitz_led_gpio_table = {
+ .dev_id = "leds-gpio",
+ .table = {
+ GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_ORANGE, NULL, 0,
+ GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP_IDX("pxa-gpio", SPITZ_GPIO_LED_GREEN, NULL, 1,
+ GPIO_ACTIVE_HIGH),
+ { }
+ }
+};
+
static struct gpio_led spitz_gpio_leds[] = {
{
.name = "spitz:amber:charge",
.default_trigger = "sharpsl-charge",
- .gpio = SPITZ_GPIO_LED_ORANGE,
},
{
.name = "spitz:green:hddactivity",
.default_trigger = "disk-activity",
- .gpio = SPITZ_GPIO_LED_GREEN,
},
};
@@ -480,7 +489,14 @@ static struct platform_device spitz_led_device = {
static void __init spitz_leds_init(void)
{
+ struct gpio_descs *leds;
+
+ gpiod_add_lookup_table(&spitz_led_gpio_table);
platform_device_register(&spitz_led_device);
+ leds = gpiod_get_array_optional(&spitz_led_device.dev,
+ NULL, GPIOD_ASIS);
+ spitz_gpio_leds[0].gpiod = leds->desc[0];
+ spitz_gpio_leds[1].gpiod = leds->desc[1];
}
#else
static inline void spitz_leds_init(void) {}
--
2.42.0
^ permalink raw reply related
* [PATCH RFT v6 0/6] ARM: pxa: GPIO descriptor conversions
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović, Bartosz Golaszewski
Hello,
Small series to convert some of the board files in the mach-pxa directory
to use the new GPIO descriptor interface.
Most notably, the am200epd, am300epd and Spitz matrix keypad among
others are not converted in this series.
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
Changes in v6:
- Address maintainer comments:
- Use devm_gpiod_get_optional() in OHCI
- Use gpiod_get_array() in Spitz LEDs
- Update trailers
- Link to v5: https://lore.kernel.org/r/20231004-pxa-gpio-v5-0-d99ae6fceea8@skole.hr
Changes in v5:
- Address maintainer comments:
- Rename "reset generator" GPIO to "reset"
- Rename ads7846_wait_for_sync() to ads7846_wait_for_sync_gpio()
- Properly bail out when requesting USB host GPIO fails
- Use dev_err_probe() when requesting touchscreen sync GPIO fails
- Use static gpio_desc for gumstix bluetooth reset
- Pulse gumstix bluetooth reset line correctly (assert, then deassert)
- Fix style issue in ads7846_wait_for_sync_gpio()
- Update trailers
- Link to v4: https://lore.kernel.org/r/20231001-pxa-gpio-v4-0-0f3b975e6ed5@skole.hr
Changes in v4:
- Address maintainer comments:
- Move wait_for_sync() from spitz.c to driver
- Register LED platform device before getting its gpiod-s
- Add Linus' Reviewed-by
- Link to v3: https://lore.kernel.org/r/20230929-pxa-gpio-v3-0-af8d5e5d1f34@skole.hr
Changes in v3:
- Address maintainer comments:
- Use GPIO_LOOKUP_IDX for LEDs
- Drop unnecessary NULL assignments
- Don't give up on *all* SPI devices if hsync cannot be set up
- Add Linus' Acked-by
- Link to v2: https://lore.kernel.org/r/20230926-pxa-gpio-v2-0-984464d165dd@skole.hr
Changes in v2:
- Address maintainer comments:
- Change mentions of function to function()
- Drop cast in OHCI driver dev_warn() call
- Use %pe in OHCI and reset drivers
- Use GPIO _optional() API in OHCI driver
- Drop unnecessary not-null check in OHCI driver
- Use pr_err() instead of printk() in reset driver
- Rebase on v6.6-rc3
- Link to v1: https://lore.kernel.org/r/20230924-pxa-gpio-v1-0-2805b87d8894@skole.hr
---
Duje Mihanović (6):
ARM: pxa: Convert Spitz OHCI to GPIO descriptors
ARM: pxa: Convert Spitz LEDs to GPIO descriptors
ARM: pxa: Convert Spitz CF power control to GPIO descriptors
ARM: pxa: Convert reset driver to GPIO descriptors
ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
input: ads7846: Move wait_for_sync() logic to driver
arch/arm/mach-pxa/gumstix.c | 22 ++++++------
arch/arm/mach-pxa/reset.c | 39 +++++++-------------
arch/arm/mach-pxa/reset.h | 3 +-
arch/arm/mach-pxa/spitz.c | 71 +++++++++++++++++++++++++------------
drivers/input/touchscreen/ads7846.c | 22 ++++++++----
drivers/usb/host/ohci-pxa27x.c | 7 ++++
include/linux/spi/ads7846.h | 1 -
7 files changed, 96 insertions(+), 69 deletions(-)
---
base-commit: 8a749fd1a8720d4619c91c8b6e7528c0a355c0aa
change-id: 20230807-pxa-gpio-3ce25d574814
Best regards,
--
Duje Mihanović <duje.mihanovic@skole.hr>
^ permalink raw reply
* [PATCH RFT v6 3/6] ARM: pxa: Convert Spitz CF power control to GPIO descriptors
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović, Bartosz Golaszewski
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
Sharp's Spitz board still uses the legacy GPIO interface for controlling
the power supply to its CF and SD card slots.
Convert it to use the GPIO descriptor interface.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 1fb4102ea39e..8e9301cd0c93 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -133,6 +133,10 @@ static unsigned long spitz_pin_config[] __initdata = {
* Scoop GPIO expander
******************************************************************************/
#if defined(CONFIG_SHARP_SCOOP) || defined(CONFIG_SHARP_SCOOP_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_card_pwr_ctrl_gpio_table, "pxa2xx-mci.0",
+ "sharp-scoop", SPITZ_GPIO_CF_POWER, "cf_power",
+ GPIO_ACTIVE_HIGH);
+
/* SCOOP Device #1 */
static struct resource spitz_scoop_1_resources[] = {
[0] = {
@@ -190,6 +194,7 @@ struct platform_device spitz_scoop_2_device = {
static void __init spitz_scoop_init(void)
{
platform_device_register(&spitz_scoop_1_device);
+ gpiod_add_lookup_table(&spitz_card_pwr_ctrl_gpio_table);
/* Akita doesn't have the second SCOOP chip */
if (!machine_is_akita())
@@ -201,9 +206,18 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
{
unsigned short cpr;
unsigned long flags;
+ struct gpio_desc *cf_power;
+
+ cf_power = gpiod_get(&pxa_device_mci.dev, "cf_power", GPIOD_ASIS);
+ if (IS_ERR(cf_power)) {
+ dev_err(&pxa_device_mci.dev,
+ "failed to get power control GPIO with %ld\n",
+ PTR_ERR(cf_power));
+ return;
+ }
if (new_cpr & 0x7) {
- gpio_set_value(SPITZ_GPIO_CF_POWER, 1);
+ gpiod_direction_output(cf_power, 1);
mdelay(5);
}
@@ -222,8 +236,10 @@ static void __maybe_unused spitz_card_pwr_ctrl(uint8_t enable, uint8_t new_cpr)
if (!(cpr & 0x7)) {
mdelay(1);
- gpio_set_value(SPITZ_GPIO_CF_POWER, 0);
+ gpiod_direction_output(cf_power, 0);
}
+
+ gpiod_put(cf_power);
}
#else
--
2.42.0
^ permalink raw reply related
* [PATCH RFT v6 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
Sharp's Spitz board still uses the legacy GPIO interface for controlling
a GPIO pin related to the USB host controller.
Convert this function to use the new GPIO descriptor interface.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 13 ++++++-------
drivers/usb/host/ohci-pxa27x.c | 7 +++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index cc691b199429..535e2b2e997b 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -649,23 +649,22 @@ static inline void spitz_mmc_init(void) {}
* USB Host
******************************************************************************/
#if defined(CONFIG_USB_OHCI_HCD) || defined(CONFIG_USB_OHCI_HCD_MODULE)
+GPIO_LOOKUP_SINGLE(spitz_usb_host_gpio_table, "pxa27x-ohci", "gpio-pxa",
+ SPITZ_GPIO_USB_HOST, "usb-host", GPIO_ACTIVE_LOW);
+
static int spitz_ohci_init(struct device *dev)
{
- int err;
-
- err = gpio_request(SPITZ_GPIO_USB_HOST, "USB_HOST");
- if (err)
- return err;
+ gpiod_add_lookup_table(&spitz_usb_host_gpio_table);
/* Only Port 2 is connected, setup USB Port 2 Output Control Register */
UP2OCR = UP2OCR_HXS | UP2OCR_HXOE | UP2OCR_DPPDE | UP2OCR_DMPDE;
- return gpio_direction_output(SPITZ_GPIO_USB_HOST, 1);
+ return 0;
}
static void spitz_ohci_exit(struct device *dev)
{
- gpio_free(SPITZ_GPIO_USB_HOST);
+ gpiod_remove_lookup_table(&spitz_usb_host_gpio_table);
}
static struct pxaohci_platform_data spitz_ohci_platform_data = {
diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
index 357d9aee38a3..7f04421c80d6 100644
--- a/drivers/usb/host/ohci-pxa27x.c
+++ b/drivers/usb/host/ohci-pxa27x.c
@@ -121,6 +121,7 @@ struct pxa27x_ohci {
void __iomem *mmio_base;
struct regulator *vbus[3];
bool vbus_enabled[3];
+ struct gpio_desc *usb_host;
};
#define to_pxa27x_ohci(hcd) (struct pxa27x_ohci *)(hcd_to_ohci(hcd)->priv)
@@ -447,6 +448,10 @@ static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
pxa_ohci = to_pxa27x_ohci(hcd);
pxa_ohci->clk = usb_clk;
pxa_ohci->mmio_base = (void __iomem *)hcd->regs;
+ pxa_ohci->usb_host = devm_gpiod_get_optional(&pdev->dev, "usb-host", GPIOD_OUT_LOW);
+ if (IS_ERR(pxa_ohci->usb_host))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pxa_ohci->usb_host),
+ "failed to get USB host GPIO\n");
for (i = 0; i < 3; ++i) {
char name[6];
@@ -512,6 +517,8 @@ static void ohci_hcd_pxa27x_remove(struct platform_device *pdev)
for (i = 0; i < 3; ++i)
pxa27x_ohci_set_vbus_power(pxa_ohci, i, false);
+ gpiod_put(pxa_ohci->usb_host);
+
usb_put_hcd(hcd);
}
--
2.42.0
^ permalink raw reply related
* [PATCH RFT v6 4/6] ARM: pxa: Convert reset driver to GPIO descriptors
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović, Bartosz Golaszewski
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
The PXA reset driver still uses the legacy GPIO interface for
configuring and asserting the reset pin.
Convert it to use the GPIO descriptor interface.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/reset.c | 39 +++++++++++++--------------------------
arch/arm/mach-pxa/reset.h | 3 +--
arch/arm/mach-pxa/spitz.c | 6 +++++-
3 files changed, 19 insertions(+), 29 deletions(-)
diff --git a/arch/arm/mach-pxa/reset.c b/arch/arm/mach-pxa/reset.c
index 27293549f8ad..08bc104b9411 100644
--- a/arch/arm/mach-pxa/reset.c
+++ b/arch/arm/mach-pxa/reset.c
@@ -2,7 +2,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/io.h>
#include <asm/proc-fns.h>
#include <asm/system_misc.h>
@@ -14,33 +14,20 @@
static void do_hw_reset(void);
-static int reset_gpio = -1;
+static struct gpio_desc *reset_gpio;
-int init_gpio_reset(int gpio, int output, int level)
+int init_gpio_reset(int output, int level)
{
- int rc;
-
- rc = gpio_request(gpio, "reset generator");
- if (rc) {
- printk(KERN_ERR "Can't request reset_gpio\n");
- goto out;
+ reset_gpio = gpiod_get(NULL, "reset", GPIOD_ASIS);
+ if (IS_ERR(reset_gpio)) {
+ pr_err("Can't request reset_gpio: %pe\n", reset_gpio);
+ return PTR_ERR(reset_gpio);
}
if (output)
- rc = gpio_direction_output(gpio, level);
+ return gpiod_direction_output(reset_gpio, level);
else
- rc = gpio_direction_input(gpio);
- if (rc) {
- printk(KERN_ERR "Can't configure reset_gpio\n");
- gpio_free(gpio);
- goto out;
- }
-
-out:
- if (!rc)
- reset_gpio = gpio;
-
- return rc;
+ return gpiod_direction_input(reset_gpio);
}
/*
@@ -50,16 +37,16 @@ int init_gpio_reset(int gpio, int output, int level)
*/
static void do_gpio_reset(void)
{
- BUG_ON(reset_gpio == -1);
+ BUG_ON(IS_ERR(reset_gpio));
/* drive it low */
- gpio_direction_output(reset_gpio, 0);
+ gpiod_direction_output(reset_gpio, 0);
mdelay(2);
/* rising edge or drive high */
- gpio_set_value(reset_gpio, 1);
+ gpiod_set_value(reset_gpio, 1);
mdelay(2);
/* falling edge */
- gpio_set_value(reset_gpio, 0);
+ gpiod_set_value(reset_gpio, 0);
/* give it some time */
mdelay(10);
diff --git a/arch/arm/mach-pxa/reset.h b/arch/arm/mach-pxa/reset.h
index 963dd190bc13..5864f61a0e94 100644
--- a/arch/arm/mach-pxa/reset.h
+++ b/arch/arm/mach-pxa/reset.h
@@ -13,10 +13,9 @@ extern void pxa_register_wdt(unsigned int reset_status);
/**
* init_gpio_reset() - register GPIO as reset generator
- * @gpio: gpio nr
* @output: set gpio as output instead of input during normal work
* @level: output level
*/
-extern int init_gpio_reset(int gpio, int output, int level);
+extern int init_gpio_reset(int output, int level);
#endif /* __ASM_ARCH_RESET_H */
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 8e9301cd0c93..554a4b9782c5 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -1026,9 +1026,13 @@ static void spitz_restart(enum reboot_mode mode, const char *cmd)
spitz_poweroff();
}
+GPIO_LOOKUP_SINGLE(spitz_reset_gpio_table, NULL, "pxa-gpio",
+ SPITZ_GPIO_ON_RESET, "reset", GPIO_ACTIVE_HIGH);
+
static void __init spitz_init(void)
{
- init_gpio_reset(SPITZ_GPIO_ON_RESET, 1, 0);
+ gpiod_add_lookup_table(&spitz_reset_gpio_table);
+ init_gpio_reset(1, 0);
pm_power_off = spitz_poweroff;
PMCR = 0x00;
--
2.42.0
^ permalink raw reply related
* [PATCH RFT v6 5/6] ARM: pxa: Convert gumstix Bluetooth to GPIO descriptors
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović, Bartosz Golaszewski
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
Gumstix still uses the legacy GPIO interface for resetting the Bluetooth
device.
Convert it to use the GPIO descriptor interface.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/gumstix.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/arch/arm/mach-pxa/gumstix.c b/arch/arm/mach-pxa/gumstix.c
index c9f0f62187bd..0bca6e2c80a9 100644
--- a/arch/arm/mach-pxa/gumstix.c
+++ b/arch/arm/mach-pxa/gumstix.c
@@ -20,8 +20,8 @@
#include <linux/delay.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
+#include <linux/gpio/consumer.h>
#include <linux/gpio/machine.h>
-#include <linux/gpio.h>
#include <linux/err.h>
#include <linux/clk.h>
@@ -129,6 +129,11 @@ static void gumstix_udc_init(void)
#endif
#ifdef CONFIG_BT
+GPIO_LOOKUP_SINGLE(gumstix_bt_gpio_table, "pxa2xx-uart.1", "pxa-gpio",
+ GPIO_GUMSTIX_BTRESET, "BTRST", GPIO_ACTIVE_LOW);
+
+static struct gpio_desc *bt_reset;
+
/* Normally, the bootloader would have enabled this 32kHz clock but many
** boards still have u-boot 1.1.4 so we check if it has been turned on and
** if not, we turn it on with a warning message. */
@@ -153,24 +158,19 @@ static void gumstix_setup_bt_clock(void)
static void __init gumstix_bluetooth_init(void)
{
- int err;
+ gpiod_add_lookup_table(&gumstix_bt_gpio_table);
gumstix_setup_bt_clock();
- err = gpio_request(GPIO_GUMSTIX_BTRESET, "BTRST");
- if (err) {
+ bt_reset = gpiod_get(&pxa_device_btuart.dev, "BTRST", GPIOD_OUT_HIGH);
+ if (IS_ERR(bt_reset)) {
pr_err("gumstix: failed request gpio for bluetooth reset\n");
return;
}
- err = gpio_direction_output(GPIO_GUMSTIX_BTRESET, 1);
- if (err) {
- pr_err("gumstix: can't reset bluetooth\n");
- return;
- }
- gpio_set_value(GPIO_GUMSTIX_BTRESET, 0);
+ gpiod_set_value(bt_reset, 1);
udelay(100);
- gpio_set_value(GPIO_GUMSTIX_BTRESET, 1);
+ gpiod_set_value(bt_reset, 0);
}
#else
static void gumstix_bluetooth_init(void)
--
2.42.0
^ permalink raw reply related
* [PATCH RFT v6 6/6] input: ads7846: Move wait_for_sync() logic to driver
From: Duje Mihanović @ 2023-10-06 13:44 UTC (permalink / raw)
To: Daniel Mack, Haojian Zhuang, Robert Jarzmik, Russell King,
Alan Stern, Greg Kroah-Hartman, Linus Walleij,
Bartosz Golaszewski, Andy Shevchenko, Dmitry Torokhov, Mark Brown
Cc: linux-arm-kernel, linux-kernel, linux-usb, linux-gpio,
linux-input, linux-spi, Duje Mihanović, Bartosz Golaszewski
In-Reply-To: <20231006-pxa-gpio-v6-0-981b4910d599@skole.hr>
If this code is left in the board file, the sync GPIO would have to be
separated into another lookup table during conversion to the GPIO
descriptor API (which is also done in this patch).
The only user of this code (Sharp Spitz) is also converted in this
patch.
Suggested-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Mark Brown <broonie@kernel.org>
Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Duje Mihanović <duje.mihanovic@skole.hr>
---
arch/arm/mach-pxa/spitz.c | 12 ++----------
drivers/input/touchscreen/ads7846.c | 22 +++++++++++++++-------
include/linux/spi/ads7846.h | 1 -
3 files changed, 17 insertions(+), 18 deletions(-)
diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 554a4b9782c5..1df72d4d6a35 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -522,22 +522,12 @@ static inline void spitz_leds_init(void) {}
* SSP Devices
******************************************************************************/
#if defined(CONFIG_SPI_PXA2XX) || defined(CONFIG_SPI_PXA2XX_MODULE)
-static void spitz_ads7846_wait_for_hsync(void)
-{
- while (gpio_get_value(SPITZ_GPIO_HSYNC))
- cpu_relax();
-
- while (!gpio_get_value(SPITZ_GPIO_HSYNC))
- cpu_relax();
-}
-
static struct ads7846_platform_data spitz_ads7846_info = {
.model = 7846,
.vref_delay_usecs = 100,
.x_plate_ohms = 419,
.y_plate_ohms = 486,
.pressure_max = 1024,
- .wait_for_sync = spitz_ads7846_wait_for_hsync,
};
static struct gpiod_lookup_table spitz_ads7846_gpio_table = {
@@ -545,6 +535,8 @@ static struct gpiod_lookup_table spitz_ads7846_gpio_table = {
.table = {
GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_TP_INT,
"pendown", GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP("gpio-pxa", SPITZ_GPIO_HSYNC,
+ "sync", GPIO_ACTIVE_LOW),
{ }
},
};
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index faea40dd66d0..139b0f3735d0 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -138,8 +138,7 @@ struct ads7846 {
void *filter_data;
int (*get_pendown_state)(void);
struct gpio_desc *gpio_pendown;
-
- void (*wait_for_sync)(void);
+ struct gpio_desc *sync;
};
enum ads7846_filter {
@@ -636,9 +635,15 @@ static const struct attribute_group ads784x_attr_group = {
};
/*--------------------------------------------------------------------------*/
-
-static void null_wait_for_sync(void)
+static void ads7846_wait_for_sync_gpio(struct ads7846 *ts)
{
+ if (!ts->sync)
+ return;
+ while (!gpiod_get_value(ts->sync))
+ cpu_relax();
+
+ while (gpiod_get_value(ts->sync))
+ cpu_relax();
}
static int ads7846_debounce_filter(void *ads, int data_idx, int *val)
@@ -803,7 +808,7 @@ static void ads7846_read_state(struct ads7846 *ts)
packet->last_cmd_idx = 0;
while (true) {
- ts->wait_for_sync();
+ ads7846_wait_for_sync_gpio(ts);
m = &ts->msg[msg_idx];
error = spi_sync(ts->spi, m);
@@ -1261,8 +1266,6 @@ static int ads7846_probe(struct spi_device *spi)
ts->penirq_recheck_delay_usecs =
pdata->penirq_recheck_delay_usecs;
- ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync;
-
snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(dev));
snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model);
@@ -1361,6 +1364,11 @@ static int ads7846_probe(struct spi_device *spi)
if (err)
return err;
+ ts->sync = devm_gpiod_get_optional(dev, "sync", GPIOD_IN);
+ if (IS_ERR(ts->sync))
+ return dev_err_probe(dev, PTR_ERR(ts->sync),
+ "Failed to get sync GPIO");
+
err = input_register_device(input_dev);
if (err)
return err;
diff --git a/include/linux/spi/ads7846.h b/include/linux/spi/ads7846.h
index a04c1c34c344..fa7c4f119023 100644
--- a/include/linux/spi/ads7846.h
+++ b/include/linux/spi/ads7846.h
@@ -38,7 +38,6 @@ struct ads7846_platform_data {
int gpio_pendown_debounce; /* platform specific debounce time for
* the gpio_pendown */
int (*get_pendown_state)(void);
- void (*wait_for_sync)(void);
bool wakeup;
unsigned long irq_flags;
};
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Benjamin Tissoires @ 2023-10-06 13:46 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <20231006081858.17677-2-hdegoede@redhat.com>
Hi Hans,
On Oct 06 2023, Hans de Goede wrote:
> hidpp_probe() restarts IO after setting things up, if we get a connect
> event just before hidpp_probe() stops all IO then hidpp_connect_event()
> will see IO errors causing it to fail to setup the connected device.
I think I see why you are doing this, but it scares me to be honest.
>
> Add a new io_mutex which hidpp_probe() locks while restarting IO and
> which is also taken by hidpp_connect_event() to avoid these 2 things
> from racing.
So now we are adding a new mutex to prevent a workqueue to be executed,
which is held while there is another semaphore going down/up...
It feels error prone to say the least and I'm not sure we are not
actually fixing the problem.
My guts tells me that the issue is tackled at the wrong time, and that
maybe there is a better solution that doesn't involve a new lock in the
middle of 2 other locks being actually held...
One minor comment in the code.
>
> Hopefully this will help with the occasional connect errors leading to
> e.g. missing battery monitoring.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 35 ++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index a209d51bd247..33f9cd98485a 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -181,6 +181,7 @@ struct hidpp_scroll_counter {
> struct hidpp_device {
> struct hid_device *hid_dev;
> struct input_dev *input;
> + struct mutex io_mutex;
> struct mutex send_mutex;
> void *send_receive_buf;
> char *name; /* will never be NULL and should not be freed */
> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> return;
> }
>
> + /* Avoid probe() restarting IO */
> + mutex_lock(&hidpp->io_mutex);
I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
path and forcing any caller to `hidpp_connect_event()` (which will
eventually only be the work struct) to take the lock.
This should simplify the patch by a lot and also ensure someone doesn't
forget the `goto out_unlock`.
> +
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> ret = wtp_connect(hdev, connected);
> if (ret)
> - return;
> + goto out_unlock;
> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
> ret = m560_send_config_command(hdev, connected);
> if (ret)
> - return;
> + goto out_unlock;
> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
> ret = k400_connect(hdev, connected);
> if (ret)
> - return;
> + goto out_unlock;
> }
>
> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
> ret = hidpp10_wheel_connect(hidpp);
> if (ret)
> - return;
> + goto out_unlock;
> }
>
> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS) {
> ret = hidpp10_extra_mouse_buttons_connect(hidpp);
> if (ret)
> - return;
> + goto out_unlock;
> }
>
> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
> ret = hidpp10_consumer_keys_connect(hidpp);
> if (ret)
> - return;
> + goto out_unlock;
> }
>
> /* the device is already connected, we can ask for its name and
> @@ -4245,7 +4249,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> ret = hidpp_root_get_protocol_version(hidpp);
> if (ret) {
> hid_err(hdev, "Can not get the protocol version.\n");
> - return;
> + goto out_unlock;
> }
> }
>
> @@ -4256,7 +4260,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> "%s", name);
> kfree(name);
> if (!devm_name)
> - return;
> + goto out_unlock;
>
> hidpp->name = devm_name;
> }
> @@ -4291,12 +4295,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>
> if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
> /* if the input nodes are already created, we can stop now */
> - return;
> + goto out_unlock;
>
> input = hidpp_allocate_input(hdev);
> if (!input) {
> hid_err(hdev, "cannot allocate new input device: %d\n", ret);
> - return;
> + goto out_unlock;
> }
>
> hidpp_populate_input(hidpp, input);
> @@ -4304,10 +4308,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> ret = input_register_device(input);
> if (ret) {
> input_free_device(input);
> - return;
> + goto out_unlock;
> }
>
> hidpp->delayed_input = input;
> +out_unlock:
> + mutex_unlock(&hidpp->io_mutex);
> }
>
> static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
> @@ -4450,6 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> will_restart = true;
>
> INIT_WORK(&hidpp->work, delayed_work_cb);
> + mutex_init(&hidpp->io_mutex);
> mutex_init(&hidpp->send_mutex);
> init_waitqueue_head(&hidpp->wait);
>
> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> flush_work(&hidpp->work);
>
> if (will_restart) {
> + /* Avoid hidpp_connect_event() running while restarting */
> + mutex_lock(&hidpp->io_mutex);
> +
> /* Reset the HID node state */
> hid_device_io_stop(hdev);
That's the part that makes me raise an eyebrow. Because we lock, then
release the semaphore to get it back later. Can this induce a dead lock?
Can't we solve that same scenario without a mutex, but forcing either
the workqueue to not run or to be finished at this point?
> hid_hw_close(hdev);
> @@ -4529,6 +4539,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> /* Now export the actual inputs and hidraw nodes to the world */
> ret = hid_hw_start(hdev, connect_mask);
> + mutex_unlock(&hidpp->io_mutex);
> if (ret) {
> hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
> goto hid_hw_start_fail;
> @@ -4553,6 +4564,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> + mutex_destroy(&hidpp->io_mutex);
> return ret;
> }
>
> @@ -4568,6 +4580,7 @@ static void hidpp_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> + mutex_destroy(&hidpp->io_mutex);
> }
>
> #define LDJ_DEVICE(product) \
> --
> 2.41.0
>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH v2] HID: i2c-hid: fix handling of unpopulated devices
From: Benjamin Tissoires @ 2023-10-06 14:12 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Johan Hovold
Cc: linux-input, linux-kernel, Douglas Anderson, Maxime Ripard
In-Reply-To: <20231002155857.24584-1-johan+linaro@kernel.org>
On Mon, 02 Oct 2023 17:58:57 +0200, Johan Hovold wrote:
> A recent commit reordered probe so that the interrupt line is now
> requested before making sure that the device exists.
>
> This breaks machines like the Lenovo ThinkPad X13s which rely on the
> HID driver to probe second-source devices and only register the variant
> that is actually populated. Specifically, the interrupt line may now
> already be (temporarily) claimed when doing asynchronous probing of the
> touchpad:
>
> [...]
Applied to hid/hid.git (for-6.6/upstream-fixes), thanks!
[1/1] HID: i2c-hid: fix handling of unpopulated devices
https://git.kernel.org/hid/hid/c/9af867c05b5d
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Benjamin Tissoires @ 2023-10-06 14:12 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, Hans de Goede
Cc: linux-input, stable
In-Reply-To: <20231005182638.3776-1-hdegoede@redhat.com>
On Thu, 05 Oct 2023 20:26:38 +0200, Hans de Goede wrote:
> hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
> races when it races with itself.
>
> hidpp_connect_event() primarily runs from a workqueue but it also runs
> on probe() and if a "device-connected" packet is received by the hw
> when the thread running hidpp_connect_event() from probe() is waiting on
> the hw, then a second thread running hidpp_connect_event() will be
> started from the workqueue.
>
> [...]
Applied to hid/hid.git (for-6.6/upstream-fixes), thanks!
[1/1] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
https://git.kernel.org/hid/hid/c/dac501397b9d
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Fix kernel crash on receiver USB disconnect
From: Benjamin Tissoires @ 2023-10-06 14:15 UTC (permalink / raw)
To: Hans de Goede
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <0509ed50-571e-fa6e-a323-c73abbb938c7@redhat.com>
On Oct 06 2023, Hans de Goede wrote:
> Hi,
>
> On 10/5/23 20:26, Hans de Goede wrote:
> > hidpp_connect_event() has *four* time-of-check vs time-of-use (TOCTOU)
> > races when it races with itself.
> >
> > hidpp_connect_event() primarily runs from a workqueue but it also runs
> > on probe() and if a "device-connected" packet is received by the hw
> > when the thread running hidpp_connect_event() from probe() is waiting on
> > the hw, then a second thread running hidpp_connect_event() will be
> > started from the workqueue.
> >
> > This opens the following races (note the below code is simplified):
> >
> > 1. Retrieving + printing the protocol (harmless race):
> >
> > if (!hidpp->protocol_major) {
> > hidpp_root_get_protocol_version()
> > hidpp->protocol_major = response.rap.params[0];
> > }
> >
> > We can actually see this race hit in the dmesg in the abrt output
> > attached to rhbz#2227968:
> >
> > [ 3064.624215] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> > [ 3064.658184] logitech-hidpp-device 0003:046D:4071.0049: HID++ 4.5 device connected.
> >
> > Testing with extra logging added has shown that after this the 2 threads
> > take turn grabbing the hw access mutex (send_mutex) so they ping-pong
> > through all the other TOCTOU cases managing to hit all of them:
> >
> > 2. Updating the name to the HIDPP name (harmless race):
> >
> > if (hidpp->name == hdev->name) {
> > ...
> > hidpp->name = new_name;
> > }
> >
> > 3. Initializing the power_supply class for the battery (problematic!):
> >
> > hidpp_initialize_battery()
> > {
> > if (hidpp->battery.ps)
> > return 0;
> >
> > probe_battery(); /* Blocks, threads take turns executing this */
> >
> > hidpp->battery.desc.properties =
> > devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >
> > hidpp->battery.ps =
> > devm_power_supply_register(&hidpp->hid_dev->dev,
> > &hidpp->battery.desc, cfg);
> > }
> >
> > 4. Creating delayed input_device (potentially problematic):
> >
> > if (hidpp->delayed_input)
> > return;
> >
> > hidpp->delayed_input = hidpp_allocate_input(hdev);
> >
> > The really big problem here is 3. Hitting the race leads to the following
> > sequence:
> >
> > hidpp->battery.desc.properties =
> > devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >
> > hidpp->battery.ps =
> > devm_power_supply_register(&hidpp->hid_dev->dev,
> > &hidpp->battery.desc, cfg);
> >
> > ...
> >
> > hidpp->battery.desc.properties =
> > devm_kmemdup(dev, hidpp_battery_props, cnt, GFP_KERNEL);
> >
> > hidpp->battery.ps =
> > devm_power_supply_register(&hidpp->hid_dev->dev,
> > &hidpp->battery.desc, cfg);
> >
> > So now we have registered 2 power supplies for the same battery,
> > which looks a bit weird from userspace's pov but this is not even
> > the really big problem.
> >
> > Notice how:
> >
> > 1. This is all devm-maganaged
> > 2. The hidpp->battery.desc struct is shared between the 2 power supplies
> > 3. hidpp->battery.desc points to the result from the second devm_kmemdup()
>
> Small error in the commit msg here, please squash in a fix if possible, this
> should read:
>
> 3. hidpp->battery.desc.properties points to the result from the second devm_kmemdup()
Done (hopefully). Please shout if you disagree :)
Cheers,
Benjamin
>
>
> Regards,
>
> Hans
>
>
>
>
>
>
> > This causes a use after free scenario on USB disconnect of the receiver:
> > 1. The last registered power supply class device gets unregistered
> > 2. The memory from the last devm_kmemdup() call gets freed,
> > hidpp->battery.desc.properties now points to freed memory
> > 3. The first registered power supply class device gets unregistered,
> > this involves sending a remove uevent to userspace which invokes
> > power_supply_uevent() to fill the uevent data
> > 4. power_supply_uevent() uses hidpp->battery.desc.properties which
> > now points to freed memory leading to backtraces like this one:
> >
> > Sep 22 20:01:35 eric kernel: BUG: unable to handle page fault for address: ffffb2140e017f08
> > ...
> > Sep 22 20:01:35 eric kernel: Workqueue: usb_hub_wq hub_event
> > Sep 22 20:01:35 eric kernel: RIP: 0010:power_supply_uevent+0xee/0x1d0
> > ...
> > Sep 22 20:01:35 eric kernel: ? asm_exc_page_fault+0x26/0x30
> > Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0xee/0x1d0
> > Sep 22 20:01:35 eric kernel: ? power_supply_uevent+0x10d/0x1d0
> > Sep 22 20:01:35 eric kernel: dev_uevent+0x10f/0x2d0
> > Sep 22 20:01:35 eric kernel: kobject_uevent_env+0x291/0x680
> > Sep 22 20:01:35 eric kernel: power_supply_unregister+0x8e/0xa0
> > Sep 22 20:01:35 eric kernel: release_nodes+0x3d/0xb0
> > Sep 22 20:01:35 eric kernel: devres_release_group+0xfc/0x130
> > Sep 22 20:01:35 eric kernel: hid_device_remove+0x56/0xa0
> > Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> > Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> > Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> > Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> > Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> > Sep 22 20:01:35 eric kernel: logi_dj_remove+0x9a/0x100 [hid_logitech_dj 5c91534a0ead2b65e04dd799a0437e3b99b21bc4]
> > Sep 22 20:01:35 eric kernel: hid_device_remove+0x44/0xa0
> > Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> > Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> > Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> > Sep 22 20:01:35 eric kernel: ? __queue_work+0x1df/0x440
> > Sep 22 20:01:35 eric kernel: hid_destroy_device+0x4b/0x60
> > Sep 22 20:01:35 eric kernel: usbhid_disconnect+0x47/0x60 [usbhid 727dcc1c0b94e6b4418727a468398ac3bca492f3]
> > Sep 22 20:01:35 eric kernel: usb_unbind_interface+0x90/0x270
> > Sep 22 20:01:35 eric kernel: device_release_driver_internal+0x19f/0x200
> > Sep 22 20:01:35 eric kernel: bus_remove_device+0xc6/0x130
> > Sep 22 20:01:35 eric kernel: device_del+0x15c/0x3f0
> > Sep 22 20:01:35 eric kernel: ? kobject_put+0xa0/0x1d0
> > Sep 22 20:01:35 eric kernel: usb_disable_device+0xcd/0x1e0
> > Sep 22 20:01:35 eric kernel: usb_disconnect+0xde/0x2c0
> > Sep 22 20:01:35 eric kernel: usb_disconnect+0xc3/0x2c0
> > Sep 22 20:01:35 eric kernel: hub_event+0xe80/0x1c10
> >
> > There have been quite a few bug reports (see Link tags) about this crash.
> >
> > Fix all the TOCTOU issues, including the really bad power-supply related
> > system crash on USB disconnect, by making probe() use the workqueue for
> > running hidpp_connect_event() too, so that it can never run more then once.
> >
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227221
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2227968
> > Link: https://bugzilla.redhat.com/show_bug.cgi?id=2242189
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412#c58
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index ff077df0babf..a209d51bd247 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -4515,7 +4515,8 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > goto hid_hw_init_fail;
> > }
> >
> > - hidpp_connect_event(hidpp);
> > + schedule_work(&hidpp->work);
> > + flush_work(&hidpp->work);
> >
> > if (will_restart) {
> > /* Reset the HID node state */
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-06 15:11 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, linux-input, stable
In-Reply-To: <iqchunho27bqb6dp24ptfx32gdwbq6f6v654ftfme4kel3hoa6@5t2x4kcms2wk>
Hi Benjamin,
On 10/6/23 15:46, Benjamin Tissoires wrote:
> Hi Hans,
>
> On Oct 06 2023, Hans de Goede wrote:
>> hidpp_probe() restarts IO after setting things up, if we get a connect
>> event just before hidpp_probe() stops all IO then hidpp_connect_event()
>> will see IO errors causing it to fail to setup the connected device.
>
> I think I see why you are doing this, but it scares me to be honest.
>
>>
>> Add a new io_mutex which hidpp_probe() locks while restarting IO and
>> which is also taken by hidpp_connect_event() to avoid these 2 things
>> from racing.
>
> So now we are adding a new mutex to prevent a workqueue to be executed,
> which is held while there is another semaphore going down/up...
> It feels error prone to say the least and I'm not sure we are not
> actually fixing the problem.
>
> My guts tells me that the issue is tackled at the wrong time, and that
> maybe there is a better solution that doesn't involve a new lock in the
> middle of 2 other locks being actually held...
Since the lock is only taken into 2 places and 1 of them is holding
no locks when taking it (because workqueue) I don't really see how
this would be a problem.
Actually introducing a new lock for this, rather then say trying
to use the send_mutex makes this much easier to reason about,
more on this below.
> One minor comment in the code.
>
>>
>> Hopefully this will help with the occasional connect errors leading to
>> e.g. missing battery monitoring.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> drivers/hid/hid-logitech-hidpp.c | 35 ++++++++++++++++++++++----------
>> 1 file changed, 24 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> index a209d51bd247..33f9cd98485a 100644
>> --- a/drivers/hid/hid-logitech-hidpp.c
>> +++ b/drivers/hid/hid-logitech-hidpp.c
>> @@ -181,6 +181,7 @@ struct hidpp_scroll_counter {
>> struct hidpp_device {
>> struct hid_device *hid_dev;
>> struct input_dev *input;
>> + struct mutex io_mutex;
>> struct mutex send_mutex;
>> void *send_receive_buf;
>> char *name; /* will never be NULL and should not be freed */
>> @@ -4207,36 +4208,39 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> return;
>> }
>>
>> + /* Avoid probe() restarting IO */
>> + mutex_lock(&hidpp->io_mutex);
>
> I'd put a `__must_hold(&hidpp->io_mutex);` here, not changing any return
> path and forcing any caller to `hidpp_connect_event()` (which will
> eventually only be the work struct) to take the lock.
>
> This should simplify the patch by a lot and also ensure someone doesn't
> forget the `goto out_unlock`.
Ok, I can add the __must_hold() here and make
delayed_Work_cb take the lock, but that would make it
impossible to implement patch 2/2 in a clean manner and
I do like patch 2/2 since it makes it clear that
hidpp_connect_event must only run from the workqueue
but I guess we could just add a comment for that
instead.
Either way works for me, with a slight preference
for the current version even if it introduces
a bunch of gotos.
>
>> +
>> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>> ret = wtp_connect(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
>> ret = m560_send_config_command(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
>> ret = k400_connect(hdev, connected);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_WHEELS) {
>> ret = hidpp10_wheel_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_EXTRA_MOUSE_BTNS) {
>> ret = hidpp10_extra_mouse_buttons_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> if (hidpp->quirks & HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS) {
>> ret = hidpp10_consumer_keys_connect(hidpp);
>> if (ret)
>> - return;
>> + goto out_unlock;
>> }
>>
>> /* the device is already connected, we can ask for its name and
>> @@ -4245,7 +4249,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> ret = hidpp_root_get_protocol_version(hidpp);
>> if (ret) {
>> hid_err(hdev, "Can not get the protocol version.\n");
>> - return;
>> + goto out_unlock;
>> }
>> }
>>
>> @@ -4256,7 +4260,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> "%s", name);
>> kfree(name);
>> if (!devm_name)
>> - return;
>> + goto out_unlock;
>>
>> hidpp->name = devm_name;
>> }
>> @@ -4291,12 +4295,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>>
>> if (!(hidpp->quirks & HIDPP_QUIRK_DELAYED_INIT) || hidpp->delayed_input)
>> /* if the input nodes are already created, we can stop now */
>> - return;
>> + goto out_unlock;
>>
>> input = hidpp_allocate_input(hdev);
>> if (!input) {
>> hid_err(hdev, "cannot allocate new input device: %d\n", ret);
>> - return;
>> + goto out_unlock;
>> }
>>
>> hidpp_populate_input(hidpp, input);
>> @@ -4304,10 +4308,12 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
>> ret = input_register_device(input);
>> if (ret) {
>> input_free_device(input);
>> - return;
>> + goto out_unlock;
>> }
>>
>> hidpp->delayed_input = input;
>> +out_unlock:
>> + mutex_unlock(&hidpp->io_mutex);
>> }
>>
>> static DEVICE_ATTR(builtin_power_supply, 0000, NULL, NULL);
>> @@ -4450,6 +4456,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> will_restart = true;
>>
>> INIT_WORK(&hidpp->work, delayed_work_cb);
>> + mutex_init(&hidpp->io_mutex);
>> mutex_init(&hidpp->send_mutex);
>> init_waitqueue_head(&hidpp->wait);
>>
>> @@ -4519,6 +4526,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> flush_work(&hidpp->work);
>>
>> if (will_restart) {
>> + /* Avoid hidpp_connect_event() running while restarting */
>> + mutex_lock(&hidpp->io_mutex);
>> +
>> /* Reset the HID node state */
>> hid_device_io_stop(hdev);
>
> That's the part that makes me raise an eyebrow. Because we lock, then
> release the semaphore to get it back later. Can this induce a dead lock?
>
> Can't we solve that same scenario without a mutex, but forcing either
> the workqueue to not run or to be finished at this point?
I'm not sure what you are worried about after the mutex_lock
the line above we are 100% guaranteed that hidpp_connect_event()
is not running and since it is not running it will also not
be holding any other locks, so it can not cause any problems.
The other way around if hidpp_connect_event() is running
the mutex_lock() above ensures that it will finishes and
release any locks before we get here.
There is no way that both code paths can run at the same time
with the new lock. And there thus also is no way that they
can cause any new not already held locks to be taken while
the other side is running.
I actually introduced the new lock because in my mind
introducing the new lock allows to easily reason about
the impact on other locking (which is none).
I hope this helps explain. As for the making
hidpp_connect_event()'s caller take the lock thing, let me
know how you want to resolve that. Either way works for me
and I guess the less intrusive version of making the caller
take the lock is easier to backport so we should probably
go that route.
Regards,
Hans
>> hid_hw_close(hdev);
>> @@ -4529,6 +4539,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>
>> /* Now export the actual inputs and hidraw nodes to the world */
>> ret = hid_hw_start(hdev, connect_mask);
>> + mutex_unlock(&hidpp->io_mutex);
>> if (ret) {
>> hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
>> goto hid_hw_start_fail;
>> @@ -4553,6 +4564,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>> cancel_work_sync(&hidpp->work);
>> mutex_destroy(&hidpp->send_mutex);
>> + mutex_destroy(&hidpp->io_mutex);
>> return ret;
>> }
>>
>> @@ -4568,6 +4580,7 @@ static void hidpp_remove(struct hid_device *hdev)
>> hid_hw_stop(hdev);
>> cancel_work_sync(&hidpp->work);
>> mutex_destroy(&hidpp->send_mutex);
>> + mutex_destroy(&hidpp->io_mutex);
>> }
>>
>> #define LDJ_DEVICE(product) \
>> --
>> 2.41.0
>>
>
> Cheers,
> Benjamin
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox