Re: [PATCH v2 1/7] HID: sensor: hub: Synchronize callback removal with raw events

[srinivas pandruvada]

On Sat, 2026-11-07 at 20:13 +0000, Jonathan Cameron wrote:
> On Wed,  4 Nov 2026 12:13:57 -0800
> Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> 
> > Raw-event processing holds pdata->lock across callback lookup and
> > invocation, and takes dyn_callback_lock during lookup. Removal must
> > prevent both active callbacks and future lookups before client
> > state can
> > be released.
> > 
> > Take pdata->lock before dyn_callback_lock, matching raw-event
> > order,
> > and unlink the callback while holding both locks. This ensures
> > active
> > raw-event callbacks have finished and prevents future lookups.
> > 
> > Fixes: 401ca24fb34a ("HID: sensors: introduce sensor framework")
> > Reported-by: Семён Марьясин <simeon@xxxxxxxxxxxxx>
> > Closes:
> > https://lore.kernel.org/all/CAM2dg7ewfVJSP4rFyhkCB1Rmd1+X56sAg=eUdbPthG2xrLqwQQ@xxxxxxxxxxxxxx/
> > Signed-off-by: Srinivas Pandruvada
> > <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> 
> Maybe a minimal fix is good for now, but I do note that we can
> make this code nicer with some guard() magic.

Yes guard() will be better here.
guard() can be a different cleanup as this should be done in whole
driver instead of this one function here. I will do a clean up patch
instead of fixes.

Thanks,
Srinivas

> 
> Perhaps a follow up series to tidy this up.
> 
> Things are getting very messy around this driver wrt to fix vs
> cleanup.  Hopefully it won't cause too much of a problem in next
> until I can merge the fixes back after they go upstream.
> 
> > ---
> >  drivers/hid/hid-sensor-hub.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-
> > hub.c
> > index 6470a290ebfc..0c4884c5821f 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -173,7 +173,9 @@ int sensor_hub_remove_callback(struct
> > hid_sensor_hub_device *hsdev,
> >  	struct sensor_hub_data *pdata = hid_get_drvdata(hsdev-
> > >hdev);
> >  	unsigned long flags;
> >  
> > -	spin_lock_irqsave(&pdata->dyn_callback_lock, flags);
> > +	/* Serialize callback removal with raw-event callback
> > execution. */
> > +	spin_lock_irqsave(&pdata->lock, flags);
> > +	spin_lock(&pdata->dynfunction_callback_lock);
> 
> Do both of those with appropriate guard() 
> 
> >  	list_for_each_entry(callback, &pdata->dyn_callback_list,
> > list)
> >  		if (callback->usage_id == usage_id &&
> >  						callback->hsdev ==
> > hsdev) {
> I'd just go long!
>  		if (callback->usage_id == usage_id && callback-
> >hsdev == hsdev) {
> 
> 
> > @@ -181,7 +183,8 @@ int sensor_hub_remove_callback(struct
> > hid_sensor_hub_device *hsdev,
> >  			kfree(callback);
> >  			break;
> And we can return here.
> 
> >  		}
> > -	spin_unlock_irqrestore(&pdata->dyn_callback_lock, flags);
> > +	spin_unlock(&pdata->dyn_callback_lock);
> > +	spin_unlock_irqrestore(&pdata->lock, flags);
> >  
> >  	return 0;
> >  }