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

[Jonathan Cameron <jic23@kernel.org>]

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?

> 
> 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);
> > >  }
> > >  
> >