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

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

Pre-existing issues:
- [High] Double unregister of the mailbox controller on the IRQ request error path due to mixing devres with manual cleanup.
--

--- Patch [1]: [PATCH] mailbox: Remove redundant dev_err()/dev_err_probe() ---
commit 4d3272d7b1e42964746554b2fa5d303d597704a7
Author: Pan Chuang <[email protected]>

mailbox: Remove redundant dev_err()/dev_err_probe()

This commit removes redundant driver-specific dev_err() and dev_err_probe()
calls after devm_request_irq() and devm_request_threaded_irq(), since those
functions automatically log detailed error messages on failure now.

> diff --git a/drivers/mailbox/arm_mhu_db.c b/drivers/mailbox/arm_mhu_db.c
> index 9e937b09c5fbc..a39239e38a479 100644
> --- a/drivers/mailbox/arm_mhu_db.c
> +++ b/drivers/mailbox/arm_mhu_db.c
> @@ -318,7 +318,6 @@ static int mhu_db_probe(struct amba_device *adev, const struct amba_id *id)
>  						mhu_db_mbox_rx_handler,
>  						IRQF_ONESHOT, "mhu_db_link", mhu);
>  		if (err) {
> -			dev_err(dev, "Can't claim IRQ %d\n", irq);
>  			mbox_controller_unregister(&mhu->mbox);

[Severity: High]
This is a pre-existing issue, but since this patch touches the error handling
path, does calling mbox_controller_unregister() manually on a devm-managed
controller cause list corruption?

Looking at mhu_db_probe(), the mailbox controller is registered using
devm_mbox_controller_register() earlier in the function. If
devm_request_threaded_irq() fails, this explicit unregister is called. 
When the probe returns an error, the devres framework will automatically
unregister it a second time.

Since mbox_controller_unregister() calls list_del() on the mailbox node
without re-initializing it, would a second call dereference poisoned pointers
and cause a kernel panic?

>  			return err;
>  		}

[ ... ]

> diff --git a/drivers/mailbox/qcom-ipcc.c b/drivers/mailbox/qcom-ipcc.c
> index d957d989c0cee..e5ebb4a237ffd 100644
> --- a/drivers/mailbox/qcom-ipcc.c
> +++ b/drivers/mailbox/qcom-ipcc.c
> @@ -323,10 +323,8 @@ static int qcom_ipcc_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, ipcc->irq, qcom_ipcc_irq_fn,
>  			       IRQF_TRIGGER_HIGH | IRQF_NO_SUSPEND |
>  			       IRQF_NO_THREAD, name, ipcc);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "Failed to register the irq: %d\n", ret);
> +	if (ret < 0)
>  		goto err_req_irq;

[Severity: High]
This is also a pre-existing issue on the modified error path. When the
devm_request_irq() call fails, the code jumps to the err_req_irq label:

err_req_irq:
	if (ipcc->num_chans)
		mbox_controller_unregister(&ipcc->mbox);

Since the controller was also registered with devm_mbox_controller_register(),
will devres unregister it again during cleanup, leading to the same double
unregister list corruption mentioned above?

> -	}
>  
>  	platform_set_drvdata(pdev, ipcc);

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