[PATCH 0/4] HID: lg-g15: G15 v2 LCD and game-mode support

[PATCH 0/4] HID: lg-g15: G15 v2 LCD and game-mode support

[Torstein Eide]

This series adds support for more of the Logitech G15 v2 hardware to
hid-lg-g15, plus a fix for the remove path.

Patch 1 exposes the GamePanel LCD contrast as a g15::lcd_contrast LED.

Patch 2 adds an optional fbdev framebuffer (HID_LOGITECH_G15_FB) for
the 160x43 monochrome LCD, updated through deferred I/O, so userspace
can draw on the LCD or bind fbcon to it without libg15/g15daemon.

Patch 3 binds the G15 v2's second interface (046D:C226), which is
currently handled by hid-generic, and exposes the Super-key-lock
("Game Mode") state as a read-only g15::game_mode LED. Normal key
input on that interface is left to hid-input.

Patch 4 fixes a use-after-free on remove: the LED work can be
rescheduled by a key press between cancel_work_sync() and
hid_hw_stop(). The bug predates this series, but the patch is
written against the remove path as changed by patches 1-3, so
it does not apply to the current tree on its own.

The LCD wire format and contrast commands follow libg15, hence the
Based-on-work-by tags for its authors.

All patches were tested on a Logitech G15 v2.

Torstein Eide (4):
  HID: lg-g15: add LCD contrast LED
  HID: lg-g15: add LCD pixel-content framebuffer
  HID: lg-g15: report Super-key-lock state on G15 v2's keyboard
    interface
  HID: lg-g15: cancel LED work only after stopping the hardware

 drivers/hid/Kconfig      |  13 +
 drivers/hid/hid-ids.h    |   1 +
 drivers/hid/hid-lg-g15.c | 599 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 606 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH 1/4] HID: lg-g15: add LCD contrast LED

[Torstein Eide]

Wire values ({2, 32, 129, value}) match libg15's setLCDContrast()
byte-for-byte.

Tested on Logitech G15v2.

Based on libg15.c.

