From: Sascha Hauer <s.hauer@pengutronix.de>
To: Mark Brown <broonie@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>,
linux-spi@vger.kernel.org,
Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
kernel@pengutronix.de, Vladimir Oltean <olteanv@gmail.com>,
Daniel Mack <daniel@zonque.org>
Subject: Re: [PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller()
Date: Tue, 22 Sep 2020 13:06:52 +0200 [thread overview]
Message-ID: <20200922110652.GC25328@pengutronix.de> (raw)
In-Reply-To: <20200922094437.GL4792@sirena.org.uk>
On Tue, Sep 22, 2020 at 10:44:37AM +0100, Mark Brown wrote:
> On Tue, Sep 22, 2020 at 11:32:23AM +0200, Sascha Hauer wrote:
>
> > @@ -1440,9 +1440,6 @@ static int dspi_remove(struct platform_device *pdev)
> > struct spi_controller *ctlr = platform_get_drvdata(pdev);
> > struct fsl_dspi *dspi = spi_controller_get_devdata(ctlr);
> >
> > - /* Disconnect from the SPI framework */
> > - spi_unregister_controller(dspi->ctlr);
> > -
> > /* Disable RX and TX */
> > regmap_update_bits(dspi->regmap, SPI_MCR,
> > SPI_MCR_DIS_TXF | SPI_MCR_DIS_RXF,
>
> Is this fix safe - what happens if we start another transaction between
> disabling RX/TX and the unregistration taking effect? Similar concerns
> apply to some of the other patches.
I asked myself the same question until I saw that a dozen of other drivers
also do it like this.
I don't think it's safe. Currently we only have spi_unregister_controller()
which both unregisters child SPI devices and frees the SPI controller along
with the driver data.
There are many drivers have something in their remove function like
disabling the hardware, clocks, freeing interrupts. They do it either
before calling spi_unregister_controller(), in which case messages may be
queued on a teared down hardware, or they do it after calling
spi_unregister_controller(), in which case they operate on already freed
data. IMO this only works when all drivers are fully managed with devm_*
functions and runtime pm so that the remove functions for all drivers
become empty. Until this happens spi_unregister_controller() has to be
split up in a function actually unregistering the controller and another
one freeing the resources.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
next prev parent reply other threads:[~2020-09-22 11:06 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-22 9:32 [PATCH 0/6] Fix use-after-free in SPI drivers Sascha Hauer
2020-09-22 9:32 ` [PATCH 1/6] spi: fsl-dspi: Use devm_spi_register_controller() Sascha Hauer
2020-09-22 9:44 ` Mark Brown
2020-09-22 11:06 ` Sascha Hauer [this message]
2020-09-22 11:25 ` Mark Brown
2020-09-22 9:32 ` [PATCH 2/6] spi: atmel-quadspi: " Sascha Hauer
2020-09-22 9:32 ` [PATCH 3/6] spi: bcm2835: " Sascha Hauer
2020-09-22 9:39 ` Nicolas Saenz Julienne
2020-09-22 9:32 ` [PATCH 4/6] spi: dw: " Sascha Hauer
2020-09-22 9:32 ` [PATCH 5/6] spi: pxa2xx: " Sascha Hauer
2020-09-22 9:32 ` [PATCH 6/6] spi: rpc-if: " Sascha Hauer
2020-10-15 5:44 ` [PATCH 0/6] Fix use-after-free in SPI drivers Lukas Wunner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200922110652.GC25328@pengutronix.de \
--to=s.hauer@pengutronix.de \
--cc=broonie@kernel.org \
--cc=daniel@zonque.org \
--cc=kernel@pengutronix.de \
--cc=linux-spi@vger.kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=nsaenzjulienne@suse.de \
--cc=olteanv@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).