Messages in this thread Patch in this message | | | Date | Tue, 2 Jan 2024 14:46:31 +0100 | From | Luca Ceresoli <> | Subject | Re: [PATCH] nvmem: core: fix nvmem cells not being available in notifiers |
| |
On Tue, 2 Jan 2024 10:35:03 +0100 Miquel Raynal <miquel.raynal@bootlin.com> 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
| |