Signed-off-by: Torstein Eide <torsteine+linux@gmail.com>
Based-on-work-by: Philip Lawatsch <philip@lawatsch.at>
Based-on-work-by: Alex Ibrado <alex@ibrado.org>
Based-on-work-by: Anthony J. Mirabella <mirabeaj@gmail.com>
Based-on-work-by: Mike Lampard <mlampard@users.sourceforge.net>
Assisted-by: Claude:claude-sonnet-5
---
 drivers/hid/hid-lg-g15.c | 101 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index 02ef3e2094b4..50abe6c58b10 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -59,7 +59,16 @@ enum lg_g15_led_type {
 	LG_G15_MACRO_PRESET2,
 	LG_G15_MACRO_PRESET3,
 	LG_G15_MACRO_RECORD,
-	LG_G15_LED_MAX
+	LG_G15_LED_MAX,
+	/*
+	 * LCD contrast uses a different feature-report sub-format than the
+	 * brightness/macro fields above (see lg_g15_contrast_set()) and has
+	 * no hardware readback, so it lives outside [0, LG_G15_LED_MAX) to
+	 * avoid being swept into the G13/macro loops bounded by
+	 * LG_G15_LED_MAX. G15/G15 v2 only; registered explicitly.
+	 */
+	LG_G15_LCD_CONTRAST = LG_G15_LED_MAX,
+	LG_G15_LED_ARRAY_SIZE
 };
 
 struct g13_input_report {
@@ -89,9 +98,10 @@ struct lg_g15_data {
 	struct input_dev *input_js; /* Separate joystick device for G13. */
 	struct hid_device *hdev;
 	enum lg_g15_model model;
-	struct lg_g15_led leds[LG_G15_LED_MAX];
+	struct lg_g15_led leds[LG_G15_LED_ARRAY_SIZE];
 	bool game_mode_enabled;
 	bool backlight_disabled;	/* true == HW backlight toggled *OFF* */
+	bool lcd_contrast_set;		/* LCD contrast written at least once */
 };
 
 /********* G13 LED functions ***********/
@@ -365,6 +375,57 @@ static int lg_g15_led_set(struct led_classdev *led_cdev,
 	return ret;
 }
 
+/*
+ * LCD contrast, report 2 field 32: {2, 32, 129, value}, value in
+ * {18, 22, 26} for low/medium/high. Distinct sub-format from the
+ * brightness/macro fields (value sits in byte 3, not byte 2, and byte 2 is
+ * a fixed marker rather than the field selector) and has no GET_REPORT
+ * readback, so brightness_get just returns the last value we set.
+ */
+static int lg_g15_contrast_set(struct led_classdev *led_cdev,
+			       enum led_brightness brightness)
+{
+	struct lg_g15_led *g15_led =
+		container_of(led_cdev, struct lg_g15_led, cdev);
+	struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
+	static const u8 contrast_values[] = { 18, 22, 26 };
+	int ret;
+
+	if (led_cdev->flags & LED_UNREGISTERING)
+		return 0;
+
+	mutex_lock(&g15->mutex);
+
+	g15->transfer_buf[0] = LG_G15_FEATURE_REPORT;
+	g15->transfer_buf[1] = 32;
+	g15->transfer_buf[2] = 129;
+	g15->transfer_buf[3] = contrast_values[brightness];
+
+	ret = hid_hw_raw_request(g15->hdev, LG_G15_FEATURE_REPORT,
+				 g15->transfer_buf, 4,
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+	if (ret == 4) {
+		g15_led->brightness = brightness;
+		g15->lcd_contrast_set = true;
+		ret = 0;
+	} else {
+		hid_err(g15->hdev, "Error setting LCD contrast: %d\n", ret);
+		ret = (ret < 0) ? ret : -EIO;
+	}
+
+	mutex_unlock(&g15->mutex);
+
+	return ret;
+}
+
+static enum led_brightness lg_g15_contrast_get(struct led_classdev *led_cdev)
+{
+	struct lg_g15_led *g15_led =
+		container_of(led_cdev, struct lg_g15_led, cdev);
+
+	return g15_led->brightness;
+}
+
 static void lg_g15_leds_changed_work(struct work_struct *work)
 {
 	struct lg_g15_data *g15 = container_of(work, struct lg_g15_data, work);
@@ -1017,6 +1078,13 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
 		break;
 	case LG_G15:
 	case LG_G15_V2:
+		if (i == LG_G15_LCD_CONTRAST) {
+			g15->leds[i].cdev.brightness_set_blocking = lg_g15_contrast_set;
+			g15->leds[i].cdev.brightness_get = lg_g15_contrast_get;
+			g15->leds[i].cdev.max_brightness = 2;
+			ret = devm_led_classdev_register(&g15->hdev->dev, &g15->leds[i].cdev);
+			break;
+		}
 		g15->leds[i].cdev.brightness_get = lg_g15_led_get;
 		fallthrough;
 	case LG_Z10:
@@ -1333,6 +1401,14 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 			goto error_hw_stop;
 	}
 
+	/* The G11 shares the LG_G15 model but has no LCD */
+	if ((g15->model == LG_G15 && hdev->product != USB_DEVICE_ID_LOGITECH_G11) ||
+	    g15->model == LG_G15_V2) {
+		ret = lg_g15_register_led(g15, LG_G15_LCD_CONTRAST, "g15::lcd_contrast");
+		if (ret)
+			goto error_hw_stop;
+	}
+
 	return 0;
 
 error_hw_stop:
@@ -1389,12 +1465,33 @@ static void lg_g15_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
+/*
+ * The LCD does not retain its contrast across a USB reset or power loss;
+ * re-apply the last value userspace set, if any.
+ */
+static int lg_g15_resume(struct hid_device *hdev)
+{
+	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
+	struct lg_g15_led *contrast;
+
+	if (!g15)
+		return 0;
+
+	contrast = &g15->leds[LG_G15_LCD_CONTRAST];
+	if (contrast->cdev.dev && g15->lcd_contrast_set)
+		lg_g15_contrast_set(&contrast->cdev, contrast->brightness);
+
+	return 0;
+}
+
 static struct hid_driver lg_g15_driver = {
 	.name			= "lg-g15",
 	.id_table		= lg_g15_devices,
 	.raw_event		= lg_g15_raw_event,
 	.probe			= lg_g15_probe,
 	.remove			= lg_g15_remove,
+	.resume			= lg_g15_resume,
+	.reset_resume		= lg_g15_resume,
 };
 module_hid_driver(lg_g15_driver);
 
-- 
2.53.0

Re: [PATCH 1/4] HID: lg-g15: add LCD contrast LED

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] HID: lg-g15: out-of-bounds read in lg_g15_contrast_set() on
  unclamped brightness
- [Low] HID: lg-g15: unlocked read of contrast brightness in
  lg_g15_resume()
--

commit 8cb726418b3437c4e73ef257cf7f4986794e4a67
Author: Torstein Eide <torsteine+linux@gmail.com>

HID: lg-g15: add LCD contrast LED

This commit adds an LED class device to control the LCD contrast on
Logitech G15 and G15 v2 keyboards and restores the configured contrast on
resume.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 02ef3e2094b4c..50abe6c58b10b 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c

[ ... ]

> @@ -365,6 +375,57 @@ static int lg_g15_led_set(struct led_classdev *led_cdev,
>  	return ret;
>  }
>  
> +/*
> + * LCD contrast, report 2 field 32: {2, 32, 129, value}, value in
> + * {18, 22, 26} for low/medium/high. Distinct sub-format from the
> + * brightness/macro fields (value sits in byte 3, not byte 2, and byte 2 is
> + * a fixed marker rather than the field selector) and has no GET_REPORT
> + * readback, so brightness_get just returns the last value we set.
> + */
> +static int lg_g15_contrast_set(struct led_classdev *led_cdev,
> +			       enum led_brightness brightness)
> +{
> +	struct lg_g15_led *g15_led =
> +		container_of(led_cdev, struct lg_g15_led, cdev);
> +	struct lg_g15_data *g15 = dev_get_drvdata(led_cdev->dev->parent);
> +	static const u8 contrast_values[] = { 18, 22, 26 };
> +	int ret;
> +
> +	if (led_cdev->flags & LED_UNREGISTERING)
> +		return 0;
> +
> +	mutex_lock(&g15->mutex);
> +
> +	g15->transfer_buf[0] = LG_G15_FEATURE_REPORT;
> +	g15->transfer_buf[1] = 32;
> +	g15->transfer_buf[2] = 129;
> +	g15->transfer_buf[3] = contrast_values[brightness];

[Severity: Medium]
Can this read past the end of contrast_values[] when brightness > 2?

When LED_SET_BRIGHTNESS or LED_BLINK_DISABLE is already set in
led_cdev->work_flags (for example, during back-to-back sysfs writes to
brightness), led_set_brightness() stores the unclamped brightness value in
led_cdev->delayed_set_value before led_set_brightness_nosleep() clamps it
to max_brightness:

drivers/leds/led-core.c:led_set_brightness() {
    ...
		if (test_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags) ||
		    test_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
			led_cdev->delayed_set_value = brightness;
			set_bit(LED_SET_BRIGHTNESS, &led_cdev->work_flags);
			queue_work(led_cdev->wq, &led_cdev->set_brightness_work);
			return;
		}
    ...
}

set_brightness_delayed() then passes delayed_set_value directly to
brightness_set_blocking() without clamping, so a value >= 3 indexes out of
bounds in contrast_values[], sends that byte in the feature report, and
caches the out-of-bounds index in g15_led->brightness.

[ ... ]

> @@ -1389,12 +1465,33 @@ static void lg_g15_remove(struct hid_device *hdev)
>  	hid_hw_stop(hdev);
>  }
>  
> +/*
> + * The LCD does not retain its contrast across a USB reset or power loss;
> + * re-apply the last value userspace set, if any.
> + */
> +static int lg_g15_resume(struct hid_device *hdev)
> +{
> +	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +	struct lg_g15_led *contrast;
> +
> +	if (!g15)
> +		return 0;
> +
> +	contrast = &g15->leds[LG_G15_LCD_CONTRAST];
> +	if (contrast->cdev.dev && g15->lcd_contrast_set)
> +		lg_g15_contrast_set(&contrast->cdev, contrast->brightness);

[Severity: Low]
Can reading contrast->brightness without g15->mutex overwrite a concurrent
contrast update?

CPU0                              CPU1 (kworker)
lg_g15_resume()
  val = contrast->brightness
                                  lg_g15_contrast_set(..., new_val)
                                    mutex_lock(&g15->mutex)
                                    hid_hw_raw_request(...)
                                    contrast->brightness = new_val
                                    mutex_unlock(&g15->mutex)
  lg_g15_contrast_set(..., val)
    mutex_lock(&g15->mutex)
    hid_hw_raw_request(...)
    contrast->brightness = val    <-- overwrites new_val
    mutex_unlock(&g15->mutex)

Because lg_g15_resume() reads contrast->brightness and
g15->lcd_contrast_set before lg_g15_contrast_set() acquires g15->mutex, a
concurrent update on leds_wq can complete in between and then be clobbered
by the stale value on both the hardware and contrast->brightness.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094426.1432465-1-torsteine+linux@gmail.com?part=1

[PATCH 2/4] HID: lg-g15: add LCD pixel-content framebuffer

[Torstein Eide]

Add a 160x43 1bpp fbdev framebuffer for the GamePanel LCD's pixel
content, behind the new HID_LOGITECH_G15_FB option.

Updates use fbdev deferred I/O: when userspace dirties the framebuffer
(mmap, write() or the drawing ops) the frame is packed into the LCD's
column-major wire format and sent as Output Report 3. The packed wire
buffer is a heap allocation, not a stack array, since the USB core
refuses to DMA-map stack memory. A set bit is a dark pixel, so the
visual is FB_VISUAL_MONO01.

The keyboard ignores output reports unless an URB is queued on the
input endpoint, so the HID device is kept open while the framebuffer
exists. The fb_info is released from fb_destroy, as it may outlive the
HID device while /dev/fbN is still open. The last frame is re-sent on
resume, since the LCD loses its content across a USB reset.

Tested on Logitech G15v2.

Based on libg15.c.

Signed-off-by: Torstein Eide <torsteine+linux@gmail.com>
Based-on-work-by: Philip Lawatsch <philip@lawatsch.at>
Based-on-work-by: Alex Ibrado <alex@ibrado.org>
Based-on-work-by: Anthony J. Mirabella <mirabeaj@gmail.com>
Based-on-work-by: Mike Lampard <mlampard@users.sourceforge.net>
Assisted-by: Claude:claude-opus-5-5
---
 drivers/hid/Kconfig      |  13 ++
 drivers/hid/hid-lg-g15.c | 303 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 316 insertions(+)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 2481bc01c435..57305180818a 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -674,6 +674,19 @@ config HID_LOGITECH
 	help
 	Support for Logitech devices that are not fully compliant with HID standard.
 
+config HID_LOGITECH_G15_FB
+	bool "Logitech G15 LCD framebuffer support"
+	depends on HID_LOGITECH
+	depends on HID_LOGITECH=FB || FB=y
+	select FB_SYSMEM_HELPERS_DEFERRED
+	help
+	  Provide access to the 160x43 monochrome GamePanel LCD of the
+	  Logitech G15 and G15 v2 keyboards via a framebuffer device.
+
+	  The LCD is updated with deferred I/O whenever the framebuffer
+	  contents change. Say Y here if you want to draw on the LCD from
+	  userspace or bind a framebuffer console to it.
+
 config HID_LOGITECH_DJ
 	tristate "Logitech receivers full support"
 	depends on USB_HID
diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index 50abe6c58b10..35d183787383 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/fb.h>
 #include <linux/hid.h>
 #include <linux/leds.h>
 #include <linux/led-class-multicolor.h>
@@ -13,6 +14,7 @@
 #include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/usb.h>
+#include <linux/vmalloc.h>
 #include <linux/wait.h>
 #include <dt-bindings/leds/common.h>
 
@@ -22,6 +24,22 @@
 
 #define LG_G15_FEATURE_REPORT		0x02
 
+/*
+ * LCD pixel content, G15/G15 v2 GamePanel LCD only. Not a Feature Report:
+ * a numbered Output Report sent over the interrupt-OUT endpoint. The wire
+ * buffer has a fixed 32-byte header before the packed pixel data starts,
+ * and the packer's last (partial) row-group reads up to 132 bytes past
+ * the logical WIDTH/8*HEIGHT frame size, so the backing allocation must
+ * be padded to LG_G15_LCD_WIRE_SIZE, not just the logical frame size.
+ */
+#define LG_G15_LCD_OUTPUT_REPORT	0x03
+#define LG_G15_LCD_WIDTH		160
+#define LG_G15_LCD_HEIGHT		43
+#define LG_G15_LCD_ROW_BYTES		(LG_G15_LCD_WIDTH / 8)
+#define LG_G15_LCD_FB_SIZE		(LG_G15_LCD_ROW_BYTES * LG_G15_LCD_HEIGHT)
+#define LG_G15_LCD_WIRE_OFFSET		32
+#define LG_G15_LCD_WIRE_SIZE		0x03e0
+
 #define LG_G510_FEATURE_M_KEYS_LEDS	0x04
 #define LG_G510_FEATURE_BACKLIGHT_RGB	0x05
 #define LG_G510_FEATURE_POWER_ON_RGB	0x06
@@ -88,6 +106,17 @@ struct lg_g15_led {
 	u8 red, green, blue;
 };
 
+struct lg_g15_fbdev {
+	struct fb_info *info;	/* NULL when no framebuffer is registered */
+	/*
+	 * Packed wire-format buffer, heap-allocated (not a local/stack
+	 * array): it gets DMA-mapped by hid_hw_output_report()/usbhid, and
+	 * the USB core refuses to map stack memory (returns -EAGAIN, see
+	 * object_is_on_stack() in drivers/usb/core/hcd.c).
+	 */
+	u8 *wire;
+};
+
 struct lg_g15_data {
 	/* Must be first for proper dma alignment */
 	u8 transfer_buf[LG_G15_TRANSFER_BUF_SIZE];
@@ -99,6 +128,7 @@ struct lg_g15_data {
 	struct hid_device *hdev;
 	enum lg_g15_model model;
 	struct lg_g15_led leds[LG_G15_LED_ARRAY_SIZE];
+	struct lg_g15_fbdev fbdev; /* G15/G15 v2 GamePanel LCD only */
 	bool game_mode_enabled;
 	bool backlight_disabled;	/* true == HW backlight toggled *OFF* */
 	bool lcd_contrast_set;		/* LCD contrast written at least once */
@@ -1051,6 +1081,270 @@ static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
 	gled->mcdev.subled_info = subled_info;
 }
 
+/********* G15 LCD framebuffer *********/
+
+#ifdef CONFIG_HID_LOGITECH_G15_FB
+
+/*
+ * fb_info private data. The fb_info can outlive the HID device (e.g. when
+ * /dev/fbN is still open on unplug), so the link back to the device is
+ * cleared in lg_g15_unregister_fb() and the fb_info itself is only freed
+ * from fb_destroy once its last user is gone.
+ */
+struct lg_g15_fb_par {
+	struct fb_deferred_io defio;
+	/* Protects g15 and serializes frame updates */
+	struct mutex lock;
+	/* NULL once the HID device is gone */
+	struct lg_g15_data *g15;
+	/* At least one frame was sent successfully */
+	bool sent;
+};
+
+static const struct fb_fix_screeninfo lg_g15_fb_fix = {
+	.id		= "G15 LCD",
+	.type		= FB_TYPE_PACKED_PIXELS,
+	/* A set bit is a dark pixel */
+	.visual		= FB_VISUAL_MONO01,
+	.xpanstep	= 0,
+	.ypanstep	= 0,
+	.ywrapstep	= 0,
+	.line_length	= LG_G15_LCD_ROW_BYTES,
+	.smem_len	= LG_G15_LCD_FB_SIZE,
+	.accel		= FB_ACCEL_NONE,
+};
+
+static const struct fb_var_screeninfo lg_g15_fb_var = {
+	.xres		= LG_G15_LCD_WIDTH,
+	.yres		= LG_G15_LCD_HEIGHT,
+	.xres_virtual	= LG_G15_LCD_WIDTH,
+	.yres_virtual	= LG_G15_LCD_HEIGHT,
+	.bits_per_pixel	= 1,
+	.red		= { 0, 1, 0 },
+	.green		= { 0, 1, 0 },
+	.blue		= { 0, 1, 0 },
+	.vmode		= FB_VMODE_NONINTERLACED,
+};
+
+/*
+ * Packs pixels column-major (8 vertically-stacked source rows per
+ * destination byte), not the row-major layout the linear fb buffer holds.
+ *
+ * fb is LG_G15_LCD_WIRE_SIZE bytes (padded, not just LG_G15_LCD_FB_SIZE) -
+ * the last of the six 8-row groups only has 3 real pixel-rows (43 isn't a
+ * multiple of 8), and this reads past the logical frame into that padding
+ * for the other 5.
+ */
+static void lg_g15_fb_pack(u8 *wire, const u8 *fb)
+{
+	unsigned int output_offset = LG_G15_LCD_WIRE_OFFSET;
+	unsigned int base_offset = 0;
+	unsigned int row, col;
+	unsigned int height_in_bytes = DIV_ROUND_UP(LG_G15_LCD_HEIGHT, 8);
+
+	for (row = 0; row < height_in_bytes; row++) {
+		for (col = 0; col < LG_G15_LCD_WIDTH; col++) {
+			unsigned int bit = col % 8;
+			unsigned int sub;
+			u8 packed = 0;
+
+			for (sub = 0; sub < 8; sub++) {
+				unsigned int pixel_offset =
+					base_offset + sub * LG_G15_LCD_ROW_BYTES;
+
+				packed |= ((fb[pixel_offset] << bit) & 0x80) >> (7 - sub);
+			}
+
+			wire[output_offset] = packed;
+			output_offset++;
+			if (bit == 7)
+				base_offset++;
+		}
+		base_offset += LG_G15_LCD_WIDTH - LG_G15_LCD_ROW_BYTES;
+	}
+}
+
+/*
+ * Deferred IO callback, runs when userspace dirtied the framebuffer (through
+ * mmap, write() or the drawing ops). Writes made while a frame is being sent
+ * re-arm the deferred work, so no update is lost.
+ */
+static void lg_g15_fb_deferred_io(struct fb_info *info, struct list_head *pagereflist)
+{
+	struct lg_g15_fb_par *par = info->par;
+	struct lg_g15_data *g15;
+	u8 *wire;
+	int ret;
+
+	mutex_lock(&par->lock);
+
+	g15 = par->g15;
+	if (!g15)
+		goto out;
+
+	wire = g15->fbdev.wire;
+	memset(wire, 0, LG_G15_LCD_WIRE_OFFSET);
+	lg_g15_fb_pack(wire, (const u8 *)info->screen_buffer);
+	wire[0] = LG_G15_LCD_OUTPUT_REPORT;
+
+	ret = hid_hw_output_report(g15->hdev, wire, LG_G15_LCD_WIRE_SIZE);
+	if (ret < 0)
+		hid_err_ratelimited(g15->hdev, "Error writing LCD framebuffer: %d\n", ret);
+	else
+		par->sent = true;
+
+out:
+	mutex_unlock(&par->lock);
+}
+
+static void lg_g15_fb_damage_range(struct fb_info *info, off_t off, size_t len)
+{
+	schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
+}
+
+static void lg_g15_fb_damage_area(struct fb_info *info, u32 x, u32 y,
+				  u32 width, u32 height)
+{
+	schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
+}
+
+FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(lg_g15_fb,
+				   lg_g15_fb_damage_range,
+				   lg_g15_fb_damage_area)
+
+static int lg_g15_fb_blank(int blank, struct fb_info *info)
+{
+	return 0;
+}
+
+static void lg_g15_fb_destroy(struct fb_info *info)
+{
+	struct lg_g15_fb_par *par = info->par;
+
+	fb_deferred_io_cleanup(info);
+	mutex_destroy(&par->lock);
+	vfree(info->screen_buffer);
+	framebuffer_release(info);
+}
+
+static const struct fb_ops lg_g15_fb_ops = {
+	.owner		= THIS_MODULE,
+	FB_DEFAULT_DEFERRED_OPS(lg_g15_fb),
+	.fb_destroy	= lg_g15_fb_destroy,
+	.fb_blank	= lg_g15_fb_blank,
+};
+
+static int lg_g15_register_fb(struct lg_g15_data *g15)
+{
+	struct lg_g15_fbdev *fbdev = &g15->fbdev;
+	struct lg_g15_fb_par *par;
+	struct fb_info *info;
+	void *buffer;
+	int ret;
+
+	fbdev->wire = devm_kzalloc(&g15->hdev->dev, LG_G15_LCD_WIRE_SIZE, GFP_KERNEL);
+	if (!fbdev->wire)
+		return -ENOMEM;
+
+	/* Padded to LG_G15_LCD_WIRE_SIZE - see the comment by that macro. */
+	buffer = vzalloc(LG_G15_LCD_WIRE_SIZE);
+	if (!buffer)
+		return -ENOMEM;
+
+	info = framebuffer_alloc(sizeof(*par), &g15->hdev->dev);
+	if (!info) {
+		ret = -ENOMEM;
+		goto err_free_buffer;
+	}
+
+	par = info->par;
+	mutex_init(&par->lock);
+	par->g15 = g15;
+	par->defio.delay = HZ / 10;
+	par->defio.deferred_io = lg_g15_fb_deferred_io;
+
+	info->fbdefio = &par->defio;
+	info->fbops = &lg_g15_fb_ops;
+	info->flags |= FBINFO_VIRTFB;
+	info->screen_buffer = buffer;
+	info->screen_size = LG_G15_LCD_FB_SIZE;
+	info->fix = lg_g15_fb_fix;
+	info->var = lg_g15_fb_var;
+
+	ret = fb_deferred_io_init(info);
+	if (ret)
+		goto err_release_info;
+
+	/*
+	 * As with the G-keys settings report in probe, the keyboard ignores
+	 * output reports unless an URB is queued on the input endpoint, so
+	 * keep the device open for as long as the framebuffer exists.
+	 */
+	ret = hid_hw_open(g15->hdev);
+	if (ret)
+		goto err_defio_cleanup;
+
+	ret = register_framebuffer(info);
+	if (ret)
+		goto err_hw_close;
+
+	fbdev->info = info;
+	return 0;
+
+err_hw_close:
+	hid_hw_close(g15->hdev);
+err_defio_cleanup:
+	fb_deferred_io_cleanup(info);
+err_release_info:
+	mutex_destroy(&par->lock);
+	framebuffer_release(info);
+err_free_buffer:
+	vfree(buffer);
+	return ret;
+}
+
+static void lg_g15_unregister_fb(struct lg_g15_data *g15)
+{
+	struct fb_info *info = g15->fbdev.info;
+	struct lg_g15_fb_par *par;
+
+	if (!info)
+		return;
+
+	/* Waits for a running update; later ones see par->g15 == NULL */
+	par = info->par;
+	mutex_lock(&par->lock);
+	par->g15 = NULL;
+	mutex_unlock(&par->lock);
+
+	g15->fbdev.info = NULL;
+	/* The fb_info is freed by lg_g15_fb_destroy() once its last user is gone */
+	unregister_framebuffer(info);
+	hid_hw_close(g15->hdev);
+}
+
+/* The LCD loses its pixel content across a USB reset or power loss */
+static void lg_g15_fb_redraw(struct lg_g15_data *g15)
+{
+	struct fb_info *info = g15->fbdev.info;
+	struct lg_g15_fb_par *par;
+
+	if (!info)
+		return;
+
+	par = info->par;
+	if (par->sent)
+		schedule_delayed_work(&info->deferred_work, 0);
+}
+
+#else /* CONFIG_HID_LOGITECH_G15_FB */
+
+static inline int lg_g15_register_fb(struct lg_g15_data *g15) { return 0; }
+static inline void lg_g15_unregister_fb(struct lg_g15_data *g15) { }
+static inline void lg_g15_fb_redraw(struct lg_g15_data *g15) { }
+
+#endif /* CONFIG_HID_LOGITECH_G15_FB */
+
 static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
 {
 	int ret;
@@ -1407,6 +1701,10 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		ret = lg_g15_register_led(g15, LG_G15_LCD_CONTRAST, "g15::lcd_contrast");
 		if (ret)
 			goto error_hw_stop;
+
+		ret = lg_g15_register_fb(g15);
+		if (ret)
+			goto error_hw_stop;
 	}
 
 	return 0;
@@ -1462,6 +1760,9 @@ static void lg_g15_remove(struct hid_device *hdev)
 	if (g15 && g15->work.func)
 		cancel_work_sync(&g15->work);
 
+	if (g15)
+		lg_g15_unregister_fb(g15);
+
 	hid_hw_stop(hdev);
 }
 
@@ -1481,6 +1782,8 @@ static int lg_g15_resume(struct hid_device *hdev)
 	if (contrast->cdev.dev && g15->lcd_contrast_set)
 		lg_g15_contrast_set(&contrast->cdev, contrast->brightness);
 
+	lg_g15_fb_redraw(g15);
+
 	return 0;
 }
 
