* [PATCH 1/2] HID: picolcd: Move output requests out of spinlocked sections
2026-09-28 11:46 [PATCH 0/2] HID: picolcd: Fix output-request and framebuffer locking Aveline Noir
@ 2026-09-28 11:46 ` Aveline Noir
2026-09-28 12:28 ` Sebastian Andrzej Siewior
2026-09-28 11:46 ` [PATCH 2/2] HID: picolcd: Avoid framebuffer last-close deadlock Aveline Noir
1 sibling, 1 reply; 5+ messages in thread
From: Aveline Noir @ 2026-09-28 11:46 UTC (permalink / raw)
To: Bruno Prémont, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, syzbot+912222e4cb82423535fa, bigeasy,
linux-rt-devel
Creating a PicoLCD through UHID triggers a sleeping-in-invalid-context
warning in picolcd_set_contrast() on PREEMPT_RT. hid_hw_request() can
allocate with GFP_KERNEL and wait for a reply through __hid_request(),
but the driver invokes it while holding data->lock. The same pattern
exists in the other output paths.
Serialize output report updates and requests with a separate mutex.
Keep the spinlock for status and pending replies shared with raw_event(),
and drop it before submitting requests. Serialize the failed-state
transition with output requests during removal.
Use the blocking LED callback and protect LED state updates with the
report mutex. Move framebuffer reset outside fbdata->lock so the new
mutex is never acquired under that spinlock.
In a PREEMPT_RT QEMU guest, the original syzkaller reproducer triggered
six sleep warnings with the baseline module and none with this change
during a four-second run (214 iterations). Five rounds each of concurrent
LCD, backlight, two LED and UHID destroy operations, with and without
persistent-open framebuffer writes, completed without BUG/WARNING.
Physical hardware and suspend/resume have not been tested.
Fixes: d881427253da ("HID: use hid_hw_request() instead of direct call
to usbhid")
Reported-by: syzbot+912222e4cb82423535fa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=912222e4cb82423535fa
Signed-off-by: Aveline Noir <jm5905938@gmail.com>
---
drivers/hid/hid-picolcd.h | 2 ++
drivers/hid/hid-picolcd_backlight.c | 7 ++---
drivers/hid/hid-picolcd_core.c | 46 +++++++++++++++++++----------
drivers/hid/hid-picolcd_fb.c | 23 ++++++++-------
drivers/hid/hid-picolcd_lcd.c | 7 ++---
drivers/hid/hid-picolcd_leds.c | 31 +++++++++++--------
6 files changed, 69 insertions(+), 47 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 57c9d0a675..846a8ceb95 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -102,6 +102,8 @@ struct picolcd_data {
/* Housekeeping stuff */
spinlock_t lock;
struct mutex mutex;
+ /* Serialize updates to output reports and their HID requests. */
+ struct mutex report_mutex;
struct picolcd_pending *pending;
int status;
#define PICOLCD_BOOTLOADER 1
diff --git a/drivers/hid/hid-picolcd_backlight.c
b/drivers/hid/hid-picolcd_backlight.c
index 4b43b64537..9fe27437d2 100644
--- a/drivers/hid/hid-picolcd_backlight.c
+++ b/drivers/hid/hid-picolcd_backlight.c
@@ -23,19 +23,18 @@ static int picolcd_set_brightness(struct
backlight_device *bdev)
{
struct picolcd_data *data = bl_get_data(bdev);
struct hid_report *report = picolcd_out_report(REPORT_BRIGHTNESS, data->hdev);
- unsigned long flags;
if (!report || report->maxfield != 1 || report->field[0]->report_count != 1)
return -ENODEV;
+ mutex_lock(&data->report_mutex);
data->lcd_brightness = bdev->props.brightness & 0x0ff;
data->lcd_power = bdev->props.power;
- spin_lock_irqsave(&data->lock, flags);
hid_set_field(report->field[0], 0,
data->lcd_power == BACKLIGHT_POWER_ON ? data->lcd_brightness : 0);
- if (!(data->status & PICOLCD_FAILED))
+ if (!(READ_ONCE(data->status) & PICOLCD_FAILED))
hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
return 0;
}
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index d73e97c8b8..9d6bf75aba 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -77,7 +77,7 @@ struct picolcd_pending *picolcd_send_and_wait(struct
hid_device *hdev,
if (!report || !data)
return NULL;
- if (data->status & PICOLCD_FAILED)
+ if (READ_ONCE(data->status) & PICOLCD_FAILED)
return NULL;
work = kzalloc_obj(*work);
if (!work)
@@ -89,23 +89,27 @@ struct picolcd_pending
*picolcd_send_and_wait(struct hid_device *hdev,
work->raw_size = 0;
mutex_lock(&data->mutex);
- spin_lock_irqsave(&data->lock, flags);
+ mutex_lock(&data->report_mutex);
for (i = k = 0; i < report->maxfield; i++)
for (j = 0; j < report->field[i]->report_count; j++) {
hid_set_field(report->field[i], j, k < size ? raw_data[k] : 0);
k++;
}
+ spin_lock_irqsave(&data->lock, flags);
if (data->status & PICOLCD_FAILED) {
- kfree(work);
- work = NULL;
- } else {
- data->pending = work;
- hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
spin_unlock_irqrestore(&data->lock, flags);
- wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
- spin_lock_irqsave(&data->lock, flags);
- data->pending = NULL;
+ mutex_unlock(&data->report_mutex);
+ mutex_unlock(&data->mutex);
+ kfree(work);
+ return NULL;
}
+ data->pending = work;
+ spin_unlock_irqrestore(&data->lock, flags);
+ hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
+ mutex_unlock(&data->report_mutex);
+ wait_for_completion_interruptible_timeout(&work->ready, HZ*2);
+ spin_lock_irqsave(&data->lock, flags);
+ data->pending = NULL;
spin_unlock_irqrestore(&data->lock, flags);
mutex_unlock(&data->mutex);
return work;
@@ -224,18 +228,21 @@ int picolcd_reset(struct hid_device *hdev)
if (!data || !report || report->maxfield != 1)
return -ENODEV;
+ mutex_lock(&data->report_mutex);
spin_lock_irqsave(&data->lock, flags);
if (hdev->product == USB_DEVICE_ID_PICOLCD_BOOTLOADER)
data->status |= PICOLCD_BOOTLOADER;
- /* perform the reset */
- hid_set_field(report->field[0], 0, 1);
if (data->status & PICOLCD_FAILED) {
spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
return -ENODEV;
}
- hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
spin_unlock_irqrestore(&data->lock, flags);
+ /* perform the reset */
+ hid_set_field(report->field[0], 0, 1);
+ hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
+ mutex_unlock(&data->report_mutex);
error = picolcd_check_version(hdev);
if (error)
@@ -268,7 +275,6 @@ static ssize_t picolcd_operation_mode_store(struct
device *dev,
struct picolcd_data *data = dev_get_drvdata(dev);
struct hid_report *report = NULL;
int timeout = data->opmode_delay;
- unsigned long flags;
if (sysfs_streq(buf, "lcd")) {
if (data->status & PICOLCD_BOOTLOADER)
@@ -283,11 +289,15 @@ static ssize_t
picolcd_operation_mode_store(struct device *dev,
if (!report || report->maxfield != 1)
return -EINVAL;
- spin_lock_irqsave(&data->lock, flags);
+ mutex_lock(&data->report_mutex);
+ if (READ_ONCE(data->status) & PICOLCD_FAILED) {
+ mutex_unlock(&data->report_mutex);
+ return -ENODEV;
+ }
hid_set_field(report->field[0], 0, timeout & 0xff);
hid_set_field(report->field[0], 1, (timeout >> 8) & 0xff);
hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
return count;
}
@@ -537,6 +547,7 @@ static int picolcd_probe(struct hid_device *hdev,
spin_lock_init(&data->lock);
mutex_init(&data->mutex);
+ mutex_init(&data->report_mutex);
data->hdev = hdev;
data->opmode_delay = 5000;
if (hdev->product == USB_DEVICE_ID_PICOLCD_BOOTLOADER)
@@ -603,9 +614,11 @@ static void picolcd_remove(struct hid_device *hdev)
unsigned long flags;
dbg_hid(PICOLCD_NAME " hardware remove...\n");
+ mutex_lock(&data->report_mutex);
spin_lock_irqsave(&data->lock, flags);
data->status |= PICOLCD_FAILED;
spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
picolcd_exit_devfs(data);
device_remove_file(&hdev->dev, &dev_attr_operation_mode);
@@ -630,6 +643,7 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_keys(data);
mutex_destroy(&data->mutex);
+ mutex_destroy(&data->report_mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
}
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 8c28e982e0..c17104fd60 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -91,7 +91,6 @@ static int picolcd_fb_send_tile(struct picolcd_data
*data, u8 *vbitmap,
int chip, int tile)
{
struct hid_report *report1, *report2;
- unsigned long flags;
u8 *tdata;
int i;
@@ -102,9 +101,9 @@ static int picolcd_fb_send_tile(struct
picolcd_data *data, u8 *vbitmap,
if (!report2 || report2->maxfield != 1)
return -ENODEV;
- spin_lock_irqsave(&data->lock, flags);
- if ((data->status & PICOLCD_FAILED)) {
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_lock(&data->report_mutex);
+ if (READ_ONCE(data->status) & PICOLCD_FAILED) {
+ mutex_unlock(&data->report_mutex);
return -ENODEV;
}
hid_set_field(report1->field[0], 0, chip << 2);
@@ -133,7 +132,7 @@ static int picolcd_fb_send_tile(struct
picolcd_data *data, u8 *vbitmap,
hid_hw_request(data->hdev, report1, HID_REQ_SET_REPORT);
hid_hw_request(data->hdev, report2, HID_REQ_SET_REPORT);
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
return 0;
}
@@ -187,13 +186,16 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
struct picolcd_fb_data *fbdata = data->fb_info->par;
int i, j;
- unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00,
0x64, 0xc0 };
if (!report || report->maxfield != 1)
return -ENODEV;
- spin_lock_irqsave(&data->lock, flags);
+ mutex_lock(&data->report_mutex);
+ if (READ_ONCE(data->status) & PICOLCD_FAILED) {
+ mutex_unlock(&data->report_mutex);
+ return -ENODEV;
+ }
for (i = 0; i < 4; i++) {
for (j = 0; j < report->field[0]->maxusage; j++)
if (j == 0)
@@ -204,7 +206,7 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
hid_set_field(report->field[0], j, 0);
hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
}
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
if (clear) {
memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
@@ -232,9 +234,10 @@ static void picolcd_fb_update(struct fb_info *info)
mutex_lock(&info->lock);
spin_lock_irqsave(&fbdata->lock, flags);
- if (!fbdata->ready && fbdata->picolcd)
- picolcd_fb_reset(fbdata->picolcd, 0);
+ data = !fbdata->ready ? fbdata->picolcd : NULL;
spin_unlock_irqrestore(&fbdata->lock, flags);
+ if (data)
+ picolcd_fb_reset(data, 0);
/*
* Translate the framebuffer into the format needed by the PicoLCD.
diff --git a/drivers/hid/hid-picolcd_lcd.c b/drivers/hid/hid-picolcd_lcd.c
index 318f19eac0..a1fdbfdfde 100644
--- a/drivers/hid/hid-picolcd_lcd.c
+++ b/drivers/hid/hid-picolcd_lcd.c
@@ -27,17 +27,16 @@ static int picolcd_set_contrast(struct lcd_device
*ldev, int contrast)
{
struct picolcd_data *data = lcd_get_data(ldev);
struct hid_report *report = picolcd_out_report(REPORT_CONTRAST, data->hdev);
- unsigned long flags;
if (!report || report->maxfield != 1 || report->field[0]->report_count != 1)
return -ENODEV;
+ mutex_lock(&data->report_mutex);
data->lcd_contrast = contrast & 0x0ff;
- spin_lock_irqsave(&data->lock, flags);
hid_set_field(report->field[0], 0, data->lcd_contrast);
- if (!(data->status & PICOLCD_FAILED))
+ if (!(READ_ONCE(data->status) & PICOLCD_FAILED))
hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
- spin_unlock_irqrestore(&data->lock, flags);
+ mutex_unlock(&data->report_mutex);
return 0;
}
diff --git a/drivers/hid/hid-picolcd_leds.c b/drivers/hid/hid-picolcd_leds.c
index 6b505a7535..c6ae8ace1d 100644
--- a/drivers/hid/hid-picolcd_leds.c
+++ b/drivers/hid/hid-picolcd_leds.c
@@ -29,10 +29,9 @@
#include "hid-picolcd.h"
-void picolcd_leds_set(struct picolcd_data *data)
+static void picolcd_leds_set_locked(struct picolcd_data *data)
{
struct hid_report *report;
- unsigned long flags;
if (!data->led[0])
return;
@@ -40,14 +39,19 @@ void picolcd_leds_set(struct picolcd_data *data)
if (!report || report->maxfield != 1 || report->field[0]->report_count != 1)
return;
- spin_lock_irqsave(&data->lock, flags);
hid_set_field(report->field[0], 0, data->led_state);
- if (!(data->status & PICOLCD_FAILED))
+ if (!(READ_ONCE(data->status) & PICOLCD_FAILED))
hid_hw_request(data->hdev, report, HID_REQ_SET_REPORT);
- spin_unlock_irqrestore(&data->lock, flags);
}
-static void picolcd_led_set_brightness(struct led_classdev *led_cdev,
+void picolcd_leds_set(struct picolcd_data *data)
+{
+ mutex_lock(&data->report_mutex);
+ picolcd_leds_set_locked(data);
+ mutex_unlock(&data->report_mutex);
+}
+
+static int picolcd_led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
struct device *dev;
@@ -59,20 +63,23 @@ static void picolcd_led_set_brightness(struct
led_classdev *led_cdev,
hdev = to_hid_device(dev);
data = hid_get_drvdata(hdev);
if (!data)
- return;
+ return -ENODEV;
+ mutex_lock(&data->report_mutex);
for (i = 0; i < 8; i++) {
if (led_cdev != data->led[i])
continue;
state = (data->led_state >> i) & 1;
if (value == LED_OFF && state) {
data->led_state &= ~(1 << i);
- picolcd_leds_set(data);
+ picolcd_leds_set_locked(data);
} else if (value != LED_OFF && !state) {
data->led_state |= 1 << i;
- picolcd_leds_set(data);
+ picolcd_leds_set_locked(data);
}
break;
}
+ mutex_unlock(&data->report_mutex);
+ return 0;
}
static enum led_brightness picolcd_led_get_brightness(struct
led_classdev *led_cdev)
@@ -87,7 +94,7 @@ static enum led_brightness
picolcd_led_get_brightness(struct led_classdev *led_c
data = hid_get_drvdata(hdev);
for (i = 0; i < 8; i++)
if (led_cdev == data->led[i]) {
- value = (data->led_state >> i) & 1;
+ value = (READ_ONCE(data->led_state) >> i) & 1;
break;
}
return value ? LED_FULL : LED_OFF;
@@ -122,7 +129,7 @@ int picolcd_init_leds(struct picolcd_data *data,
struct hid_report *report)
led->brightness = 0;
led->max_brightness = 1;
led->brightness_get = picolcd_led_get_brightness;
- led->brightness_set = picolcd_led_set_brightness;
+ led->brightness_set_blocking = picolcd_led_set_brightness;
data->led[i] = led;
ret = led_classdev_register(dev, data->led[i]);
@@ -159,5 +166,3 @@ void picolcd_exit_leds(struct picolcd_data *data)
kfree(led);
}
}
-
-
--
2.55.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 2/2] HID: picolcd: Avoid framebuffer last-close deadlock
2026-09-28 11:46 [PATCH 0/2] HID: picolcd: Fix output-request and framebuffer locking Aveline Noir
2026-09-28 11:46 ` [PATCH 1/2] HID: picolcd: Move output requests out of spinlocked sections Aveline Noir
@ 2026-09-28 11:46 ` Aveline Noir
1 sibling, 0 replies; 5+ messages in thread
From: Aveline Noir @ 2026-09-28 11:46 UTC (permalink / raw)
To: Bruno Prémont, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, deller, simona, linux-fbdev, dri-devel
Closing the last framebuffer descriptor can deadlock against PicoLCD's
deferred update worker. fb_release() holds info->lock while waiting for
deferred work, and picolcd_fb_update() takes the same lock. Lockdep
reports the cycle through deferred-work completion and fbdefio_state->lock.
Repeated framebuffer open/write/close with concurrent device destruction
reproduces the hang in a PREEMPT_RT QEMU guest.
Use a private update mutex in the deferred worker instead of info->lock.
Take it in picolcd_set_par() as well to preserve serialization of pixel
format conversion and framebuffer updates. Initialize it before exposing
the framebuffer.
The test with the preceding output-request fix hung in the first round
and reported a circular locking dependency. With this change, all five
rounds completed, including 29 successful framebuffer write cycles and
concurrent LCD, backlight, two LED and UHID destroy operations, without
BUG/WARNING. The original syzkaller reproducer and persistent-open
framebuffer tests also passed again. These are bounded virtual-device
tests; physical hardware and suspend/resume remain untested.
Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing
deferred I/O devices")
Signed-off-by: Aveline Noir <jm5905938@gmail.com>
---
drivers/hid/hid-picolcd.h | 2 ++
drivers/hid/hid-picolcd_fb.c | 33 ++++++++++++++++++++++-----------
2 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 846a8ceb95..33e6654ca0 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -115,6 +115,8 @@ struct picolcd_data {
struct picolcd_fb_data {
/* Framebuffer stuff */
spinlock_t lock;
+ /* Deferred I/O runs while fbdefio_state->lock is held. */
+ struct mutex update_lock;
struct picolcd_data *picolcd;
u8 update_rate;
u8 bpp;
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index c17104fd60..258c7c4fa2 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -231,7 +231,8 @@ static void picolcd_fb_update(struct fb_info *info)
struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
- mutex_lock(&info->lock);
+ /* fb_release() flushes this work while holding info->lock. */
+ mutex_lock(&fbdata->update_lock);
spin_lock_irqsave(&fbdata->lock, flags);
data = !fbdata->ready ? fbdata->picolcd : NULL;
@@ -258,11 +259,11 @@ static void picolcd_fb_update(struct fb_info *info)
spin_lock_irqsave(&fbdata->lock, flags);
data = fbdata->picolcd;
spin_unlock_irqrestore(&fbdata->lock, flags);
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
if (!data)
return;
hid_hw_wait(data->hdev);
- mutex_lock(&info->lock);
+ mutex_lock(&fbdata->update_lock);
n = 0;
}
spin_lock_irqsave(&fbdata->lock, flags);
@@ -277,13 +278,13 @@ static void picolcd_fb_update(struct fb_info *info)
spin_lock_irqsave(&fbdata->lock, flags);
data = fbdata->picolcd;
spin_unlock_irqrestore(&fbdata->lock, flags);
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
if (data)
hid_hw_wait(data->hdev);
return;
}
out:
- mutex_unlock(&info->lock);
+ mutex_unlock(&fbdata->update_lock);
}
static int picolcd_fb_blank(int blank, struct fb_info *info)
@@ -332,17 +333,24 @@ static int picolcd_set_par(struct fb_info *info)
{
struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
+ int ret = 0;
+
+ mutex_lock(&fbdata->update_lock);
if (info->var.bits_per_pixel == fbdata->bpp)
- return 0;
+ goto out;
/* switch between 1/8 bit depths */
- if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
- return -EINVAL;
+ if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8) {
+ ret = -EINVAL;
+ goto out;
+ }
o_fb = fbdata->bitmap;
tmp_fb = kmalloc_array(PICOLCDFB_SIZE, info->var.bits_per_pixel,
GFP_KERNEL);
- if (!tmp_fb)
- return -ENOMEM;
+ if (!tmp_fb) {
+ ret = -ENOMEM;
+ goto out;
+ }
/* translate FB content to new bits-per-pixel */
if (info->var.bits_per_pixel == 1) {
@@ -369,7 +377,9 @@ static int picolcd_set_par(struct fb_info *info)
kfree(tmp_fb);
fbdata->bpp = info->var.bits_per_pixel;
- return 0;
+out:
+ mutex_unlock(&fbdata->update_lock);
+ return ret;
}
static void picolcdfb_ops_damage_range(struct fb_info *info, off_t
off, size_t len)
@@ -506,6 +516,7 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
fbdata = info->par;
spin_lock_init(&fbdata->lock);
+ mutex_init(&fbdata->update_lock);
fbdata->picolcd = data;
fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
fbdata->bpp = picolcdfb_var.bits_per_pixel;
--
2.55.0
^ permalink raw reply [flat|nested] 5+ messages in thread