The driver uses devm_spi_register_controller() for registration, which
automatically unregisters the controller via devm cleanup when the
device is removed.

The manual call to spi_unregister_controller() in the remove() callback
is therefore redundant and should be removed.

Fixes: 8011709906d0 ("spi: rockchip-sfc: Support pm ops")
Signed-off-by: Felix Gu <ustc.gu@gmail.com>
---
 drivers/spi/spi-rockchip-sfc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/spi-rockchip-sfc.c b/drivers/spi/spi-rockchip-sfc.c
index 2990bf85ee47..19e832f045d2 100644
--- a/drivers/spi/spi-rockchip-sfc.c
+++ b/drivers/spi/spi-rockchip-sfc.c
@@ -740,9 +740,7 @@ static int rockchip_sfc_probe(struct platform_device *pdev)
 static void rockchip_sfc_remove(struct platform_device *pdev)
 {
 	struct rockchip_sfc *sfc = platform_get_drvdata(pdev);
-	struct spi_controller *host = sfc->host;
 
-	spi_unregister_controller(host);
 	dma_unmap_single(&pdev->dev, sfc->dma_buffer, sfc->max_iosize,
 			 DMA_BIDIRECTIONAL);
 	free_pages((unsigned long)sfc->buffer, get_order(sfc->max_iosize));

---
base-commit: 3f9cd19e764b782706dbaacc69e502099cb014ba
change-id: 20260305-sfc-307708e9dc67

Best regards,
-- 
Felix Gu <ustc.gu@gmail.com>

On Thu, Mar 05, 2026 at 11:38:21PM +0800, Felix Gu wrote:

> The driver uses devm_spi_register_controller() for registration, which
> automatically unregisters the controller via devm cleanup when the
> device is removed.

> The manual call to spi_unregister_controller() in the remove() callback
> is therefore redundant and should be removed.

Not just redundant, it'll be a double free.

> -	spi_unregister_controller(host);
>  	dma_unmap_single(&pdev->dev, sfc->dma_buffer, sfc->max_iosize,
>  			 DMA_BIDIRECTIONAL);
>  	free_pages((unsigned long)sfc->buffer, get_order(sfc->max_iosize));

This does mean that if the device is being force unregistered with
children still present those children might still be submitting SPI
requests while the DMA buffer is unmapped, but in the normal course of
affairs it's more likely that the devices will be unregistered before
the controller.  It would be best to either roll back the use of devm or
add a devm based unmap of the buffer here, the current remove function
has correct ordering.

On Mon, Mar 9, 2026 at 8:29 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Mar 05, 2026 at 11:38:21PM +0800, Felix Gu wrote:
>
> > The driver uses devm_spi_register_controller() for registration, which
> > automatically unregisters the controller via devm cleanup when the
> > device is removed.
>
> > The manual call to spi_unregister_controller() in the remove() callback
> > is therefore redundant and should be removed.
>
> Not just redundant, it'll be a double free.
>
> > -     spi_unregister_controller(host);
> >       dma_unmap_single(&pdev->dev, sfc->dma_buffer, sfc->max_iosize,
> >                        DMA_BIDIRECTIONAL);
> >       free_pages((unsigned long)sfc->buffer, get_order(sfc->max_iosize));
>
> This does mean that if the device is being force unregistered with
> children still present those children might still be submitting SPI
> requests while the DMA buffer is unmapped, but in the normal course of
> affairs it's more likely that the devices will be unregistered before
> the controller.  It would be best to either roll back the use of devm or
> add a devm based unmap of the buffer here, the current remove function
> has correct ordering.

Hi Mark,
Thanks for the review, I will fix it in V2.

Best regards,
Felix