-- 
2.53.0

Re: [PATCH 2/4] HID: lg-g15: add LCD pixel-content framebuffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] HID: lg-g15: reversed 1bpp pixel bit order in
  lg_g15_fb_pack()
- [Low] HID: lg-g15: no-op lg_g15_fb_blank() breaks fbcon blanking and
  blank state
- [Low] HID: lg-g15: race and missed redraw onpar->sent in
  lg_g15_fb_redraw()
--

commit bcb1dfda67b01b4a6f0c45e68ab96dc227cf9166
Author: Torstein Eide <torsteine+linux@gmail.com>

HID: lg-g15: add LCD pixel-content framebuffer

This commit adds a 160x43 1bpp fbdev framebuffer using deferred I/O for
the GamePanel LCD on Logitech G15 and G15 v2 keyboards.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 50abe6c58b10b..35d183787383a 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c

[ ... ]

> @@ -1051,6 +1081,270 @@ static void lg_g15_setup_led_rgb(struct lg_g15_data *g15, int index)
>  	gled->mcdev.subled_info = subled_info;
>  }
>  
> +/********* G15 LCD framebuffer *********/
> +
> +#ifdef CONFIG_HID_LOGITECH_G15_FB

[ ... ]

> +static const struct fb_var_screeninfo lg_g15_fb_var = {
> +	.xres		= LG_G15_LCD_WIDTH,
> +	.yres		= LG_G15_LCD_HEIGHT,
> +	.xres_virtual	= LG_G15_LCD_WIDTH,
> +	.yres_virtual	= LG_G15_LCD_HEIGHT,
> +	.bits_per_pixel	= 1,
> +	.red		= { 0, 1, 0 },
> +	.green		= { 0, 1, 0 },
> +	.blue		= { 0, 1, 0 },
> +	.vmode		= FB_VMODE_NONINTERLACED,
> +};
> +
> +/*
> + * Packs pixels column-major (8 vertically-stacked source rows per
> + * destination byte), not the row-major layout the linear fb buffer holds.
> + *
> + * fb is LG_G15_LCD_WIRE_SIZE bytes (padded, not just LG_G15_LCD_FB_SIZE) -
> + * the last of the six 8-row groups only has 3 real pixel-rows (43 isn't a
> + * multiple of 8), and this reads past the logical frame into that padding
> + * for the other 5.
> + */
> +static void lg_g15_fb_pack(u8 *wire, const u8 *fb)
> +{
> +	unsigned int output_offset = LG_G15_LCD_WIRE_OFFSET;
> +	unsigned int base_offset = 0;
> +	unsigned int row, col;
> +	unsigned int height_in_bytes = DIV_ROUND_UP(LG_G15_LCD_HEIGHT, 8);
> +
> +	for (row = 0; row < height_in_bytes; row++) {
> +		for (col = 0; col < LG_G15_LCD_WIDTH; col++) {
> +			unsigned int bit = col % 8;
> +			unsigned int sub;
> +			u8 packed = 0;
> +
> +			for (sub = 0; sub < 8; sub++) {
> +				unsigned int pixel_offset =
> +					base_offset + sub * LG_G15_LCD_ROW_BYTES;
> +
> +				packed |= ((fb[pixel_offset] << bit) & 0x80) >> (7 - sub);
> +			}

[Severity: Medium]
Does lg_g15_fb_pack() extract bits from each source byte in reversed
horizontal order compared to standard 1bpp fbdev drawing helpers?

On little-endian architectures, sys_imageblit(), sys_fillrect(), and
sys_copyarea() (wired in via FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS without
FB_NONSTD_REV_PIX_IN_B) place the leftmost pixel of each 8-pixel group
(col % 8 == 0) in bit 0 (0x01) and col % 8 == 7 in bit 7 (0x80).
Shifting fb[pixel_offset] left by bit and masking with 0x80 reads bit 7
for col % 8 == 0 and bit 0 for col % 8 == 7, horizontally mirroring every
8-pixel byte group when rendering via fbcon or the sys_* drawing ops.
Extracting ((fb[pixel_offset] >> bit) & 1) << sub would match standard
1bpp ordering.

> +
> +			wire[output_offset] = packed;
> +			output_offset++;
> +			if (bit == 7)
> +				base_offset++;
> +		}
> +		base_offset += LG_G15_LCD_WIDTH - LG_G15_LCD_ROW_BYTES;
> +	}
> +}
> +
> +/*
> + * Deferred IO callback, runs when userspace dirtied the framebuffer (through
> + * mmap, write() or the drawing ops). Writes made while a frame is being sent
> + * re-arm the deferred work, so no update is lost.
> + */
> +static void lg_g15_fb_deferred_io(struct fb_info *info, struct list_head *pagereflist)
> +{
> +	struct lg_g15_fb_par *par = info->par;
> +	struct lg_g15_data *g15;
> +	u8 *wire;
> +	int ret;
> +
> +	mutex_lock(&par->lock);
> +
> +	g15 = par->g15;
> +	if (!g15)
> +		goto out;
> +
> +	wire = g15->fbdev.wire;
> +	memset(wire, 0, LG_G15_LCD_WIRE_OFFSET);
> +	lg_g15_fb_pack(wire, (const u8 *)info->screen_buffer);
> +	wire[0] = LG_G15_LCD_OUTPUT_REPORT;
> +
> +	ret = hid_hw_output_report(g15->hdev, wire, LG_G15_LCD_WIRE_SIZE);
> +	if (ret < 0)
> +		hid_err_ratelimited(g15->hdev, "Error writing LCD framebuffer: %d\n", ret);
> +	else
> +		par->sent = true;
> +
> +out:
> +	mutex_unlock(&par->lock);
> +}
> +
> +static void lg_g15_fb_damage_range(struct fb_info *info, off_t off, size_t len)
> +{
> +	schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
> +}
> +
> +static void lg_g15_fb_damage_area(struct fb_info *info, u32 x, u32 y,
> +				  u32 width, u32 height)
> +{
> +	schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
> +}
> +
> +FB_GEN_DEFAULT_DEFERRED_SYSMEM_OPS(lg_g15_fb,
> +				   lg_g15_fb_damage_range,
> +				   lg_g15_fb_damage_area)
> +
> +static int lg_g15_fb_blank(int blank, struct fb_info *info)
> +{
> +	return 0;
> +}
> +
> +static void lg_g15_fb_destroy(struct fb_info *info)
> +{
> +	struct lg_g15_fb_par *par = info->par;
> +
> +	fb_deferred_io_cleanup(info);
> +	mutex_destroy(&par->lock);
> +	vfree(info->screen_buffer);
> +	framebuffer_release(info);
> +}
> +
> +static const struct fb_ops lg_g15_fb_ops = {
> +	.owner		= THIS_MODULE,
> +	FB_DEFAULT_DEFERRED_OPS(lg_g15_fb),
> +	.fb_destroy	= lg_g15_fb_destroy,
> +	.fb_blank	= lg_g15_fb_blank,
> +};

[Severity: Low]
Does providing a no-op lg_g15_fb_blank() that returns 0 prevent fbcon
from blanking the display?

When fb_ops->fb_blank is populated, do_register_framebuffer() initializes
info->blank to FB_BLANK_POWERDOWN, and fbcon_blank() only falls back to
clearing the screen in software via fbcon_generic_blank() when fb_blank()
returns non-zero.  In addition, because info->bl_dev and info->lcd_dev
are NULL, returning 0 from fb_blank() causes fb_bl_notify_blank(),
fb_lcd_notify_blank(), and fb_ledtrig_backlight_notify_blank() to
broadcast blank events to unrelated backlight and LCD devices on the
system.  Omitting .fb_blank from lg_g15_fb_ops avoids these side effects.

[ ... ]

> +/* The LCD loses its pixel content across a USB reset or power loss */
> +static void lg_g15_fb_redraw(struct lg_g15_data *g15)
> +{
> +	struct fb_info *info = g15->fbdev.info;
> +	struct lg_g15_fb_par *par;
> +
> +	if (!info)
> +		return;
> +
> +	par = info->par;
> +	if (par->sent)
> +		schedule_delayed_work(&info->deferred_work, 0);
> +}

[Severity: Low]
Can lg_g15_fb_redraw() miss redrawing the initial frame across suspend and
resume?

