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

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 [off-list ref]
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.

[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.

[ ... ]

[ ... ]

[ ... ]

[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