Re: [PATCH v10 6/8] iio: osf: add IIO devices from capability reports

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 20 Sep 2026 14:07:34 +0900
Kim Jinseob <kimjinseob88@gmail.com> wrote:

> > Using an extra lock is one solution I suppose to the risk
> > of this racing with the buffer tear down and the problems
> > around active_scan_mask being freed. Could you use
> >
> >     if (iio_device_try_claim_buffer_mode(indio_dev))
> >
> > and later a release on that to achieve the same result?  
> 
> I looked into this against the current IIO core and tested the relevant paths.
> 
> iio_device_try_claim_buffer_mode() does hold the IIO mode lock, and
> iio_update_buffers() takes the same lock, so it would serialize normal
> buffer reconfiguration.
> 
> However, the unregister path does not hold that mode lock while
> disabling/deactivating the buffers, and that path can free
> active_scan_mask.

Ah. That seems like a hole we should close. This I think is the
call to iio_disable_all_buffers() in iio_device_unregister()?  The only
potential problem there would be lock inversion wrt to the info_exist_lock.
That shouldn't be a problem as iio_update_buffers() takes them in the same
order as we would be doing here.
So at least a first look, taking mlock inside iio_disable_all_buffers()
would seem like an easy change to make.

It would be a lot more logical if that gap had never existed!
Thanks for the analysis - I'd be surprised if we don't have other
latent bugs around that protection not being there.

Assuming there isn't something I'm missing would you mind sending that
as a separate patch that we can ask folk to test ahead of the main
series.

Thanks,

Jonathan




