Re: [PATCH v1 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 18 May 2022 12:25:59 +0000
Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

> Hi Jonathan,
> 
> I have two items to be discussed about iio_trigger_get().
> Please see my questions below and correct me if I'm wrong.
> 
> On Tue, Apr 26, 2022 at 08:24:10PM +0300, Dmitry Rokosov wrote:
> > > > +							       "%s-new-data",
> > > > +							       indio_dev->name);
> > > > +		if (!msa311->new_data_trig) {
> > > > +			dev_err(&i2c->dev, "cannot allocate new data trig\n");
> > > > +			err = -ENOMEM;
> > > > +			goto err_lock_destroy;
> > > > +		}
> > > > +
> > > > +		msa311->new_data_trig->dev.parent = &i2c->dev;
> > > > +		msa311->new_data_trig->ops = &msa311_new_data_trig_ops;
> > > > +		iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
> > > > +		indio_dev->trig = msa311->new_data_trig;  
> > > 
> > > This will create a double free if you were to change the trigger.
> > > 		indio_dev->trig = iio_trigger_get(trig);
> > >   
> > 
> > I didn't take into account other trigger usage.
> > I'll rework this place for the v2.
> >   
> 
> The first one problem is module_get() calling for trigger get()
> semantic.
> I've applied iio_trigger_get() function to acquire module refcnt,
> but I've faced with rmmod busy problem. IIO driver module doesn't want to
> stop and unload due to not having zero module refcnt.

One option is to remove the trigger from sysfs - write an empty string
current_trigger, but you are right this is a bit of a mess.

Probably the best option is just don't assign the trigger automatically at all.

This was what we almost always went with in the past.  If a driver
supports multiple triggers (and if it doesn't why expose the trigger at allm
there is no obligation to do so?)
then it's a policy decision to associate a trigger in the first place
so shouldn't really happen in kernel.

There is a corner case for drivers which can only use a particular trigger,
but that trigger can be more generally used (validate_trigger provided, but
not validate_device).  Another corner case is drivers that didn't expose
a trigger, but later gain support for other triggers then we need to set
the default value.


> Syscall delete_module() tries to stop module first and after calls
> driver exit() function (which executes devm_* handlers inside, including IIO
> trigger unregister). It means we have the chicken or the egg dilemma here.
> Module can't be unloaded until module refcnt is not zero and we can't
> execute IIO trigger unregister (decrease module refcnt) only when module
> refcnt is zero.
> I suppose the possible solution to such a problem is a different semantic
> for internal triggers (inside driver itself) and external drivers (like
> hwtimer trigger). What do you think?

Potentially though it's going to be tricky as a driver doesn't generally
have any way to know they are internal and we need to be careful not to
underflow the reference counts.  We could hid a flag somewhere and
add an iio_trigger_get_same_owner() or something that sets that flag allowing
us to decide not to drop the reference count it if is automatically unassociated.
In the path where you get:
1) iio_trigger_get_same_owner() on probe
2) sysfs write changes to another trigger.
3) sysfs write back to original trigger
it is reasonable to assume the need to clear the trigger
before driver removal is possible, whereas clearing the trigger association
if only step 1 happened is no intuitive.

> 
> The second one issue is located in the different IIO drivers. Some modules
> call iio_trigger_get() before iio_trigger_register(), trig->owner is not
> initialized to the right value (THIS_MODULE) and we don't acquire refcnt
> for proper driver object.

Ah. Good point. I guess we missed that when we were moving over to
automated setting of the module.

> I'm going to send patchset to problem driver set, but I can test only
> buildable status for such modules, are you okay with that?
That should be fine.  I can't immediately think of a case where it would
be a problem as the iio_device_register() should be later and until that happens
nothing can turn on the trigger - so there shouldn't be any other races.

Jonathan

>