Re: [PATCH 3/3] mmc: davinci: Handle optional IRQ return value correctly

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

New issues:
- [Medium] Direct return on `platform_get_irq_optional` error bypasses clock cleanup, leaking the clock enable reference.

Pre-existing issues:
- [High] The driver manually disables the hardware clock in the remove and probe error paths while devm-managed IRQs are still active, risking unclocked register accesses.
- [High] If DMA descriptor allocation fails, the driver attempts PIO fallback but leaves `host->do_dma` set and leaks the mapped scatterlist.
- [High] The system suspend routine disables the MMC controller clock without masking the SDIO interrupt, risking unclocked register access.
--

--- Patch [3]: [PATCH 3/3] mmc: davinci: Handle optional IRQ return value correctly ---
commit 6f8e3291a8bba48fcd7880ece2584b86bc2ca506
Author: bui duc phuc <[email protected]>

mmc: davinci: Handle optional IRQ return value correctly
    
The commit checks for a positive IRQ number before requesting the SDIO
IRQ, fixing an issue where 0 was treated as a valid IRQ.

> diff --git a/drivers/mmc/host/davinci_mmc.c b/drivers/mmc/host/davinci_mmc.c
> index 4765d2dee5993..dddd152d2ec45 100644
> --- a/drivers/mmc/host/davinci_mmc.c
> +++ b/drivers/mmc/host/davinci_mmc.c
> @@ -1303,7 +1303,7 @@ static int davinci_mmcsd_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto mmc_add_host_fail;
>  
> -	if (host->sdio_irq >= 0) {
> +	if (host->sdio_irq > 0) {

[Severity: Medium]
In davinci_mmcsd_probe(), if platform_get_irq_optional() returns an error:

    ret = platform_get_irq_optional(pdev, 1);
    if (ret < 0 && ret != -ENXIO)
        return ret;

Does this direct return bypass the error handling sequence (e.g., goto
parse_fail) and leak the clock enable reference?


[Severity: High]
This isn't a bug introduced by this patch, but I noticed the driver
manually disables the hardware clock in the remove and probe error paths
while devm-managed IRQs are still active.

In the davinci_mmcsd_probe() error paths and davinci_mmcsd_remove(),
clk_disable_unprepare(host->clk) is called before devres frees the IRQs.

If a spurious or SDIO interrupt fires in this window,
mmc_davinci_sdio_irq() will execute:

    status = readl(host->base + DAVINCI_SDIOIST);

Could this cause a fatal bus fault by reading from memory-mapped
registers while the clock is disabled?


[Severity: High]
This is a pre-existing issue, but I found a potential problem in the DMA
fallback path.

In mmc_davinci_start_dma_transfer():

    host->do_dma = 1;
    ret = mmc_davinci_send_dma_request(host, data);

If dmaengine_prep_slave_sg() fails inside mmc_davinci_send_dma_request(),
it returns an error but leaves host->do_dma set to 1, and does not unmap
the scatterlist mapped via dma_map_sg().

Does this break the PIO fallback since mmc_davinci_start_command() will
still configure the hardware for DMA?


[Severity: High]
This is a pre-existing issue, but the system suspend routine might have a
clock management problem.

In davinci_mmcsd_suspend():

    writel(0, host->base + DAVINCI_MMCIM);
    mmc_davinci_reset_ctrl(host, 1);
    clk_disable(host->clk);

The MMC controller clock is disabled without masking the SDIO interrupt
via DAVINCI_SDIOIEN.

If an SDIO interrupt fires between the device suspend callback and the IRQ
core's suspend_device_irqs() phase, won't mmc_davinci_sdio_irq() attempt
to read DAVINCI_SDIOIST while the clock domain is off?

>  		ret = devm_request_irq(&pdev->dev, host->sdio_irq,
>  				       mmc_davinci_sdio_irq, 0,
>  				       mmc_hostname(mmc), host);

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

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