Re: [PATCH] HID: lg-g15: Add G15 LCD framebuffer support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: lg-g15: missing hid_is_usb() check in lg_g15_fb_init()
- [Medium] HID: lg-g15: reversed horizontal pixel bit order in
  lg_g15_pack_report()
- [Medium] HID: lg-g15: LCD updates dropped during USB runtime
  autosuspend
--

commit 12c2a94ad75eaba461660e9c5ba7c7edbfce9af1
Author: Benjamin Valentin <benpicco@googlemail.com>

HID: lg-g15: Add G15 LCD framebuffer support

This commit adds deferred-I/O framebuffer support for the 160x43 monochrome
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 02ef3e2094b4c..ca1f3280c05a6 100644
> --- a/drivers/hid/hid-lg-g15.c
> +++ b/drivers/hid/hid-lg-g15.c
[ ... ]
> +static void lg_g15_pack_report(u8 *report, const u8 *pixels)
> +{
> +	unsigned int page, x, bit, y;
> +	u8 value, mask;
> +
> +	memset(report, 0, LG_G15_LCD_HEADER_SIZE);
> +	report[0] = 0x03;
> +	report += LG_G15_LCD_HEADER_SIZE;
> +
> +	/*
> +	 * The LCD consumes vertical, LSB-at-the-top groups of eight pixels.
> +	 * Its pixel polarity is opposite to FB_VISUAL_MONO10.
> +	 */
> +	for (page = 0; page < LG_G15_LCD_PAGES; page++) {
> +		for (x = 0; x < LG_G15_LCD_WIDTH; x++) {
> +			mask = BIT(7 - x % 8);

[Severity: Medium]
Should mask be BIT(x % 8) instead of BIT(7 - x % 8)?

Standard 1bpp packed-pixel sysmem framebuffers (with nonstd = 0) place the
leftmost pixel of each byte (x % 8 == 0) in BIT(0) on little-endian
architectures, as used by sys_imageblit(), sys_fillrect(), sys_copyarea(),
and samples/auxdisplay/cfag12864b-example.c.  Testing BIT(7 - x % 8) mirrors
every 8-pixel horizontal byte on the LCD when drawn via standard fbdev
operations.

[ ... ]
> +static int lg_g15_fb_init(struct hid_device *hdev, struct lg_g15_data *g15)
> +{
> +	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> +	struct usb_host_interface *alt = intf->cur_altsetting;
> +	struct usb_endpoint_descriptor *out;
> +	struct fb_info *info;
> +	struct lg_g15_fb *fb;
> +	int ret;
> +
> +	ret = usb_find_int_out_endpoint(alt, &out);

[Severity: High]
Does lg_g15_fb_init() need to check hid_is_usb(hdev) before casting
hdev->dev.parent with to_usb_interface()?

A virtual HID device created via /dev/uhid with bus = BUS_USB and a G15 LCD
product ID passes all earlier transport-agnostic HID calls in
lg_g15_probe(), causing to_usb_interface(hdev->dev.parent) to cast a
non-USB parent device and dereference an invalid intf->cur_altsetting
pointer.

[ ... ]
> +static int lg_g15_suspend(struct hid_device *hdev, pm_message_t message)
> +{
> +	struct lg_g15_data *g15 = hid_get_drvdata(hdev);
> +	struct lg_g15_fb *fb;
> +
> +	if (!g15 || !g15->fb)
> +		return 0;
> +
> +	fb = g15->fb;
> +	mutex_lock(&fb->lock);
> +	fb->suspended = true;
> +	mutex_unlock(&fb->lock);
> +	flush_delayed_work(&fb->info->deferred_work);
> +	return 0;
> +}

[Severity: Medium]
Does setting fb->suspended during runtime autosuspend cause
lg_g15_fb_deferred_io() to drop all subsequent LCD updates?

Because usbhid enables autosuspend and G15 / G15 v2 devices do not connect
hidinput LEDs, the USB interface can runtime-suspend while no G-keys are
pressed.  lg_g15_suspend() then sets fb->suspended = true even when
PMSG_IS_AUTO(message) is true, and lg_g15_fb_deferred_io() bails out
instead of waking the interface (for example via
usb_autopm_get_interface() / usb_autopm_put_interface()).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108173920.424072-1-benpicco@googlemail.com?part=1