Re: [PATCH] nvmem: core: fix nvmem cells not being available in notifiers

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Luca,

luca.ceresoli@bootlin.com wrote on Fri, 29 Dec 2023 11:26:26 +0100:

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

Nice commit log :) a few minor comments below.

...

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

								     per?

> cell.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.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;

The ternary operator is not needed here, (event == VAL) will return the
correct value.

Could we rename new_notified into something like "is_addition"? It took
me a bit of time understanding what this boolean meant.

> +	int was_notified = atomic_xchg(&cell->notified_add, new_notified);
> +
> +	if (new_notified != was_notified)

I believe what you want is (with my terms):

	if ((is_addition && !was_notified) || !is_addition)

> +		blocking_notifier_call_chain(&nvmem_notifier, event, cell);

I believe your if condition works, but is a bit complex to read. Is
there a reason for the following condition ?

	(new_notified := 0) /*removal */ != (was_notified := 1)

I see no use to this, but I am probably over looking something.

> +}
> +
>  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;

Could we rename this as well to be simpler? Like
"notify_cell_additions" or "cells_can_be_notified"? I am actually
asking myself whether this boolean is useful. In practice we call the
notifier after setting this to true. On the other hand, the layouts
will only probe after the device_add(), so they should be safe?

> +
> +	/* 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,


Thanks,
Miquèl