Date: Tue, 2 Jan 2024 16:30:54 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>
Cc: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>, 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
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

Powered by blists - more mailing lists