Re: [PATCH 2/5] spi: bcm2835: Fix controller unregister order

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, May 16, 2020 at 9:56 AM Lukas Wunner <lukas@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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);

This one shouldn't be counted. You can init it as the last op in
->probe() and that mustn't fail the probe.

>         /* Clear FIFOs, and disable the HW block */
>         bcm2835_wr(bs, BCM2835_SPI_CS,
>                    BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX)

This one I don't know.

>         clk_disable_unprepare(bs->clk);

This one traditionally done via devm_add_action_or_reset.

>         bcm2835_dma_release(ctlr, bs);

This one probably the one which needs to be addressed ideally in DMA
engine for all.

>
> 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.

All generic devm_* APIs have a release counterparts, you can
explicitly call them in order you want during ->remove(). So, I still
see a benefit of devm_*() even in such cases (makes ->probe() much
easier).

-- 
With Best Regards,
Andy Shevchenko