Re: [PATCH v2 04/11] ARM: s3c: crag6410: switch keypad device to software properties

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Legacy device tree parsing fails because matrix_keypad_parse_properties() overwrites legacy values.
- [Low] pr_err() message lacks trailing newline.
- [Low] S3C64XX_SETUP_KEYPAD Kconfig symbol is dead code but remains selected.

Pre-existing issues:
- [High] Uninitialized variables in samsung_parse_verbose_keymap() generate random keycodes and leak kernel memory.
- [High] Asynchronous data abort occurs due to IRQ registration before PM runtime enablement.
--

--- Patch [4]: [PATCH v2 04/11] ARM: s3c: crag6410: switch keypad device to software properties ---
commit acad872c671c37651ef2ad547b4a44b17438b49e
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: ARM: s3c: crag6410: switch keypad device to software properties

This commit switches the keypad device to use software properties. This change
allows dropping support for platform data from the samsung-keypad driver. The
platform data and related setup code are removed from the machine
initialization.

> diff --git a/arch/arm/mach-s3c/Makefile.s3c64xx b/arch/arm/mach-s3c/Makefile.s3c64xx
> --- a/arch/arm/mach-s3c/Makefile.s3c64xx
> +++ b/arch/arm/mach-s3c/Makefile.s3c64xx
> @@ -32,7 +32,6 @@ obj-y				+= dev-audio-s3c64xx.o
>  obj-$(CONFIG_S3C64XX_SETUP_FB_24BPP)	+= setup-fb-24bpp-s3c64xx.o
>  obj-$(CONFIG_S3C64XX_SETUP_I2C0)	+= setup-i2c0-s3c64xx.o
>  obj-$(CONFIG_S3C64XX_SETUP_I2C1)	+= setup-i2c1-s3c64xx.o
> -obj-$(CONFIG_S3C64XX_SETUP_KEYPAD)	+= setup-keypad-s3c64xx.o

[Severity: Low]
Is the S3C64XX_SETUP_KEYPAD Kconfig symbol dead code now?

Since the file is removed from the Makefile, the config symbol doesn't seem
to compile any code anymore, but it is still selected by MACH_WLF_CRAGG_6410
in Kconfig.s3c64xx.

[ ... ]

> diff --git a/arch/arm/mach-s3c/mach-crag6410.c b/arch/arm/mach-s3c/mach-crag6410.c
> --- a/arch/arm/mach-s3c/mach-crag6410.c
> +++ b/arch/arm/mach-s3c/mach-crag6410.c
[ ... ]
> @@ -192,17 +193,41 @@ static uint32_t crag6410_keymap[] = {
[ ... ]
> +static void __init crag6410_setup_keypad(void)
> +{
> +	struct platform_device *pd;
> +
> +	/* Set all the necessary GPK pins to special-function 3: KP_ROW[x] */
> +	s3c_gpio_cfgrange_nopull(S3C64XX_GPK(8), 2, S3C_GPIO_SFN(3));
> +
> +	/* Set all the necessary GPL pins to special-function 3: KP_COL[x] */
> +	s3c_gpio_cfgrange_nopull(S3C64XX_GPL(0), 6, S3C_GPIO_SFN(3));
> +
> +	pd = platform_device_register_full(&crag6410_keypad_info);
> +	if (IS_ERR(pd))
> +		pr_err("failed to instantiate keypad device");

[Severity: Low]
Would it be better to add a trailing newline to this pr_err() message?

It looks like the kernel log output might become garbled if the registration
fails.

> +}

[Severity: High]
Could this sequence cause a regression where legacy device trees fail to probe?

When reading properties:

drivers/input/keyboard/samsung-keypad.c:samsung_keypad_parse_properties() {
    ...
    error = matrix_keypad_parse_properties(dev, &num_rows, &num_cols);
    ...
}

The matrix_keypad_parse_properties() function unconditionally zeroes the
variables at entry, so if modern properties are absent, the legacy
samsung,keypad-num-rows properties are overwritten and the function returns
-EINVAL.

[Severity: High]
This is a pre-existing issue, but might samsung_parse_verbose_keymap() leak
uninitialized stack memory to userspace?

drivers/input/keyboard/samsung-keypad.c:samsung_parse_verbose_keymap() {
    ...
    u32 row, col, key_code;

    fwnode_property_read_u32(child, "keypad,row", &row);
    ...
    *keymap++ = KEY(row, col, key_code);
    ...
}

If a child node is missing the keypad,row, keypad,column, or linux,code
properties, fwnode_property_read_u32() leaves row, col, and key_code
uninitialized. These stack values are used in the KEY() macro and stored in
the keymap, where userspace can read them via the EVIOCGKEYCODE ioctl.

[Severity: High]
This is a pre-existing issue, but is it possible for an asynchronous data abort
to occur if an IRQ is pending before PM runtime is enabled?

In probe, the IRQ is requested before the device is powered on:

drivers/input/keyboard/samsung-keypad.c:samsung_keypad_probe() {
    ...
    error = devm_request_threaded_irq(&pdev->dev, keypad->irq, NULL,
                                      samsung_keypad_irq, IRQF_ONESHOT,
                                      dev_name(&pdev->dev), keypad);
    ...
}

If an IRQ is pending from the bootloader, the handler runs immediately:

drivers/input/keyboard/samsung-keypad.c:samsung_keypad_irq() {
    ...
    pm_runtime_get_sync(&keypad->pdev->dev);

    do {
        readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
    ...
}

Since the device is unclocked and pm_runtime_get_sync() fails, readl()
accesses unclocked registers and triggers a data abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-samsung-kp-v2-0-3c6ed4c9b3b6@gmail.com?part=4