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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 4 Oct 2021 13:49:21 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Lino Sanfilippo <LinoSanfilippo@....de>
Cc:     f.fainelli@...il.com, rjui@...adcom.com, sbranden@...adcom.com,
        bcm-kernel-feedback-list@...adcom.com, nsaenz@...nel.org,
        linux-spi@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        jgg@...pe.ca, p.rosenberger@...bus.com,
        linux-integrity@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] spi: bcm2835: do not unregister controller in shutdown
 handler

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?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)