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 21:32:26 +0000
Message-ID: <20210324213225.GD4596@sirena.org.uk> (raw)
In-Reply-To: <YFo7wkq037P2Dosz@google.com>


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

On Tue, Mar 23, 2021 at 12:04:34PM -0700, Dmitry Torokhov wrote:
> On Tue, Mar 23, 2021 at 05:36:06PM +0000, Mark Brown wrote:

> No it is ordering issue. I do not have a proven real-life example for
> SPI, but we do have one for I2C:

> https://lore.kernel.org/linux-devicetree/20210305041236.3489-7-jeff@labundy.com/

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.

If we want this to work reliably it really feels like we should have two
remove callbacks in the driver core doing this rather than open coding
in every single bus which is what we'd need to do - this is going to
affect any bus that does anything other than just call the device's
remove() callback.  PCI looks like it might have issues too for example,
and platform does as well and those were simply the first two buses I
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.

> Note how dev_pm_domain_detach() jumped ahead of everything, and
> strictly speaking past this point we can no longer guarantee that we can
> access the chip and disable it.

Frankly it looks like the PM domain stuff shouldn't be in the probe()
and remove() paths at all and this has been bogusly copies from other
buses, it should be in the device registration paths.  The device is in
the domain no matter what's going on with binding it.  Given how generic
code is I'm not even sure why it's in the buses.

[-- 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 [this message]
2021-03-24 22:27           ` Dmitry Torokhov
2021-03-24 23:39             ` Mark Brown
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=20210324213225.GD4596@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