In lg_g15_fb_deferred_io(), par->sent is only set when
hid_hw_output_report() succeeds.  If the first frame's deferred I/O work
runs while the USB device is suspended, fb_deferred_io_work consumes the
dirty state and hid_hw_output_report() fails with -EHOSTUNREACH, leaving
par->sent false so lg_g15_fb_redraw() skips rescheduling the update on
resume.

Also, lg_g15_fb_redraw() reads par->sent without holding par->lock:

kworker                             CPU1
lg_g15_fb_deferred_io()
  mutex_lock(&par->lock)
  hid_hw_output_report(...)
                                    lg_g15_resume()
                                      lg_g15_fb_redraw()
                                        if (par->sent)  <-- reads false
  par->sent = true
  mutex_unlock(&par->lock)

Setting par->sent = true before hid_hw_output_report() in
lg_g15_fb_deferred_io() and taking par->lock in lg_g15_fb_redraw() would
ensure the frame is re-sent after resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094426.1432465-1-torsteine+linux@gmail.com?part=2

[PATCH 3/4] HID: lg-g15: report Super-key-lock state on G15 v2's keyboard interface

[Torstein Eide]

The G15 v2 exposes a second USB HID interface (046D:C226) previously
left to hid-generic, which reports the Super-key-lock ("Game Mode")
state on report 3. Bind it as LG_G15_V2_KBD: keep default hid-input
behavior for normal typing, and additionally decode report 3 to expose
the lock state as a read-only g15::game_mode LED.

Tested on Logitech G15v2.

Signed-off-by: Torstein Eide <torsteine+linux@gmail.com>
Assisted-by: Claude:claude-sonnet-5
---
 drivers/hid/hid-ids.h    |   1 +
 drivers/hid/hid-lg-g15.c | 192 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 189 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 91319aa6516d..44ebf2a0accc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -950,6 +950,7 @@
 #define USB_DEVICE_ID_LOGITECH_G13		0xc21c
 #define USB_DEVICE_ID_LOGITECH_G15_LCD		0xc222
 #define USB_DEVICE_ID_LOGITECH_G11		0xc225
+#define USB_DEVICE_ID_LOGITECH_G15_V2_KBD	0xc226
 #define USB_DEVICE_ID_LOGITECH_G15_V2_LCD	0xc227
 #define USB_DEVICE_ID_LOGITECH_G510		0xc22d
 #define USB_DEVICE_ID_LOGITECH_G510_USB_AUDIO	0xc22e
diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index 35d183787383..592c9747738e 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -63,7 +63,8 @@
 enum lg_g15_model {
 	LG_G13,
 	LG_G15,
-	LG_G15_V2,
+	LG_G15_V2,	/* 046D:C227, GamePanel LCD iface */
+	LG_G15_V2_KBD,	/* 046D:C226, keyboard/consumer-ctrl iface, not the LCD */
 	LG_G510,
 	LG_G510_USB_AUDIO,
 	LG_Z10,
@@ -134,6 +135,17 @@ struct lg_g15_data {
 	bool lcd_contrast_set;		/* LCD contrast written at least once */
 };
 
+/*
+ * State for the G15 v2's separate keyboard + consumer-control USB device
+ * (046D:C226). Unlike lg_g15_data above, this is not the GamePanel LCD
+ * interface: it carries normal typing (left to generic hid-input) plus one
+ * extra report we care about, so it only needs a small amount of state.
+ */
+struct lg_g15_kbd_data {
+	struct led_classdev led;	/* g15::game_mode, read-only */
+	bool super_lock;		/* Cached HW state of the Super/Windows-key lock */
+};
+
 /********* G13 LED functions ***********/
 /*
  * G13 retains no state across power cycles, and always powers up with the backlight on,
@@ -717,6 +729,8 @@ static int lg_g15_get_initial_led_brightness(struct lg_g15_data *g15)
 		 * the LCD and touch keys part of the speakers.
 		 */
 		return 0;
+	case LG_G15_V2_KBD:
+		break; /* Never reached, this model has no lg_g15_data */
 	}
 	return -EINVAL; /* Never reached */
 }
