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

[Luca Ceresoli]

On Tue, 2 Jan 2024 10:35:03 +0100
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

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

Sure.

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

OK.

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

Let me explain better the idea. This is the value that
cell->notified_add gets over time:

 1. at initialization: 0
 2. when calling nvmem_cell_notify(cell, NVMEM_CELL_ADD): 1
    and ADD notifier functions are called
 3. if calling nvmem_cell_notify(cell, NVMEM_CELL_ADD) again
    nothing happens
 4. when calling nvmem_cell_notify(cell, NVMEM_CELL_REMOVE): 0
    and REMOVE notifier functions are called
 5. if calling nvmem_cell_notify(cell, NVMEM_CELL_REMOVE) again
    nothing happens

So it avoids calling multiple notifiers both for addition, which is the
main goal, but also for removal. I understand there is probably no code
path for multiple removal calls, so maybe this is not useful.

I tried to find a good variable name to express this, and failed. :)

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

The "{was,new}_notified" names in my mind mean "{old,new} value of the
atomic flag". Thus "if (new_notified != was_notified)" means "if there
is a change of state, then notify it".

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

From my explanation above, it is hopefully now clear that this means:

 (new_notified := 0, i.e. we are having a removal event) !=
 (was_notified := 1, i.e. the last even notified was not a removal)

That said, I'm open to remove this logic, and on cell removal just
unconditionally send a notifier, probably without changing the variable
value:

  if (removal || !notify_cell_additions(&cell->notified_add, 1)

> > @@ -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"?

"notify_cell_additions" seems the best, thanks for the suggestion.

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

What if the module implementing the layout is loaded after
nvmem_register() finished? of_nvmem_cell_get() ->
nvmem_layout_module_get_optional() -> try_module_get() should allow
that, but I may be missing something.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com