Linux-SPI Archive on lore.kernel.org
 help / color / Atom feed
From: Lukas Wunner <lukas@wunner.de>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Dan Williams <dan.j.williams@intel.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Pantelis Antoniou <pantelis.antoniou@konsulko.com>,
	Alexander Duyck <alexander.h.duyck@linux.intel.com>,
	Mark Brown <broonie@kernel.org>,
	linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org
Subject: Re: [PATCH 2/3] driver core: Use rwsem for kill_device() serialization
Date: Fri, 31 Jul 2020 11:53:10 +0200
Message-ID: <20200731095310.xcyehzci33lfzvxx@wunner.de> (raw)
In-Reply-To: <20200731064505.GH1508201@kroah.com>

On Fri, Jul 31, 2020 at 08:45:05AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 30, 2020 at 11:56:10AM +0200, Lukas Wunner wrote:
> > On Thu, Jul 30, 2020 at 08:53:26AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 08, 2020 at 03:27:02PM +0200, Lukas Wunner wrote:
> > > > kill_device() is currently serialized with driver probing by way of the
> > > > device_lock().  We're about to serialize it with device_add() as well
> > > > to prevent addition of children below a device which is going away.
> > > 
> > > Why?  Who does this?  Shouldn't the bus that is trying to do this know
> > > this is happening?
> > 
> > AFAICS, at least spi and i2c are affected.  Any bus which
> > creates a device hierarchy with dynamic addition & removal needs
> > to make sure no new children are added after removal of the parent
> > has begun.
> 
> I thought the bus code itself had this type of serialization already...

An SPI device is inaccessible once the controller has been torn down,
yet drivers may need to reach SPI devices to unbind cleanly (e.g. to
quiesce interrupts on SPI devices).

Therefore SPI devices need to be unregistered first and the controller
last.  However with CONFIG_OF_DYNAMIC=y, an SPI device may be added
at runtime via of_spi_notify(), which does take a ref on the controller,
but otherwise runs lockless against spi_unregister_controller().

What can happen here is an SPI device gets instantiated below a
controller as it is being removed and the SPI device can't be unbound
or removed cleanly because it's inaccessible.

The bus code can't do anything about this.  It doesn't learn about the
controller going away until device_unregister() is called at the *end* of
spi_unregister_controller().

Anyway, the preliminary patch below should do the trick and I've also
cooked up something similar for i2c.  Needs to be tested still.

Thanks,

Lukas

-- >8 --

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 6626587..b6876dd 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -475,6 +475,12 @@ struct boardinfo {
  */
 static DEFINE_MUTEX(board_lock);
 
+/*
+ * Prevents addition of devices with same chip select and
+ * addition of devices below an unregistering controller.
+ */
+static DEFINE_MUTEX(spi_add_lock);
+
 /**
  * spi_alloc_device - Allocate a new SPI device
  * @ctlr: Controller to which device is connected
@@ -554,7 +560,6 @@ static int spi_dev_check(struct device *dev, void *data)
  */
 int spi_add_device(struct spi_device *spi)
 {
-	static DEFINE_MUTEX(spi_add_lock);
 	struct spi_controller *ctlr = spi->controller;
 	struct device *dev = ctlr->dev.parent;
 	int status;
@@ -575,6 +580,12 @@ int spi_add_device(struct spi_device *spi)
 	 */
 	mutex_lock(&spi_add_lock);
 
+	if ((IS_ENABLED(CONFIG_OF_DYNAMIC) || IS_ENABLED(CONFIG_ACPI) ||
+	     IS_ENABLED(CONFIG_SPI_SLAVE)) && ctlr->unregistering) {
+		status = -ENODEV;
+		goto done;
+	}
+
 	status = bus_for_each_dev(&spi_bus_type, NULL, spi, spi_dev_check);
 	if (status) {
 		dev_err(dev, "chipselect %d already in use\n",
@@ -2795,6 +2806,13 @@ void spi_unregister_controller(struct spi_controller *ctlr)
 	struct spi_controller *found;
 	int id = ctlr->bus_num;
 
+	/* Prevent addition of new devices, then remove existing ones */
+	if (IS_ENABLED(CONFIG_OF_DYNAMIC) || IS_ENABLED(CONFIG_ACPI) ||
+	    IS_ENABLED(CONFIG_SPI_SLAVE)) {
+		mutex_lock(&spi_add_lock);
+		ctlr->unregistering = true;
+		mutex_unlock(&spi_add_lock);
+	}
 	device_for_each_child(&ctlr->dev, NULL, __unregister);
 
 	/* First make sure that this controller was ever added */
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 99380c0..6d95515 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -451,6 +451,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @irq_flags: Interrupt enable state during PTP system timestamping
  * @fallback: fallback to pio if dma transfer return failure with
  *	SPI_TRANS_FAIL_NO_START.
+ * @unregistering: Set on controller removal, prevents addition of new devices.
  *
  * Each SPI controller can communicate with one or more @spi_device
  * children.  These make a small bus, sharing MOSI, MISO and SCK signals
@@ -667,6 +668,8 @@ struct spi_controller {
 
 	/* Interrupt enable state during PTP system timestamping */
 	unsigned long		irq_flags;
+
+	unsigned int		unregistering:1;
 };
 
 static inline void *spi_controller_get_devdata(struct spi_controller *ctlr)

  reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-08 13:27 [PATCH 0/3] Fix races on device removal Lukas Wunner
2020-07-08 13:27 ` [PATCH 1/3] driver core: Avoid binding drivers to dead devices Lukas Wunner
2020-07-08 13:27 ` [PATCH 2/3] driver core: Use rwsem for kill_device() serialization Lukas Wunner
2020-07-30  6:53   ` Greg Kroah-Hartman
2020-07-30  9:56     ` Lukas Wunner
2020-07-31  6:45       ` Greg Kroah-Hartman
2020-07-31  9:53         ` Lukas Wunner [this message]
2020-07-08 13:27 ` [PATCH 3/3] driver core: Avoid adding children below a dead parent Lukas Wunner
2020-07-24 14:29   ` [driver core] e3b1cb5c89: WARNING:possible_recursive_locking_detected kernel test robot
2020-07-25 11:07     ` Lukas Wunner
2020-07-30  6:54 ` [PATCH 0/3] Fix races on device removal Greg Kroah-Hartman

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=20200731095310.xcyehzci33lfzvxx@wunner.de \
    --to=lukas@wunner.de \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=broonie@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=geert+renesas@glider.be \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=pantelis.antoniou@konsulko.com \
    --cc=rafael@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