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?