Re: [PATCH 1/2] iio: hid-sensor-gyro-3d: move iio_device_register() to end of probe()

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Sat, 2026-03-07 at 14:21 +0000, Jonathan Cameron wrote:
> On Mon, 2 Mar 2026 01:00:06 +0530
> Bhargav Joshi <rougueprince47@gmail.com> wrote:
> 
> > On Sun, Mar 1, 2026 at 5:15 PM Jonathan Cameron <jic23@kernel.org>
> > wrote:
> > > 
> > > On Sun,  1 Mar 2026 00:43:59 +0530
> > > Bhargav Joshi <rougueprince47@gmail.com> wrote:
> > >  
> > > > Currently, calling iio_device_register() before
> > > > sensor_hub_register_callback() may create a race condition
> > > > where the
> > > > device is exposed to userspace before callbacks are wired.  
> > > 
> > > We needs some more here on 'why' this is a problem.
> > > Whilst this is an unusual arrangement, I couldn't immediately
> > > find
> > > anything that was actually broken.  It's possible data will turn
> > > up
> > > after we tear down the callbacks and before the userspace
> > > interfaces
> > > are removed, but I think that just results in dropping data.
> > > Given it's
> > > in a race anyway I don't think we care about that. The callbacks
> > > don't seem to be involved in device configuration which can go on
> > > whether
> > > or not the callbacks are registered.  Maybe there is something
> > > that
> > > won't get acknowledged if they aren't in place in time?
> > > 
> > > For a fix like this we'd normally want to see a clear flow that
> > > leads to a bug.
> > > 
> > > Also a fixes tag so we know how far to backport.
> > >  
> > 
> > Hi Jonathan,
> > 
> > I originally submitted the patch for the driver because calling
> > iio_device_register() before the callbacks are wired up violates
> > standard IIO LIFO ordering. I assumed this created a dangerous race
> > condition with userspace. However, as you correctly pointed out,
> > the
> > HID sensor hub safely drops those early events without crashing,
> > even
> > if it is unusual behaviour still won't have a hard crash.
> > 
> > My underlying goal was simply a structural cleanup to align the
> > probe() and remove() sequences with standard IIO architecture.
> > 
> > Would you like me to send a v2 of this patch with a revised commit
> > message classifying it purely as a structural cleanup (and without
> > a
> > Fixes tag), or is the current driver behavior acceptable enough
> > that I
> > should just drop this patch entirely?
> 
> My gut is leave this one alone. It's a rather unusual driver in lots
> of ways, so I don't really mind one more ;)
> 
> Lets see if anyone else has strong views one way or the other.
> 
> Srinivas?
> 
I don't see anything wrong with this. If you register callback before, 
then iio_push_to_buffers_with_timestamp() can be called before
iio_device_register(), although it will not happen because nobody
powered on sensors in the hub.

Thanks,
Srinivs






> > 
> > Thanks,
> > Bhargav
> > 
> > > > 
> > > > Move iio_device_register() to the end of the probe() function
> > > > to prevent
> > > > race condition.
> > > > 
> > > > Consequently, update the error handling path in probe() and in
> > > > remove()
> > > > ensuring that iio_device_unregister() is called first to cut
> > > > off
> > > > userspace access before the hardware callbacks are removed.
> > > > 
> > > > Signed-off-by: Bhargav Joshi <rougueprince47@gmail.com>
> > > > ---
> > > >  drivers/iio/gyro/hid-sensor-gyro-3d.c | 20 ++++++++++---------
> > > > -
> > > >  1 file changed, 10 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> > > > b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> > > > index c43990c518f7..8e3628cd8529 100644
> > > > --- a/drivers/iio/gyro/hid-sensor-gyro-3d.c
> > > > +++ b/drivers/iio/gyro/hid-sensor-gyro-3d.c
> > > > @@ -333,12 +333,6 @@ static int hid_gyro_3d_probe(struct
> > > > platform_device *pdev)
> > > >               return ret;
> > > >       }
> > > > 
> > > > -     ret = iio_device_register(indio_dev);
> > > > -     if (ret) {
> > > > -             dev_err(&pdev->dev, "device register failed\n");
> > > > -             goto error_remove_trigger;
> > > > -     }
> > > > -
> > > >       gyro_state->callbacks.send_event = gyro_3d_proc_event;
> > > >       gyro_state->callbacks.capture_sample =
> > > > gyro_3d_capture_sample;
> > > >       gyro_state->callbacks.pdev = pdev;
> > > > @@ -346,13 +340,19 @@ static int hid_gyro_3d_probe(struct
> > > > platform_device *pdev)
> > > >                                       &gyro_state->callbacks);
> > > >       if (ret < 0) {
> > > >               dev_err(&pdev->dev, "callback reg failed\n");
> > > > -             goto error_iio_unreg;
> > > > +             goto error_remove_trigger;
> > > > +     }
> > > > +
> > > > +     ret = iio_device_register(indio_dev);
> > > > +     if (ret) {
> > > > +             dev_err(&pdev->dev, "device register failed\n");
> > > > +             goto error_remove_callback;
> > > >       }
> > > > 
> > > >       return ret;
> > > > 
> > > > -error_iio_unreg:
> > > > -     iio_device_unregister(indio_dev);
> > > > +error_remove_callback:
> > > > +     sensor_hub_remove_callback(hsdev,
> > > > HID_USAGE_SENSOR_GYRO_3D);
> > > >  error_remove_trigger:
> > > >       hid_sensor_remove_trigger(indio_dev, &gyro_state-
> > > > >common_attributes);
> > > >       return ret;
> > > > @@ -365,8 +365,8 @@ static void hid_gyro_3d_remove(struct
> > > > platform_device *pdev)
> > > >       struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> > > >       struct gyro_3d_state *gyro_state = iio_priv(indio_dev);
> > > > 
> > > > -     sensor_hub_remove_callback(hsdev,
> > > > HID_USAGE_SENSOR_GYRO_3D);
> > > >       iio_device_unregister(indio_dev);
> > > > +     sensor_hub_remove_callback(hsdev,
> > > > HID_USAGE_SENSOR_GYRO_3D);
> > > >       hid_sensor_remove_trigger(indio_dev, &gyro_state-
> > > > >common_attributes);
> > > >  }
> > > >  
> > >