Messages in this thread | ![/](/images/icornerl.gif) | | Date | Tue, 2 Jan 2024 16:30:54 +0100 | From | Miquel Raynal <> | Subject | Re: [PATCH] nvmem: core: fix nvmem cells not being available in notifiers |
| |
Hi Luca,
[...]
> > 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.
Ok, that's clear now, I was on the wrong path, not because of the naming, but because you also focused on the REMOVE, while I was not expecting anything on that side.
> 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)
Yes, I see no use of the atomic counter in the right path for now, so I'd suggest to keep the logic simpler for now, if you don't mind.
> > > @@ -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.
Consumers should get -EPROBE_DEFER in this case. They can either try it later or... wait on the notifier :)
Thanks, Miquèl
| ![\](/images/icornerr.gif) |