Re: [PATCH v3 06/13] media: sunxi-cir: Ensure no more interrupts can occur before free

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

Pre-existing issues:
- [High] The patch attempts to fix a race condition by reordering `rc_free_device()` after `sunxi_ir_hw_exit()`, but fails to synchronize or explicitly disable the devm-managed IRQ, leaving the race open.
- [High] The device is registered with the RC core before its hardware clocks are enabled, resulting in MMIO accesses to unclocked registers during probe.
--

--- Patch [6]: [PATCH v3 06/13] media: sunxi-cir: Ensure no more interrupts can occur before free ---
commit a8d7020d0e53fc07953c4088225c769898287e76
Author: Sean Young <[email protected]>

media: sunxi-cir: Ensure no more interrupts can occur before free

This commit reorders rc_free_device() in the remove function so that it is
called after the hardware is shut down. This is intended to fix a race
between device unregistration and the interrupt callback.

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 28e840a7e5b8c..af1ee08ffdbec 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -374,8 +374,8 @@ static void sunxi_ir_remove(struct platform_device *pdev)
>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>  
>  	rc_unregister_device(ir->rc);
> -	rc_free_device(ir->rc);
>  	sunxi_ir_hw_exit(&pdev->dev);
> +	rc_free_device(ir->rc);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this actually close the
race condition?

In sunxi_ir_remove(), sunxi_ir_hw_exit() disables the hardware clocks.
However, if the devm-managed interrupt is not explicitly disabled and
synchronized (for example, using disable_irq()), couldn't an interrupt
still run concurrently or immediately after clocks are disabled?

If sunxi_ir_irq() fires after sunxi_ir_hw_exit() runs, it will attempt to
read SUNXI_IR_RXSTA_REG while the clocks are disabled, which could cause a
crash. Additionally, since the IRQ is devm-managed, it could also race with
rc_free_device(), leading to a use-after-free on ir->rc.

This same issue appears to exist in sunxi_ir_suspend(), where clocks are
disabled while system interrupts are still active.

[Severity: High]
This is a pre-existing issue, but I noticed another potential problem in
the probe path. Does sunxi_ir_probe() access unclocked registers during
device registration?

In sunxi_ir_probe(), rc_register_device() is called before sunxi_ir_hw_init():

	ret = rc_register_device(ir->rc);
	if (ret) {
		dev_err(dev, "failed to register rc device\n");
		goto exit_free_dev;
	}
...
	ret = sunxi_ir_hw_init(dev);

The registration synchronously invokes core callbacks which unconditionally
call the driver's protocol change hook, and in turn sunxi_ir_set_timeout().
This callback writes to the SUNXI_IR_CIR_REG MMIO register. Since ir->clk
and ir->apb_clk are not enabled until sunxi_ir_hw_init() runs later, won't
this cause an abort or bus fault by accessing APB memory-mapped registers
without their clocks enabled?

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