Re: [PATCH 2/2] bus: Remove redundant dev_err()/dev_err_probe()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unhandled negative error code from platform_get_irq() in omap_l3_noc is passed directly to devm_request_irq(), masking the original error and breaking probe deferral.
- [High] IRQ is requested and enabled before hardware clock and reset initialization in sunxi-rsb, risking synchronous bus aborts.
- [High] The IRQ handler in sunxi-rsb remains registered after the device's clock is disabled during driver removal, risking a fatal crash on spurious interrupts.
- [Medium] Use of a static struct pointer in the probe function of omap_l3_noc leads to a race condition and data corruption on concurrent device probes.
--

--- Patch [2]: [PATCH 2/2] bus: Remove redundant dev_err()/dev_err_probe() ---
commit 78c3bde11117dc0a88b0af99a88dd1bbb4e58713
Author: Pan Chuang <panchuang@vivo.com>
    
bus: Remove redundant dev_err()/dev_err_probe()
    
Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() and dev_err_probe() calls.
    
> diff --git a/drivers/bus/omap_l3_noc.c b/drivers/bus/omap_l3_noc.c
> index eb1ba6319fdaf..45f3f793976c5 100644
> --- a/drivers/bus/omap_l3_noc.c
> +++ b/drivers/bus/omap_l3_noc.c
> @@ -278,17 +278,12 @@ static int omap_l3_probe(struct platform_device *pdev)
>  	l3->debug_irq = platform_get_irq(pdev, 0);
>  	ret = devm_request_irq(l3->dev, l3->debug_irq, l3_interrupt_handler,
>  			       IRQF_NO_THREAD, "l3-dbg-irq", l3);

[Severity: Medium]
This isn't a bug introduced by this patch, but could the use of the l3 pointer
here lead to cross-linked interrupts or data corruption on concurrent probes?

Looking at omap_l3_probe(), the l3 pointer is declared as a static struct:

static int omap_l3_probe(struct platform_device *pdev)
{
        ...
        static struct omap_l3 *l3;
        ...
        l3 = devm_kzalloc(&pdev->dev, sizeof(*l3), GFP_KERNEL);

If two instances are probed concurrently, one thread's allocation could
overwrite the other's before it is passed to devm_request_irq().

[Severity: High]
This is also a pre-existing issue, but does passing an unhandled negative
error code from platform_get_irq() directly into devm_request_irq() mask
probe deferral errors?

If platform_get_irq() returns -EPROBE_DEFER, passing it as an unsigned int
to devm_request_irq() will result in a permanent -EINVAL failure. Since
this patch causes devm_request_irq() to log the -EINVAL on failure, this
could also unnecessarily log errors in the kernel during probe deferral.

> -	if (ret) {
> -		dev_err(l3->dev, "request_irq failed for %d\n",
> -			l3->debug_irq);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	l3->app_irq = platform_get_irq(pdev, 1);
>  	ret = devm_request_irq(l3->dev, l3->app_irq, l3_interrupt_handler,
>  			       IRQF_NO_THREAD, "l3-app-irq", l3);
> -	if (ret)
> -		dev_err(l3->dev, "request_irq failed for %d\n", l3->app_irq);
>  
>  	return ret;
>  }
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index daf0ea563e477..671c9b33e783a 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -787,8 +787,7 @@ static int sunxi_rsb_probe(struct platform_device *pdev)
>  
>  	ret = devm_request_irq(dev, irq, sunxi_rsb_irq, 0, RSB_CTRL_NAME, rsb);
>  	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "can't register interrupt handler irq %d\n", irq);
> +		return ret;
>  
>  	ret = sunxi_rsb_hw_init(rsb);

[Severity: High]
This is a pre-existing issue, but does registering the IRQ handler before
sunxi_rsb_hw_init() enables the clock and clears the reset risk a
synchronous external abort?

If an interrupt is already pending from a previous boot stage, the
sunxi_rsb_irq handler will execute immediately. Because the clocks are
gated and the device is in reset, reading the device registers inside the
handler could crash the system.

[Severity: High]
This is another pre-existing issue related to the devm_request_irq() usage.
Does the IRQ handler remain registered after the device's clock is manually
disabled during driver removal?

Because sunxi_rsb_remove() manually disables the hardware by calling
sunxi_rsb_hw_exit(), but relies on devres to free the interrupt handler
afterward, a spurious interrupt firing in that window could cause a crash
when the handler attempts to access gated device registers.

>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710110930.462109-1-panchuang@vivo.com?part=2