@@ -981,11 +995,138 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
 	return 0;
 }
 
+/********* G15 v2 keyboard interface functions *********/
+
+/* Report 3, byte 1 bit 0: Super/Windows key lock state ("Game Mode") */
+#define LG_G15_V2_KBD_GAME_MODE_REPORT	0x03
+
+/*
+ * The G15 v2's keyboard device uses struct lg_g15_kbd_data as drvdata, not
+ * struct lg_g15_data. This is the one check used by every entry point to
+ * tell the two apart.
+ */
+static bool lg_g15_is_v2_kbd(struct hid_device *hdev)
+{
+	return hdev->vendor == USB_VENDOR_ID_LOGITECH &&
+	       hdev->product == USB_DEVICE_ID_LOGITECH_G15_V2_KBD;
+}
+
+static enum led_brightness lg_g15_v2_kbd_super_lock_get(struct led_classdev *led_cdev)
+{
+	struct lg_g15_kbd_data *kbd = container_of(led_cdev, struct lg_g15_kbd_data, led);
+
+	return kbd->super_lock;
+}
+
+static void lg_g15_v2_kbd_set_super_lock(struct lg_g15_kbd_data *kbd, bool super_lock)
+{
+	if (super_lock == kbd->super_lock)
+		return;
+
+	kbd->super_lock = super_lock;
+	led_classdev_notify_brightness_hw_changed(&kbd->led, super_lock);
+}
+
+/* Fetches the current lock state, which may already be on at probe/resume. */
+static void lg_g15_v2_kbd_read_super_lock(struct hid_device *hdev,
+					  struct lg_g15_kbd_data *kbd)
+{
+	u8 *buf;
+	int ret;
+
+	buf = kzalloc(2, GFP_KERNEL); /* DMA-able, not on the stack */
+	if (!buf)
+		return;
+
+	ret = hid_hw_raw_request(hdev, LG_G15_V2_KBD_GAME_MODE_REPORT, buf, 2,
+				 HID_INPUT_REPORT, HID_REQ_GET_REPORT);
+	if (ret == 2 && buf[0] == LG_G15_V2_KBD_GAME_MODE_REPORT)
+		lg_g15_v2_kbd_set_super_lock(kbd, buf[1] & 0x01);
+	else
+		hid_dbg(hdev, "Error reading Super-key-lock state: %d\n", ret);
+
+	kfree(buf);
+}
+
+/* Decodes report 3 and mirrors the Super-key-lock bit into the game_mode LED. */
+static int lg_g15_v2_kbd_raw_event(struct hid_device *hdev, u8 *data, int size)
+{
+	struct lg_g15_kbd_data *kbd = hid_get_drvdata(hdev);
+
+	if (!kbd || data[0] != LG_G15_V2_KBD_GAME_MODE_REPORT || size != 2)
+		return 0; /* Not our report, or the non-consumer-control interface */
+
+	lg_g15_v2_kbd_set_super_lock(kbd, data[1] & 0x01);
+
+	return 0;
+}
+
+/*
+ * The G15 v2's keyboard USB device exposes two HID interfaces of its own
+ * (boot keyboard + consumer control); both match this same USB product ID,
+ * so this probe runs once per interface. Only the consumer-control one
+ * carries report 3, so the LED is only registered there; the other gets
+ * plain generic hid-input and nothing else, same as if hid-generic had
+ * bound it.
+ */
+static int lg_g15_v2_kbd_probe(struct hid_device *hdev)
+{
+	struct hid_report_enum *rep_enum;
+	struct hid_report *rep;
+	bool has_game_mode_report = false;
+	struct lg_g15_kbd_data *kbd;
+	int ret;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
+	list_for_each_entry(rep, &rep_enum->report_list, list) {
+		if (rep->id == LG_G15_V2_KBD_GAME_MODE_REPORT)
+			has_game_mode_report = true; /* This is the consumer-control interface */
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT); /* Plain generic keyboard input */
+	if (ret)
+		return ret;
+
+	if (!has_game_mode_report)
+		return 0; /* Boot-keyboard interface, nothing more to do */
+
+	kbd = devm_kzalloc(&hdev->dev, sizeof(*kbd), GFP_KERNEL);
+	if (!kbd)
+		return 0; /* Non-fatal, keyboard input still works without the LED */
+
+	kbd->led.name = "g15::game_mode";
+	kbd->led.max_brightness = 1;
+	kbd->led.brightness_get = lg_g15_v2_kbd_super_lock_get;
+	kbd->led.flags = LED_BRIGHT_HW_CHANGED; /* Enables the hw_changed notify below */
+
+	ret = devm_led_classdev_register(&hdev->dev, &kbd->led);
+	if (ret)
+		return 0; /* Non-fatal, keyboard input still works without the LED */
+
+	/*
+	 * Only set drvdata once the LED is registered: raw_event may run as
+	 * soon as hid_hw_start() returns, and notifying a brightness change
+	 * on an unregistered LED triggers a WARN.
+	 */
+	hid_set_drvdata(hdev, kbd);
+	lg_g15_v2_kbd_read_super_lock(hdev, kbd);
+
+	return 0;
+}
+
 static int lg_g15_raw_event(struct hid_device *hdev, struct hid_report *report,
 			    u8 *data, int size)
 {
-	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
+	struct lg_g15_data *g15;
+
+	if (lg_g15_is_v2_kbd(hdev)) /* Separate kbd/consumer-ctrl dev */
+		return lg_g15_v2_kbd_raw_event(hdev, data, size);
 
+	g15 = hid_get_drvdata(hdev);
 	if (!g15)
 		return 0;
 
@@ -1015,6 +1156,8 @@ static int lg_g15_raw_event(struct hid_device *hdev, struct hid_report *report,
 		if (data[0] == LG_G510_INPUT_KBD_BACKLIGHT && size == 2)
 			return lg_g510_leds_event(g15, data);
 		break;
+	case LG_G15_V2_KBD:
+		break; /* Never reached, handled via lg_g15_is_v2_kbd() above */
 	}
 
 	return 0;
@@ -1417,6 +1560,9 @@ static int lg_g15_register_led(struct lg_g15_data *g15, int i, const char *name)
 			ret = devm_led_classdev_register(&g15->hdev->dev, &g15->leds[i].cdev);
 		}
 		break;
+	case LG_G15_V2_KBD:
+		ret = -EINVAL; /* Never reached, this model has no lg_g15_data LEDs */
+		break;
 	}
 
 	return ret;
@@ -1492,6 +1638,12 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	struct hid_report *rep;
 	int ret, i, gkeys = 0;
 
+	if (lg_g15_is_v2_kbd(hdev)) /* Not the GamePanel LCD interface */
+		return lg_g15_v2_kbd_probe(hdev);
+
+	if (id->driver_data == LG_G15_V2_KBD) /* e.g. a mismatching new_id */
+		return -ENODEV;
+
 	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
 
 	ret = hid_parse(hdev);
@@ -1574,6 +1726,8 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	case LG_Z10:
 		connect_mask = HID_CONNECT_HIDRAW;
 		break;
+	case LG_G15_V2_KBD:
+		break; /* Never reached, rejected above */
 	}
 
 	ret = hid_hw_start(hdev, connect_mask);
@@ -1732,6 +1886,16 @@ static const struct hid_device_id lg_g15_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
 			 USB_DEVICE_ID_LOGITECH_G15_V2_LCD),
 		.driver_data = LG_G15_V2 },
+	/*
+	 * The G15 v2's keyboard + consumer-control USB device (a separate
+	 * device from the GamePanel LCD one above, on the keyboard's builtin
+	 * hub). Normal typing is handled by generic hid-input; we only bind
+	 * this to also expose the Super/Windows-key-lock ("Game Mode") state,
+	 * see lg_g15_v2_kbd_probe().
+	 */
+	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
+			 USB_DEVICE_ID_LOGITECH_G15_V2_KBD),
+		.driver_data = LG_G15_V2_KBD },
 	/* G510 without a headset plugged in */
 	{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
 			 USB_DEVICE_ID_LOGITECH_G510),