> Because the OSF receive path can still have an
> admitted buffer push at that point, the mode claim alone does not
> appear to provide the same lifetime guarantee as the existing private
> lock.
> 
> I therefore kept the private lock for now. I also reran the
> disable/unregister race coverage with KASAN and lockdep enabled; those
> tests pass without diagnostics.
> 
> Thanks,
> 
> Jinseob
> 
> 2026년 9월 20일 (일) 오후 1:20, Kim Jinseob <kimjinseob88@gmail.com>님이 작성:
> >  
> > > Using an extra lock is one solution I suppose to the risk
> > > of this racing with the buffer tear down and the problems
> > > around active_scan_mask being freed. Could you use
> > >
> > >     if (iio_device_try_claim_buffer_mode(indio_dev))
> > >
> > > and later a release on that to achieve the same result?
> > >
> > > Only the ade9000 is doing similar at the moment so would need
> > > some testing to make sure no lock inversions occur in general
> > > but given you don't take any other locks in here it should be fine.  
> >
> > I think that should be a better fit here. I'll test replacing the
> > private buffer lock and active flag with
> > iio_device_try_claim_buffer_mode() , iio_device_release_buffer_mode(),
> > including the existing buffer disable/unregister race coverage and
> > lockdep checks.
> >
> > If that confirms the expected serialization against buffer
> > reconfiguration, I'll use that approach in the next revision.
> >
> > Thanks,
> >
> > Jinseob
> >
> > 2026년 9월 20일 (일) 오전 11:03, Jonathan Cameron <jic23@kernel.org>님이 작성:  
> > >
> > > On Sat, 19 Sep 2026 03:24:44 +0900
> > > Jinseob Kim <kimjinseob88@gmail.com> wrote:
> > >  
> > > > Register IIO devices from supported capability descriptors and expose
> > > > signed raw samples, descriptor scales and software buffers. Keep early
> > > > capabilities owned by the core until UART and supply setup is complete,
> > > > and unregister children after the receive producer has stopped.
> > > >
> > > > Validate nonzero descriptor scales and sample/descriptor equality, leave
> > > > discovery open after empty or unsupported inventories, and compare
> > > > repeated descriptors independently of order. Fault a bound session if a
> > > > descriptor changes meaning, gating cache and buffer publication together.
> > > >
> > > > Add initialized active-scan packing and buffer producer quiescence. The
> > > > production implementation is now complete; subsequent patches only add
> > > > the existing core and IIO KUnit suites and their build wiring.  
> > >
> > > No need to talk about what other patches do and that a series is functionally
> > > complete in a patch. If you want to do it, under the --- so it doesn't end
> > > up in the git log.
> > >  
> > > >
> > > > Assisted-by: LLM
> > > > Signed-off-by: Jinseob Kim <kimjinseob88@gmail.com>  
> > >
> > > A few things inline.  Biggest one is a question (I'm not sure) on
> > > whether you can use a claim on buffered mode (which will hold it in that
> > > if it succeeds until released) is sufficient to replace the extra lock you
> > > have to block that transition.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > >
> > >  
> > > > diff --git a/drivers/iio/opensensorfusion/osf_iio.c b/drivers/iio/opensensorfusion/osf_iio.c
> > > > new file mode 100644
> > > > index 000000000000..f4011b8fae14
> > > > --- /dev/null
> > > > +++ b/drivers/iio/opensensorfusion/osf_iio.c  
> > > ...
> > >  
> > > > +
> > > > +struct osf_iio_scan_3axis {
> > > > +     s32 values[3];
> > > > +     u32 padding;  
> > > No needed unless you explicitly write into padding for some reason.
> > > C rules add it anyway and then we don't need to make sure it remains
> > > correct if this structure changes in future.  
> > > > +     aligned_s64 timestamp;
> > > > +};
> > > > +
> > > > +struct osf_iio_scan_1axis {
> > > > +     s32 value;
> > > > +     u32 padding;  
> > > As above, unneeded.  
> > > > +     aligned_s64 timestamp;
> > > > +};  
> > >
> > > Given I think these are only used in one place, moving them to there would
> > > avoid need for named structure types.
> > >  
> > > > +
> > > > +#define OSF_MOD_CHAN(_type, _mod, _idx)                              \
> > > > +     {                                                               \
> > > > +             .type = (_type),                                        \
> > > > +             .modified = 1,                                          \
> > > > +             .channel2 = (_mod),                                     \
> > > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > > > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > > > +             .scan_index = (_idx),                                   \
> > > > +             .scan_type = {                                          \
> > > > +                     .sign = 's',                                    \
> > > > +                     .realbits = 32,                                 \
> > > > +                     .storagebits = 32,                              \
> > > > +                     .endianness = IIO_CPU,                          \
> > > > +             },                                                      \
> > > > +     }
> > > > +
> > > > +#define OSF_CHAN(_type, _idx)                                        \  
> > >
> > > Given this is not much used, you could use the IIO_NO_MOD modifier as a gate
> > > on setting .modified = (_mod != IIO_NO_MOD) ? 1 : 0,
> > > and have just one macro.
> > >  
> > > > +     {                                                               \
> > > > +             .type = (_type),                                        \
> > > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > > > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > > > +             .scan_index = (_idx),                                   \
> > > > +             .scan_type = {                                          \
> > > > +                     .sign = 's',                                    \
> > > > +                     .realbits = 32,                                 \
> > > > +                     .storagebits = 32,                              \
> > > > +                     .endianness = IIO_CPU,                          \
> > > > +             },                                                      \
> > > > +     }
> > > > +
> > > > +static const struct iio_chan_spec osf_accel_channels[] = {
> > > > +     OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_X, 0),
> > > > +     OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_Y, 1),
> > > > +     OSF_MOD_CHAN(IIO_ACCEL, IIO_MOD_Z, 2),
> > > > +     IIO_CHAN_SOFT_TIMESTAMP(3),
> > > > +};
> > > > +
> > > > +static const struct iio_chan_spec osf_gyro_channels[] = {
> > > > +     OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_X, 0),
> > > > +     OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_Y, 1),
> > > > +     OSF_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_Z, 2),
> > > > +     IIO_CHAN_SOFT_TIMESTAMP(3),
> > > > +};
> > > > +
> > > > +static const struct iio_chan_spec osf_mag_channels[] = {
> > > > +     OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_X, 0),
> > > > +     OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_Y, 1),
> > > > +     OSF_MOD_CHAN(IIO_MAGN, IIO_MOD_Z, 2),
> > > > +     IIO_CHAN_SOFT_TIMESTAMP(3),
> > > > +};
> > > > +
> > > > +static const struct iio_chan_spec osf_temp_channels[] = {
> > > > +     OSF_CHAN(IIO_TEMP, 0),
> > > > +     IIO_CHAN_SOFT_TIMESTAMP(1),
> > > > +};
> > > > +
> > > > +static const struct osf_iio_sensor_spec osf_iio_sensor_specs[] = {
> > > > +     {
> > > > +             .sensor_type = OSF_SENSOR_ACCELEROMETER,
> > > > +             .channel_count = 3,
> > > > +             .name = "osf-accel",
> > > > +             .channels = osf_accel_channels,
> > > > +             .num_channels = ARRAY_SIZE(osf_accel_channels),
> > > > +     },
> > > > +     {
> > > > +             .sensor_type = OSF_SENSOR_GYROSCOPE,
> > > > +             .channel_count = 3,
> > > > +             .name = "osf-gyro",
> > > > +             .channels = osf_gyro_channels,
> > > > +             .num_channels = ARRAY_SIZE(osf_gyro_channels),
> > > > +     },
> > > > +     {
> > > > +             .sensor_type = OSF_SENSOR_MAGNETOMETER,
> > > > +             .channel_count = 3,
> > > > +             .name = "osf-magn",
> > > > +             .channels = osf_mag_channels,
> > > > +             .num_channels = ARRAY_SIZE(osf_mag_channels),
> > > > +     },
> > > > +     {
> > > > +             .sensor_type = OSF_SENSOR_TEMPERATURE,
> > > > +             .channel_count = 1,
> > > > +             .name = "osf-temp",
> > > > +             .channels = osf_temp_channels,
> > > > +             .num_channels = ARRAY_SIZE(osf_temp_channels),
> > > > +     },
> > > > +};
> > > > +
> > > > +static const struct osf_iio_sensor_spec *
> > > > +osf_iio_find_sensor_spec(u16 sensor_type, u16 channel_count)
> > > > +{
> > > > +     for (unsigned int i = 0; i < ARRAY_SIZE(osf_iio_sensor_specs); i++) {
> > > > +             if (osf_iio_sensor_specs[i].sensor_type == sensor_type &&
> > > > +                 osf_iio_sensor_specs[i].channel_count == channel_count)
> > > > +                     return &osf_iio_sensor_specs[i];
> > > > +     }
> > > > +
> > > > +     return NULL;
> > > > +}
> > > > +
> > > > +bool osf_iio_sensor_supported(u16 sensor_type, u16 channel_count)
> > > > +{
> > > > +     if (osf_iio_find_sensor_spec(sensor_type, channel_count))
> > > > +             return true;
> > > > +
> > > > +     return false  
> > >
> > >         return osf_iio_find_sensor_spec(sensor_type, channel_count) != NULL;
> > >  
> > > > +}  
> > >  
> > > > +
> > > > +static int osf_iio_buffer_postenable(struct iio_dev *indio_dev)
> > > > +{
> > > > +     struct osf_iio_state *state = iio_priv(indio_dev);
> > > > +
> > > > +     guard(mutex)(&state->buffer_lock);  
> > >
> > > These transistions are already guarded by the core, so see the question later
> > > on whether you can just claim buffered mode in the data handler
> > > to avoid the need for this. That might not work but the way you did
> > > this got me thinking about a more general solution to the problem.
> > >  
> > > > +     state->buffer_active = true;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int osf_iio_buffer_predisable(struct iio_dev *indio_dev)
> > > > +{
> > > > +     struct osf_iio_state *state = iio_priv(indio_dev);
> > > > +
> > > > +     /* Wait for the current push before the IIO core changes its buffers. */
> > > > +     guard(mutex)(&state->buffer_lock);
> > > > +     state->buffer_active = false;
> > > > +
> > > > +     return 0;
> > > > +}  
> > >
> > >  
> > > > +
> > > > +int osf_iio_push_sample(struct iio_dev *indio_dev, const s32 *values,
> > > > +                     u16 channel_count)
> > > > +{
> > > > +     struct osf_iio_state *state = iio_priv(indio_dev);
> > > > +     s64 timestamp;
> > > > +
> > > > +     if (channel_count != state->spec->channel_count)
> > > > +             return -EPROTO;
> > > > +
> > > > +     guard(mutex)(&state->buffer_lock);  
> > >
> > > Using an extra lock is one solution I suppose to the risk
> > > of this racing with the buffer tear down and the problems
> > > around active_scan_mask being freed. Could you use
> > >
> > >         if (iio_device_try_claim_buffer_mode(indio_dev))
> > > and later a release on that to achieve the same result?
> > >
> > > Only the ade9000 is doing similar at the moment so would need
> > > some testing to make sure no lock inversions occur in general
> > > but given you don't take any other locks in here it should be fine.
> > >
> > >  
> > > > +     if (!state->buffer_active || !iio_buffer_enabled(indio_dev))
> > > > +             return 0;
> > > > +
> > > > +     timestamp = iio_get_time_ns(indio_dev);
> > > > +
> > > > +     switch (channel_count) {
> > > > +     case 1: {
> > > > +             struct osf_iio_scan_1axis scan = {
> > > > +                     .value = values[0],
> > > > +             };  
> > > As suggested above, this structure type is only used locally here. So
> > > make the definition local as well.
> > >                 struct {
> > >                         s32 value;
> > >                         aligned_s64 timestamp;
> > >                 } scan = { };
> > >  
> > > > +
> > > > +             return iio_push_to_buffers_with_ts(indio_dev, &scan,
> > > > +                                                sizeof(scan), timestamp);
> > > > +     }
> > > > +     case 3: {
> > > > +             struct osf_iio_scan_3axis scan = {
> > > > +                     .values = { },  
> > >
> > > Setting values doesn't add anything useful as = { }
> > > zeros the whole structure (including any holes - they kernel is
> > > built with specific compiler options to ensure that last bit
> > > as it wasn't required in older c specs).
> > >  
> > > > +             };
> > > > +             unsigned int channel, index = 0;  
> > >
> > > Please split declarations that assign and ones that don't on to separate lines.
> > > Ends up a little more readable.
> > >                 unsigned int index = 0;
> > >                 unsigned int channel;
> > >  
> > > > +
> > > > +             /* Pack the active channels; unused storage remains initialized. */
> > > > +             for_each_set_bit(channel, indio_dev->active_scan_mask, channel_count)
> > > > +                     scan.values[index++] = values[channel];
> > > > +
> > > > +             return iio_push_to_buffers_with_ts(indio_dev, &scan,
> > > > +                                                sizeof(scan), timestamp);
> > > > +     }
> > > > +     default:
> > > > +             return -EPROTO;
> > > > +     }
> > > > +}  
> > >  
> > > > diff --git a/drivers/iio/opensensorfusion/osf_serdev.c b/drivers/iio/opensensorfusion/osf_serdev.c
> > > > index 8a747c01ff9d..3d5e90d83967 100644
> > > > --- a/drivers/iio/opensensorfusion/osf_serdev.c
> > > > +++ b/drivers/iio/opensensorfusion/osf_serdev.c
> > > > @@ -23,7 +23,7 @@ struct osf_serdev {
> > > >       bool vcc_enabled;
> > > >       struct osf_device osf;
> > > >       struct osf_stream stream;
> > > > -     /* Protects the parser and all RX counters. */
> > > > +     /* Protects the parser, all RX counters, and the registration gate. */
> > > >       struct mutex rx_lock;  
> > >
> > > Just name it 'lock' given it has scope beyond rx.  Do that in earlier patch
> > > though rather than changing it here.
> > >  
> > > >  };  
> > >  
> > > >
> > > > @@ -139,5 +144,5 @@ static struct serdev_device_driver osf_serdev_driver = {
> > > >  };
> > > >  module_serdev_device_driver(osf_serdev_driver);
> > > >
> > > > -MODULE_DESCRIPTION("Open Sensor Fusion UART receiver");
> > > > +MODULE_DESCRIPTION("Open Sensor Fusion IIO driver");  
> > >
> > > Just have that being slightly missleading in earlier patch to reduce churn here.
> > >  
> > > >  MODULE_LICENSE("GPL");  
> > >