Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler

From: Mark Brown <broonie@kernel.org>
To: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: f.fainelli@gmail.com, rjui@broadcom.com, sbranden@broadcom.com,
	bcm-kernel-feedback-list@broadcom.com, nsaenz@kernel.org,
	linux-spi@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, jgg@ziepe.ca,
	p.rosenberger@kunbus.com, linux-integrity@vger.kernel.org,
	stable@vger.kernel.org
Subject: Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown handler
Date: Mon, 4 Oct 2021 13:49:21 +0100	[thread overview]
Message-ID: <YVr4USeiIoQJ0Pqh@sirena.org.uk> (raw)
In-Reply-To: <2c4d7115-7a02-f79e-c91b-3c2dd54051b2@gmx.de>

[-- Attachment #1.1: Type: text/plain, Size: 1393 bytes --]

On Sun, Oct 03, 2021 at 05:25:47PM +0200, Lino Sanfilippo wrote:

> I see your point here. So what about narrowing down the shutdown handler
> to only disable the hardware:

> static void bcm2835_spi_shutdown(struct platform_device *pdev)
> {
> 	struct spi_controller *ctlr = platform_get_drvdata(pdev);
> 	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
> 
> 	if (ctlr->dma_tx)
> 		dmaengine_terminate_sync(ctlr->dma_tx);
> 
> 	if (ctlr->dma_rx)
> 		dmaengine_terminate_sync(ctlr->dma_rx);
> 
> 	/* Clear FIFOs, and disable the HW block */
> 	bcm2835_wr(bs, BCM2835_SPI_CS,
> 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
> 
> 	clk_disable_unprepare(bs->clk);
> }

This still leaves a potential race where something (eg, an interrupt
handler) could come in and try to schedule more SPI transfers on the
shut down hardware.  I'm really not sure we can do something that's
totally robust here without also ensuring that all the client drivers
also have effective shutdown implementations (which seems ambitious) or
doing what we have now and unregistering the clients.  I am, however,
wondering if we really need the shutdown callback at all - the commit
adding it just describes what it's doing, it doesn't explain why it's
particularly needed.  I guess there might be an issue on reboot with
reset not completely resetting the hardware?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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