@@ -1750,7 +1914,18 @@ MODULE_DEVICE_TABLE(hid, lg_g15_devices);
 
 static void lg_g15_remove(struct hid_device *hdev)
 {
-	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
+	struct lg_g15_data *g15;
+
+	/*
+	 * The G15 v2's keyboard device uses struct lg_g15_kbd_data, not
+	 * lg_g15_data below; it has nothing to clean up beyond hid_hw_stop().
+	 */
+	if (lg_g15_is_v2_kbd(hdev)) {
+		hid_hw_stop(hdev);
+		return;
+	}
+
+	g15 = hid_get_drvdata(hdev);
 
 	/*
 	 * g15->work is only initialized for the models that schedule it
@@ -1772,9 +1947,18 @@ static void lg_g15_remove(struct hid_device *hdev)
  */
 static int lg_g15_resume(struct hid_device *hdev)
 {
-	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
+	struct lg_g15_data *g15;
 	struct lg_g15_led *contrast;
 
+	if (lg_g15_is_v2_kbd(hdev)) {
+		struct lg_g15_kbd_data *kbd = hid_get_drvdata(hdev);
+
+		if (kbd)
+			lg_g15_v2_kbd_read_super_lock(hdev, kbd);
+		return 0;
+	}
+
+	g15 = hid_get_drvdata(hdev);
 	if (!g15)
 		return 0;
 
-- 
2.53.0

Re: [PATCH 3/4] HID: lg-g15: report Super-key-lock state on G15 v2's keyboard interface

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] HID: lg-g15: missing HID_QUIRK_INPUT_PER_APP in
  lg_g15_v2_kbd_probe()
- [Medium] HID: lg-g15: initial lock state read after LED registration
  in lg_g15_v2_kbd_probe()
- [Low] HID: lg-g15: race updating super_lock between lg_g15_resume()
  and raw_event
--

commit 429a4d981b469fb0f361faf62d0245c19b75257d
Author: Torstein Eide <torsteine+linux@gmail.com>

HID: lg-g15: report Super-key-lock state on G15 v2's keyboard interface

This commit binds the Logitech G15 v2 keyboard USB device (046D:C226) to
hid-lg-g15 and exposes the Super-key lock state from report 3 as a
read-only g15::game_mode LED while keeping default hid-input handling.

> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 35d183787383a..592c9747738e2 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
[ ... ]
> @@ -981,11 +995,138 @@ static int lg_g510_leds_event(struct lg_g15_data *g15, u8 *data)
[ ... ]
> +static void lg_g15_v2_kbd_set_super_lock(struct lg_g15_kbd_data *kbd, bool super_lock)
> +{
> +	if (super_lock == kbd->super_lock)
> +		return;
> +
> +	kbd->super_lock = super_lock;
> +	led_classdev_notify_brightness_hw_changed(&kbd->led, super_lock);
> +}
> +
> +/* Fetches the current lock state, which may already be on at probe/resume. */
> +static void lg_g15_v2_kbd_read_super_lock(struct hid_device *hdev,
> +					  struct lg_g15_kbd_data *kbd)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kzalloc(2, GFP_KERNEL); /* DMA-able, not on the stack */
> +	if (!buf)
> +		return;
> +
> +	ret = hid_hw_raw_request(hdev, LG_G15_V2_KBD_GAME_MODE_REPORT, buf, 2,
> +				 HID_INPUT_REPORT, HID_REQ_GET_REPORT);
> +	if (ret == 2 && buf[0] == LG_G15_V2_KBD_GAME_MODE_REPORT)
> +		lg_g15_v2_kbd_set_super_lock(kbd, buf[1] & 0x01);
> +	else
> +		hid_dbg(hdev, "Error reading Super-key-lock state: %d\n", ret);
> +
> +	kfree(buf);
> +}
[ ... ]
> +	kbd->led.name = "g15::game_mode";
> +	kbd->led.max_brightness = 1;
> +	kbd->led.brightness_get = lg_g15_v2_kbd_super_lock_get;
> +	kbd->led.flags = LED_BRIGHT_HW_CHANGED; /* Enables the hw_changed notify below */
> +
> +	ret = devm_led_classdev_register(&hdev->dev, &kbd->led);
> +	if (ret)
> +		return 0; /* Non-fatal, keyboard input still works without the LED */
> +
> +	/*
> +	 * Only set drvdata once the LED is registered: raw_event may run as
> +	 * soon as hid_hw_start() returns, and notifying a brightness change
> +	 * on an unregistered LED triggers a WARN.
> +	 */
> +	hid_set_drvdata(hdev, kbd);
> +	lg_g15_v2_kbd_read_super_lock(hdev, kbd);

[Severity: Medium]
Does registering kbd->led before reading the initial hardware lock state
publish a stale brightness of 0 and emit a spurious brightness_hw_changed
notification when the Super-key lock switch is already on at probe?

Inside devm_led_classdev_register(), led_classdev_register_ext() calls
kbd->led.brightness_get while kbd->super_lock is still false, caches
kbd->led.brightness = 0, initializes kbd->led.brightness_hw_changed to -1
(-ENODATA), and emits KOBJ_ADD.  Userspace readers reacting to the uevent
can observe 0 while the GET_REPORT transfer is in flight, and
lg_g15_v2_kbd_read_super_lock() -> lg_g15_v2_kbd_set_super_lock() then fires
led_classdev_notify_brightness_hw_changed() and sets brightness_hw_changed
to 1 even though no hardware change occurred after registration.

Reading the initial report into kbd->super_lock before calling
devm_led_classdev_register() (similar to lg_g15_get_initial_led_brightness()
in lg_g15_probe()) would register the LED with the actual hardware state.

[ ... ]

> @@ -1492,6 +1638,12 @@ static int lg_g15_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	struct hid_report *rep;
>  	int ret, i, gkeys = 0;
>  
> +	if (lg_g15_is_v2_kbd(hdev)) /* Not the GamePanel LCD interface */
> +		return lg_g15_v2_kbd_probe(hdev);
> +
> +	if (id->driver_data == LG_G15_V2_KBD) /* e.g. a mismatching new_id */
> +		return -ENODEV;
> +
>  	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;

[Severity: Medium]
Should lg_g15_v2_kbd_probe() also set HID_QUIRK_INPUT_PER_APP on
hdev->quirks before calling hid_parse() and hid_hw_start()?

Previously 046D:C226 was bound by hid-generic, where hid_generic_probe()
sets HID_QUIRK_INPUT_PER_APP:

drivers/hid/hid-generic.c:hid_generic_probe() {
	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;

	ret = hid_parse(hdev);
	if (ret)
		return ret;

	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
}

Because lg_g15_probe() dispatches to lg_g15_v2_kbd_probe() before setting
HID_QUIRK_INPUT_PER_APP, any interface on 046D:C226 with multiple
application collections (such as the consumer-control/extra-report
interface) has all application collections merged into a single input_dev
and loses the per-application input device name suffix, which is a
regression relative to hid-generic.

[ ... ]

