Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] spi: ensure timely release of driver-allocated resources
Date: Wed, 24 Mar 2021 23:39:35 +0000
Message-ID: <20210324233935.GE4596@sirena.org.uk> (raw)
In-Reply-To: <YFu8y9CuG6Mouxnq@google.com>


[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-22  1:43 Dmitry Torokhov
2021-03-22 12:37 ` Mark Brown
2021-03-22 19:38   ` Dmitry Torokhov
2021-03-23 17:36     ` Mark Brown
2021-03-23 19:04       ` Dmitry Torokhov
2021-03-24 21:32         ` Mark Brown
2021-03-24 22:27           ` Dmitry Torokhov
2021-03-24 23:39             ` Mark Brown [this message]
2021-03-25  0:17               ` Dmitry Torokhov
2021-03-30 17:19                 ` 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=20210324233935.GE4596@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    /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