Re: [PATCH v6 03/22] firmware: arm_scmi: Introduce protocol instance notifiers

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Jul 28, 2026 at 01:00:04AM +0100, Jonathan Cameron wrote:
> On Fri, 24 Jul 2026 15:44:11 +0100
> Cristian Marussi <cristian.marussi@arm.com> wrote:
> 
> > Allow protocols themselves to register for their own notifications and
> > provide their own notifier callbacks. Each protocol can now register one
> > unique per-protocol instance notifier block whose callback will be
> > registered on the proper notification chain as usual: such notifier will
> > be automatically removed during the protocol de-initialiazation phase.
> > 
> 
> Would be nice to say why they might do this. I'm sure it becomes
> apparent later in the series, but anyone looking just this patch
> is missing that useful information.
>

Hi Jonathan,

thanks for your review... 

I've reworked this comment in V8.

> I'd also like something here to talk briefly about why it is fine to
> drop the lock briefly on the unregister side.

I've reviewed this logic... (spoiler... it was wrong :P)...the lock still
cannot be held BUT the new implementation should avoid races by using
the refcount and a new flag...

> 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index 8e06e40d1a11..0f9e8dfc6137 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> ...
> 
> 
> > @@ -2367,13 +2402,29 @@ int scmi_protocol_acquire(const struct scmi_handle *handle, u8 protocol_id)
> >  void scmi_protocol_release(const struct scmi_handle *handle, u8 protocol_id)
> >  {
> >  	struct scmi_info *info = handle_to_scmi_info(handle);
> > +	struct notifier_block *proto_notifier_nb = NULL;
> >  	struct scmi_protocol_instance *pi;
> >  
> > -	mutex_lock(&info->protocols_mtx);
> > -	pi = idr_find(&info->protocols, protocol_id);
> > -	if (WARN_ON(!pi))
> > -		goto out;
> > +	scoped_guard(mutex, &info->protocols_mtx) {
> > +		pi = idr_find(&info->protocols, protocol_id);
> > +		if (WARN_ON(!pi))
> > +			return;
> > +
> > +		proto_notifier_nb = pi->pno.nb;
> > +		/* Ensure NULL is visible */
> > +		smp_store_mb(pi->pno.nb, NULL);
> > +	}
> >  
> 
> I'd like to see a little commentary on safety of not holding
> the mutex over the whole sequence.  Can anything mess around
> with pi between the code getting hold of it and the release below?
> I assume the refcount is enough.

I changed a bit the implementation and added a comment in v8 that
should explain...it has to do with the refcount indeed (but with the
new revised implementation..)

> 
> > +	if (proto_notifier_nb) {
> > +		int ret;
> > +
> > +		ret = scmi_protocol_notifier_unregister(pi->handle, &pi->pno);
> > +		if (ret)
> 
> Maybe this changes later but if not get rid of ret. It kind of suggests
> the value itself is useful in a way that isn't true.

...good point...reviewed in v8.

Thanks,
Cristian