Re: [PATCH] spi: amlogic-spisg: Balance runtime PM and clock cleanup

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without verifying the device's runtime PM state.
- [High] Division by zero in aml_spisg_setup_transfer when bits_per_word is less than 8.
- [Low] The dev_pm_ops structure is unconditionally assigned to the driver structure, violating subsystem guidelines for conditional compilation of PM callbacks.
--

commit 9ad90d104df3bfd91e2fb30632c7e2e3d7ab281c
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: spi: amlogic-spisg: Balance runtime PM and clock cleanup

This commit explicitly manages the core and sclk enable references for the
Amlogic SPISG driver. It checks pm_runtime_set_active() and holds a no-resume
reference before enabling runtime PM, balancing that reference during probe
failure and teardown.

> diff --git a/drivers/spi/spi-amlogic-spisg.c b/drivers/spi/spi-amlogic-spisg.c
> index 1b00f34e9e931..33c5cbf57cb87 100644
> --- a/drivers/spi/spi-amlogic-spisg.c
> +++ b/drivers/spi/spi-amlogic-spisg.c
[ ... ]
> @@ -832,9 +841,13 @@ static int aml_spisg_probe(struct platform_device *pdev)
[ ... ]
> +	/* Clock initialization has already powered up the controller. */
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_enable(dev);
>  
>  	ctlr->num_chipselect = 4;
>  	ctlr->mode_bits = SPI_CPHA | SPI_CPOL | SPI_LSB_FIRST |

[Severity: High]
This is a pre-existing issue, but does failing to initialize
ctlr->bits_per_word_mask here bypass core validation?

Because the mask is 0, __spi_validate_bits_per_word() in the SPI core allows
arbitrary sizes. If a user sets bits_per_word to a value like 4 via the
SPI_IOC_WR_BITS_PER_WORD ioctl, could this trigger a division by zero in
aml_spisg_setup_transfer() when shifting right by 3?

drivers/spi/spi-amlogic-spisg.c:aml_spisg_setup_transfer() {
	...
	block_size = xfer->bits_per_word >> 3;
	blocks = xfer->len / block_size;
	...
}

> @@ -856,22 +869,26 @@ static int aml_spisg_probe(struct platform_device *pdev)
>  	ret = devm_request_irq(&pdev->dev, irq, aml_spisg_irq, 0, NULL, spisg);

[Severity: High]
This is a pre-existing issue, but can spurious IRQs cause a crash here if the
device is suspended?

Looking at aml_spisg_irq(), the handler accesses hardware registers without
verifying the runtime PM state:

drivers/spi/spi-amlogic-spisg.c:aml_spisg_irq() {
	...
	spisg->status = 0;
	regmap_read(spisg->map, SPISG_REG_IRQ_STS, &sts);
	...
}

If the interrupt fires while the device is in RPM_SUSPENDED (where both sclk
and core clocks are disabled), could reading this MMIO register cause a
synchronous external abort or bus hang? Should we use
pm_runtime_get_if_active() before accessing hardware registers in the IRQ
handler?

[ ... ]
> @@ -882,11 +899,15 @@ static void aml_spisg_remove(struct platform_device *pdev)
[ ... ]
>  static int spisg_suspend_runtime(struct device *dev)

[Severity: Low]
This is a pre-existing issue, but is it safe to assign the pm operations
unconditionally?

Looking at the driver definition:

drivers/spi/spi-amlogic-spisg.c:amlogic_spisg_driver {
	...
	.driver = {
		.name = "amlogic-spisg",
		.pm = &amlogic_spisg_pm_ops,
		.of_match_table = aml_spisg_of_match,
	...
}

When CONFIG_PM is disabled, amlogic_spisg_pm_ops and its callbacks remain
linked into the kernel as dead code. Should this be wrapped with the pm_ptr()
macro to avoid wasting memory when power management is disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913043745.35022-1-mhun512@gmail.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic