Date: Fri, 29 Dec 2023 11:26:26 +0100
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: Miquel Raynal <miquel.raynal@...tlin.com>, 
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
 Michael Walle <michael@...le.cc>, 
 "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>, 
 Rafał Miłecki <rafal@...ecki.pl>, 
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
 linux-kernel@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>
Subject: [PATCH] nvmem: core: fix nvmem cells not being available in
 notifiers

With current code, when an NVMEM notifier for NVMEM_CELL_ADD is called, the
cell is not accessible within the notifier call function.

This can be easily seen with a simple NVMEM notifier call doing:

    if (action == NVMEM_CELL_ADD)
        cell = nvmem_cell_get(hpm->dev, "id");

In this case nvmem_cell_get() always returns -EPROBE_DEFER if trying to get
the cell whose addition is being notified. Adding a long msleep() before
nvmem_cell_get() does not change the result. On the other hand,
nvmem_cell_get() works when invoked from a different code path not
involving the NVMEM event notifier.

The failing code path in my test case is:

 nvmem_cell_get()
 -> of_nvmem_cell_get()
    -> __nvmem_device_get()
       -> bus_find_device(); ---> returns NULL

The cause is in nvmem_register(), which adds the cells (via
nvmem_add_cells_from_fixed_layout() in my case) and in the process calls
the NVMEM_CELL_ADD notifiers _before_ calling device_add(&nvmem->dev), thus
before the nvmem device is added to the bus. This prevents
bus_find_device() from finding the device.

This makes the NVMEM_CELL_ADD notifier pretty useless, at least in the use
case where a consumer driver wants to read a cell when it becomes
available, without additional infrastructure to postpone the
nvmem_cell_get() call.

The easy solution would be moving the device_add() just before cell
addition instead of just after. This is exactly what the original
implementation was doing, but it had a race condition, which was fixed in
commit ab3428cfd9aa ("nvmem: core: fix registration vs use race") exactly
by swapping device_add() and cell addition.

Solve this problem by leaving cell addition before device_add() but moving
the notifications after. This would be pretty simple if all cells were
added by nvmem_register(), but cell layouts could run later if they are
built as modules, so they need to be allowed to notify when they add cells
after nvmem_register().

Solve this by adding a flag in struct nvmem_device to block all
notifications before calling device_add(), and keep track of whether each
cell got notified or not, so that exactly one notification is sent ber
cell.

Signed-off-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
---
 drivers/nvmem/core.c      | 35 +++++++++++++++++++++++++++++++++--
 drivers/nvmem/internals.h |  2 ++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index ba559e81f77f..42f8edbfb39c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -36,6 +36,7 @@ struct nvmem_cell_entry {
 	struct device_node	*np;
 	struct nvmem_device	*nvmem;
 	struct list_head	node;
+	atomic_t		notified_add;
 };
 
 struct nvmem_cell {
@@ -520,9 +521,29 @@ static struct bus_type nvmem_bus_type = {
 	.name		= "nvmem",
 };
 
+/*
+ * Send cell add/remove notification unless it has been already sent.
+ *
+ * Uses and updates cell->notified_add to avoid duplicates.
+ *
+ * Must never be called with NVMEM_CELL_ADD after being called with
+ * NVMEM_CELL_REMOVE.
+ *
+ * @cell: the cell just added or going to be removed
+ * @event: NVMEM_CELL_ADD or NVMEM_CELL_REMOVE
+ */
+static void nvmem_cell_notify(struct nvmem_cell_entry *cell, unsigned long event)
+{
+	int new_notified = (event == NVMEM_CELL_ADD) ? 1 : 0;
+	int was_notified = atomic_xchg(&cell->notified_add, new_notified);
+
+	if (new_notified != was_notified)
+		blocking_notifier_call_chain(&nvmem_notifier, event, cell);
+}
+
 static void nvmem_cell_entry_drop(struct nvmem_cell_entry *cell)
 {
-	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
+	nvmem_cell_notify(cell, NVMEM_CELL_REMOVE);
 	mutex_lock(&nvmem_mutex);
 	list_del(&cell->node);
 	mutex_unlock(&nvmem_mutex);
@@ -544,7 +565,9 @@ static void nvmem_cell_entry_add(struct nvmem_cell_entry *cell)
 	mutex_lock(&nvmem_mutex);
 	list_add_tail(&cell->node, &cell->nvmem->cells);
 	mutex_unlock(&nvmem_mutex);
-	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_ADD, cell);
+
+	if (cell->nvmem->do_notify_cell_add)
+		nvmem_cell_notify(cell, NVMEM_CELL_ADD);
 }
 
 static int nvmem_cell_info_to_nvmem_cell_entry_nodup(struct nvmem_device *nvmem,
@@ -902,6 +925,7 @@ EXPORT_SYMBOL_GPL(nvmem_layout_get_match_data);
 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 {
 	struct nvmem_device *nvmem;
+	struct nvmem_cell_entry *cell;
 	int rval;
 
 	if (!config->dev)
@@ -1033,6 +1057,13 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
 
+	/* After device_add() it is now OK to notify of new cells */
+	nvmem->do_notify_cell_add = true;
+
+	/* Notify about cells previously added but not notified */
+	list_for_each_entry(cell, &nvmem->cells, node)
+		nvmem_cell_notify(cell, NVMEM_CELL_ADD);
+
 	return nvmem;
 
 #ifdef CONFIG_NVMEM_SYSFS
diff --git a/drivers/nvmem/internals.h b/drivers/nvmem/internals.h
index 18fed57270e5..3dbaa8523530 100644
--- a/drivers/nvmem/internals.h
+++ b/drivers/nvmem/internals.h
@@ -33,6 +33,8 @@ struct nvmem_device {
 	struct nvmem_layout	*layout;
 	void *priv;
 	bool			sysfs_cells_populated;
+	/* Enable sending NVMEM_CELL_ADD notifications */
+	bool			do_notify_cell_add;
 };
 
 #if IS_ENABLED(CONFIG_OF)

---
base-commit: 399769c2014d2aa0463636d50f2bc6431b377331
change-id: 20231229-nvmem-cell-add-notification-feb857742f0a

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@...tlin.com>

Powered by blists - more mailing lists