Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Mark Brown <broonie@kernel.org>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Martin Sperl <kernel@martin.sperl.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	linux-rpi-kernel <linux-rpi-kernel@lists.infradead.org>
Subject: Re: [PATCH 2/5] spi: bcm2835: Fix controller unregister order
Date: Sat, 16 May 2020 08:56:07 +0200
Message-ID: <20200516065607.zrqwlh5ayoth37y4@wunner.de> (raw)
In-Reply-To: <CAHp75Vfs=gc9c2h2amhu0eb3ux8jJAuC7CRY8fLrFiBsTwtSYQ@mail.gmail.com>

On Sat, May 16, 2020 at 12:33:00AM +0300, Andy Shevchenko wrote:
> On Fri, May 15, 2020 at 7:45 PM Lukas Wunner <lukas@wunner.de> wrote:
> > On Fri, May 15, 2020 at 05:29:03PM +0100, Mark Brown wrote:
> > > On Fri, May 15, 2020 at 05:58:02PM +0200, Lukas Wunner wrote:
> > > > Fix by using the non-devm variant spi_register_controller().  Note that
> > > > the struct spi_controller as well as the driver-private data are not
> > > > freed until after bcm2835_spi_remove() has finished, so accessing them
> > > > is safe.
> > >
> > > Why not use managed allocations of clocks and DMA channels?  This is a
> > > standard issue with the devm APIs, if you're using them you basically
> > > need to use them for *everything* up to the point where you start using
> > > them.
> >
> > There is no devm version of clk_prepare_enable(), dma_request_chan()
> > and various other functions invoked on ->probe() by spi-bcm2835.c.
> > So tearing down DMA channels, disabling clocks etc needs to happen
> > in the ->remove() hook and consequently devm_spi_register_controller()
> > cannot be used.
> 
> There is devm_add_action_or_reset (IIRC the name). It does a trick.

Interesting, thanks.

However there are currently four actions performed by bcm2835_spi_remove():

	bcm2835_debugfs_remove(bs);

	/* 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);

	bcm2835_dma_release(ctlr, bs);

So I think I'd have to add four functions to perform these devm actions,
which would add a lot more code than just the single line added by my
patch.  It also seems doubtful that the teardown code will still be easy
to follow.  And small patches like the ones I've submitted lend themselves
better to backporting to stable.

Mark, please provide guidance as to which variant you'd prefer:
Switching to the non-devm variant of spi_register_controller() as I've
done here or adding devm calls for all the existing teardown steps.

Thanks,

Lukas

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-15 15:58 [PATCH 0/5] Raspberry Pi SPI unbind fixes Lukas Wunner
2020-05-15 15:58 ` [PATCH 1/5] spi: Fix controller unregister order Lukas Wunner
2020-05-15 16:27   ` Mark Brown
2020-05-15 16:31     ` Lukas Wunner
2020-05-15 21:37       ` Andy Shevchenko
2020-05-16  6:45         ` Lukas Wunner
2020-05-16  9:45           ` Andy Shevchenko
2020-05-15 15:58 ` [PATCH 2/5] spi: bcm2835: " Lukas Wunner
2020-05-15 16:29   ` Mark Brown
2020-05-15 16:44     ` Lukas Wunner
2020-05-15 21:33       ` Andy Shevchenko
2020-05-16  6:56         ` Lukas Wunner [this message]
2020-05-16  9:40           ` Andy Shevchenko
2020-05-15 15:58 ` [PATCH 3/5] spi: bcm2835aux: " Lukas Wunner
2020-05-15 15:58 ` [PATCH 4/5] spi: bcm2835: Tear down DMA before turning off SPI controller Lukas Wunner
2020-05-15 15:58 ` [PATCH 5/5] spi: Document devm_spi_register_controller() gotcha Lukas Wunner
2020-05-15 16:30   ` Mark Brown
2020-05-20 17:17 ` [PATCH 0/5] Raspberry Pi SPI unbind fixes Mark Brown

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=20200516065607.zrqwlh5ayoth37y4@wunner.de \
    --to=lukas@wunner.de \
    --cc=andy.shevchenko@gmail.com \
    --cc=broonie@kernel.org \
    --cc=kernel@martin.sperl.org \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=nsaenzjulienne@suse.de \
    /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

Linux-SPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-spi/0 linux-spi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-spi linux-spi/ https://lore.kernel.org/linux-spi \
		linux-spi@vger.kernel.org
	public-inbox-index linux-spi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-spi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git