Re: [PATCH] media: rc: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

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

New issues:
- [Low] Incorrect PM wrapper macro used for the dev_pm_ops structure pointer.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but there is a use-after-free of the RC device on driver removal and probe error paths due to an active devres-managed IRQ handler.
--

--- Patch [1]: [PATCH] media: rc: Convert to DEFINE_SIMPLE_DEV_PM_OPS() ---
commit 5ad013ce47b901f4bd8a53a1c02b222a8df6858a
Author: Triet Hoang <[email protected]>

media: rc: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

Convert the deprecated SIMPLE_DEV_PM_OPS to DEFINE_SIMPLE_DEV_PM_OPS
and pm_sleep_ptr(). This lets us drop the __maybe_unused annotations
from its suspend and resume callbacks, also reduces kernel size in
case CONFIG_PM or CONFIG_PM_SLEEP is disabled.

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 28e840a7e5b8c..7fd48c510143d 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c

[ ... ]

> @@ -422,7 +422,7 @@ static struct platform_driver sunxi_ir_driver = {
>  	.driver = {
>  		.name = SUNXI_IR_DEV,
>  		.of_match_table = sunxi_ir_match,
> -		.pm = &sunxi_ir_pm_ops,
> +		.pm = pm_sleep_ptr(&sunxi_ir_pm_ops),

[Severity: Low]
Does this code use the correct wrapper macro for a dev_pm_ops structure
pointer? Since sunxi_ir_pm_ops is a structure pointer rather than a sleep
callback, should it use pm_ptr() instead of pm_sleep_ptr()?

>  	},
>  };

[Severity: Critical]
This is a pre-existing issue, but could there be a use-after-free of the RC
device on driver removal and probe error paths?

The driver initializes a devres-managed IRQ in sunxi_ir_probe():

	ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);

On probe failure or during sunxi_ir_remove(), the driver manually calls
rc_unregister_device() and rc_free_device(), freeing the RC device memory:

exit_unregister_dev:
	rc_unregister_device(ir->rc);
exit_free_dev:
	rc_free_device(ir->rc);

Because devres cleanup has not yet run, the IRQ remains active. If an
interrupt fires or is already in-flight on another CPU during this window,
would sunxi_ir_irq() dereference the freed ir->rc?

	ir_raw_event_overflow(ir->rc);

Also, could this read from disabled hardware if an interrupt fires after
sunxi_ir_hw_exit() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic