Message-ID: <20210324233935.GE4596@sirena.org.uk>
Date:   Wed, 24 Mar 2021 23:39:35 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: ensure timely release of driver-allocated resources

On Wed, Mar 24, 2021 at 03:27:23PM -0700, Dmitry Torokhov wrote:
> On Wed, Mar 24, 2021 at 09:32:26PM +0000, Mark Brown wrote:

> > TBH that looks like a fairly standard case where you probably don't want
> > to be using devm for the interrupts in the first place.  Leaving the
> > interrupts live after the bus thinks it freed the device doesn't seem
> > like the best idea, I'm not sure I'd expect that to work reliably when
> > the device tries to call into the bus code to interact with the device
> > that the bus thought was already freed anyway.
> 
> That is not an argument really. By that token we should not be using
> devm for anything but memory, and that is only until we implement some
> kind of memleak checker that will ensure that driver-allocated memory is
> released after driver's remove() method exits.

I pretty much agree with that perspective TBH, I rarely see interrupt
users that I conside safe.  It's the things that actively do stuff that
cause issues, interrupts and registration of userspace interfaces both
being particularly likely to do so as work comes in asynchronously.

> You also misread that the issue is limited to interrupts, when i fact
> in this particular driver it is the input device that is being
> unregistered too late and fails to timely quiesce the device. Resulting
> interrupt storm is merely a side effect of this.

My understanding was that the issue is that the driver is generating
work because the interrupt is firing, if the interrupt had been
unregistered we'd not be getting the interupts delivered (probably
they'd have been disabled at the interrupt controller level) and not
have the problem of trying to handle them on a partially unwound device.

> > looked at.  Possibly we want a driver core callback which is scheduled
> > via devm (remove_devm(), cleanup() or something).  We'd still need to
> > move things about in all the buses but it seems preferable to do it that
> > way rather than open coding opening a group and the comments about
> > what's going on and the ordering requirements everywhere, it's a little
> > less error prone going forward.

> From the driver's perspective they expect devm-allocated resources to
> happen immediately after ->remove() method is run. I do not believe any
> driver is interested in additional callback, and you'd need to modify
> a boatload of drivers to fix this issue.

Not a callback for the device drivers, for the buses.  This is
essentially about converting the unwinding the bus does to be sequenced
for devm.

> I agree with you that manual group handling might be a bit confusing
> and sprinkling the same comment everywhere is not too nice, so how about
> we provide:

> 	void *devm_mark_driver_resources(struct device *dev)

> and

> 	void devm_release_driver_resources()

> and keep the commentary there? The question is where to keep
> driver_res_id field - in generic device structure or put it into bus'
> device structure as some buses and classes do not need it and we'd be
> sawing 4-8 bytes per device structure this way.

I guess bus' device :/

> Another way is to force buses to use devm for their resource management
> (I.e. in case of SPI wrap dev_pm_domain_detach() in
> devm_add_action_or_release()). It works for buses that have small number
> of resource allocated, but gets more unwieldy and more expensive the
> more resources are managed at bus level, that is why I opted for opening
> a group.

If the driver core were doing it and scheduling a single callback the
bus provides then that callback could do as much as it likes...

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

Powered by blists - more mailing lists