> @@ -1772,9 +1947,18 @@ static void lg_g15_remove(struct hid_device *hdev)
>   */
>  static int lg_g15_resume(struct hid_device *hdev)
>  {
> -	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +	struct lg_g15_data *g15;
>  	struct lg_g15_led *contrast;
>  
> +	if (lg_g15_is_v2_kbd(hdev)) {
> +		struct lg_g15_kbd_data *kbd = hid_get_drvdata(hdev);
> +
> +		if (kbd)
> +			lg_g15_v2_kbd_read_super_lock(hdev, kbd);
> +		return 0;
> +	}

[Severity: Low]
Can lg_g15_resume() race with lg_g15_v2_kbd_raw_event() when updating
kbd->super_lock and kbd->led.brightness_hw_changed?

In usbhid, hid_resume_common() and hid_post_reset() call hid_restart_io() to
restart the interrupt IN URB before invoking the HID driver's resume or
reset_resume callback.  If the Super-key lock switch is toggled during
resume, the GET_REPORT control transfer and an interrupt IN report can run
concurrently:

CPU0 (resume)                     CPU1 (URB completion)
lg_g15_resume()
  lg_g15_v2_kbd_read_super_lock()
    hid_hw_raw_request()
      -> reads old state (0)
                                  lg_g15_raw_event()
                                    lg_g15_v2_kbd_raw_event()
                                      lg_g15_v2_kbd_set_super_lock(kbd, 1)
                                        kbd->super_lock = 1;
                                        ..._hw_changed(&kbd->led, 1);
    lg_g15_v2_kbd_set_super_lock(kbd, 0)
      kbd->super_lock = 0;  <-- overwrites newer interrupt state

The two paths can also interleave between assigning kbd->super_lock and
calling led_classdev_notify_brightness_hw_changed(), leaving kbd->super_lock
and kbd->led.brightness_hw_changed with opposite values.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094426.1432465-1-torsteine+linux@gmail.com?part=3

[PATCH 4/4] HID: lg-g15: cancel LED work only after stopping the hardware

[Torstein Eide]

lg_g15_remove() cancels g15->work before calling hid_hw_stop(). In
between, a backlight or game-mode key press makes raw_event call
schedule_work() again. The work then runs after remove() has returned
and devm has freed g15, which is a use-after-free in
lg_g15_leds_changed_work().

Cancel the work after hid_hw_stop(), once no more raw events can arrive.

Fixes: 7705b4140d18 ("HID: lg-g15: cancel pending work on remove to fix a use-after-free")
Signed-off-by: Torstein Eide <torsteine+linux@gmail.com>
Assisted-by: Claude:claude-opus-5-5
---
 drivers/hid/hid-lg-g15.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
index 592c9747738e..a341b0aca249 100644
--- a/drivers/hid/hid-lg-g15.c
+++ b/drivers/hid/hid-lg-g15.c
@@ -1927,18 +1927,19 @@ static void lg_g15_remove(struct hid_device *hdev)
 
 	g15 = hid_get_drvdata(hdev);
 
+	if (g15)
+		lg_g15_unregister_fb(g15);
+
+	hid_hw_stop(hdev);
+
 	/*
 	 * g15->work is only initialized for the models that schedule it
 	 * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
-	 * cancel it when it was set up.
+	 * cancel it when it was set up. Cancel it after hid_hw_stop(), as
+	 * until then raw_event can still schedule it again.
 	 */
 	if (g15 && g15->work.func)
 		cancel_work_sync(&g15->work);
-
-	if (g15)
-		lg_g15_unregister_fb(g15);
-
-	hid_hw_stop(hdev);
 }
 
 /*
-- 
2.53.0

Re: [PATCH 4/4] HID: lg-g15: cancel LED work only after stopping the hardware

[Hans de Goede]

Hi,

On 9-Nov-26 10:44, Torstein Eide wrote:
> lg_g15_remove() cancels g15->work before calling hid_hw_stop(). In
> between, a backlight or game-mode key press makes raw_event call
> schedule_work() again. The work then runs after remove() has returned
> and devm has freed g15, which is a use-after-free in
> lg_g15_leds_changed_work().
> 
> Cancel the work after hid_hw_stop(), once no more raw events can arrive.

This is not necessary the hid-core avoids the raw_event callback getting
called during remove() by hid_device_remove() doing:

	down(&hdev->driver_input_lock);

before calling the drivers remove callback.

Regards,

Hans




> 
> Fixes: 7705b4140d18 ("HID: lg-g15: cancel pending work on remove to fix a use-after-free")
> Signed-off-by: Torstein Eide <torsteine+linux@gmail.com>
> Assisted-by: Claude:claude-opus-5-5
> ---
>  drivers/hid/hid-lg-g15.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-lg-g15.c b/drivers/hid/hid-lg-g15.c
> index 592c9747738e..a341b0aca249 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
> @@ -1927,18 +1927,19 @@ static void lg_g15_remove(struct hid_device *hdev)
>  
>  	g15 = hid_get_drvdata(hdev);
>  
> +	if (g15)
> +		lg_g15_unregister_fb(g15);
> +
> +	hid_hw_stop(hdev);
> +
>  	/*
>  	 * g15->work is only initialized for the models that schedule it
>  	 * (G15, G15 v2, G510). The G13 and Z-10 leave it zeroed, so only
> -	 * cancel it when it was set up.
> +	 * cancel it when it was set up. Cancel it after hid_hw_stop(), as
> +	 * until then raw_event can still schedule it again.
>  	 */
>  	if (g15 && g15->work.func)
>  		cancel_work_sync(&g15->work);
> -
> -	if (g15)
> -		lg_g15_unregister_fb(g15);
> -
> -	hid_hw_stop(hdev);
>  }
>  
>  /*

Re: [PATCH 0/4] HID: lg-g15: G15 v2 LCD and game-mode support

[Hans de Goede]

Hi,

+To: Benjamin Valentin <benpicco@googlemail.com>

Who also just submitted a G15 LCD HID framebuffer driver:

https://lore.kernel.org/linux-input/20261108173920.424072-1-benpicco@googlemail.com/

On 9-Nov-26 10:44, Torstein Eide wrote:
> This series adds support for more of the Logitech G15 v2 hardware to
> hid-lg-g15, plus a fix for the remove path.
> 
> Patch 1 exposes the GamePanel LCD contrast as a g15::lcd_contrast LED.
> 
> Patch 2 adds an optional fbdev framebuffer (HID_LOGITECH_G15_FB) for
> the 160x43 monochrome LCD, updated through deferred I/O, so userspace
> can draw on the LCD or bind fbcon to it without libg15/g15daemon.

Ok, so both of you are trying to turn this into a /dev/fb# device, which
I can understand but I'm not sure this is the best idea.

The /dev/fb# API is sorta deprecated in favor of the /dev/dri/card#
kernel-mode-setting (KMS) interface. Note this is not an invitation to
turn this into a KMS driver instead. Please do NOT tell your LLM agent
to turn this into a KMS driver.

Regardless of /dev/fb# vs KMS the problem is that userspace code
like GNOME/KDE will try to treat this as an actual display / an extra
monitor and that userspace code will now need some special code to not
treat it as such and may break without special handling added to it.

And we *never* break userspace so using a userspace API which is
intended for normal (extra) monitor / LCD panel  display output
is not really appropriate IMHO.

Which brings us to the question, why handle this in the kernel at
all. I realize that libg15/g15daemon are no longer maintained and
are obsolete. This is part of why I wrote the drivers/hid/hid-lg-g15.c
driver to move handling of the key-presses into the kernel.

But while handling key-presses is typically always done in the kernel
so far these kind of small status displays (again not a full blown
monitor / display output) has typically been done in userspace.

There are several projects for this, e.g. lcdproc, which is still
actively developed on github:

https://github.com/lcdproc/lcdproc

This works more or less out of the box with the G15 display, it does
still rely on the libg15render library. But that can easily be fixed
as shown by these kernel patches which in essence integrate a copy of
the relevant libg15render bits.

So maybe instead of writing a kernel driver you can write a PR for
lcdproc to remove the dependency on the unmaintained libg15render,
instead folding the render bits directly into the lcdproc code.

Note that on Fedora (you can try by booting a F45 livecd) after
installing lcdprod "sudo dnf install lcdproc" just replugging
the G15 keyboard will trigger a udev script which automatically
creates a lcdproc conf file to use the keyboard if no lcdproc.conf
exists yet. So this pretty much works plug and play without any
kernel changes...

> Patch 3 binds the G15 v2's second interface (046D:C226), which is
> currently handled by hid-generic, and exposes the Super-key-lock
> ("Game Mode") state as a read-only g15::game_mode LED. Normal key
> input on that interface is left to hid-input.

Patches 1 and 3 do sound potentially interesting although I wonder
how these interact with e.g. the existing lcdproc support which
uses this interface directly in hidraw mode.

Note again we never break userspace. So existing lcdproc setups
must keep working with these changes.

> Patch 4 fixes a use-after-free on remove: the LED work can be
> rescheduled by a key press between cancel_work_sync() and
> hid_hw_stop(). The bug predates this series, but the patch is
> written against the remove path as changed by patches 1-3, so
> it does not apply to the current tree on its own.

I'll reply to this patch seperately.

Regards,

Hans


> 
> The LCD wire format and contrast commands follow libg15, hence the
> Based-on-work-by tags for its authors.
> 
> All patches were tested on a Logitech G15 v2.
> 
> Torstein Eide (4):
>   HID: lg-g15: add LCD contrast LED
>   HID: lg-g15: add LCD pixel-content framebuffer
>   HID: lg-g15: report Super-key-lock state on G15 v2's keyboard
>     interface
>   HID: lg-g15: cancel LED work only after stopping the hardware
> 
>  drivers/hid/Kconfig      |  13 +
>  drivers/hid/hid-ids.h    |   1 +
>  drivers/hid/hid-lg-g15.c | 599 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 606 insertions(+), 7 deletions(-)
>