Re: [PATCH v14 03/32] firmware: arm_scmi: Refactor __scmi_event_handler_get_ops helper

[Cristian Marussi]

On Mon, Oct 19, 2026 at 01:46:57PM -0700, Jonathan Cameron wrote:
> On Fri, 16 Oct 2026 19:55:24 +0100
> Cristian Marussi <cristian.marussi@xxxxxxx> wrote:
> 
> > Refactor the __scmi_event_handler_get_ops notification helper to simplify
> > its logic and make use of guards for locking.
> > 
> > Mostly no functional change, in the sense that while the functionality
> > remained the same, a richer set of errors is returned where before was
> > always a -EINVAL.
> 
> Maybe mention why scmi_allocate_event_handler() is now wrapped in
> an scmi_allocate_pending_event_handler()
> 
> I like the change but beyond logical division of activities it
> isn't immediately obvious why that split exists.

I'll do.

> 
> One really minor thing inline that I don't care that much about.
> 
> Anyhow, both trivial so
> Reviewed-by: Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>
> 
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> > ---
> >  drivers/firmware/arm_scmi/notify.c | 73 +++++++++++++++++++-----------
> >  1 file changed, 46 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c
> > index 7d1bb424a723..2bc2e03df639 100644
> > --- a/drivers/firmware/arm_scmi/notify.c
> > +++ b/drivers/firmware/arm_scmi/notify.c
> > @@ -890,7 +890,6 @@ void scmi_deregister_protocol_events(const struct scmi_handle *handle,
> >  
> >  /**
> >   * scmi_allocate_event_handler()  - Allocate Event handler
> > - * @ni: A reference to the notification instance to use
> >   * @evt_key: 32bit key uniquely bind to the event identified by the tuple
> >   *	     (proto_id, evt_id, src_id)
> >   *
> > @@ -903,8 +902,7 @@ void scmi_deregister_protocol_events(const struct scmi_handle *handle,
> >   * Context: Assumes to be called with @pending_mtx already acquired.
> >   * Return: the freshly allocated structure on Success
> >   */
> > -static struct scmi_event_handler *
> > -scmi_allocate_event_handler(struct scmi_notify_instance *ni, u32 evt_key)
> > +static struct scmi_event_handler *scmi_allocate_event_handler(u32 evt_key)
> >  {
> >  	struct scmi_event_handler *hndl;
> >  
> > @@ -914,8 +912,20 @@ scmi_allocate_event_handler(struct scmi_notify_instance *ni, u32 evt_key)
> >  	hndl->key = evt_key;
> >  	BLOCKING_INIT_NOTIFIER_HEAD(&hndl->chain);
> >  	refcount_set(&hndl->users, 1);
> > -	/* New handlers are created pending */
> > -	hash_add(ni->pending_events_handlers, &hndl->hash, hndl->key);
> > +
> > +	return hndl;
> > +}
> > +
> > +static struct scmi_event_handler *
> > +scmi_allocate_pending_event_handler(struct scmi_notify_instance *ni,
> > +				    u32 evt_key)
> > +{
> > +	struct scmi_event_handler *hndl;
> > +
> > +	lockdep_assert_held(&ni->pending_mtx);
> > +	hndl =  scmi_allocate_event_handler(evt_key);
> > +	if (hndl)
> > +		hash_add(ni->pending_events_handlers, &hndl->hash, hndl->key);
> Trivial but I'd keep to a strict errors out of line
> 	if (!hndl)
> 		return NULL
> 
> 	hash_add(ni->...
> 	
> 	return hndl;
> 
> It burns a few lines be saves a few brain cells.
> 

yes, I prefer this too...I'll do.

Thanks,
Cristian