Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
From:   Dmitry Rokosov <DDRokosov@sberdevices.ru>
To:     Jonathan Cameron <Jonathan.Cameron@Huawei.com>
CC:     "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "stano.jakubek@gmail.com" <stano.jakubek@gmail.com>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "jic23@kernel.org" <jic23@kernel.org>,
        "lars@metafoo.de" <lars@metafoo.de>,
        "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
        "stephan@gerhold.net" <stephan@gerhold.net>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        kernel <kernel@sberdevices.ru>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver
Thread-Topic: [PATCH v1 2/3] iio: add MEMSensing MSA311 3-axis accelerometer
 driver
Thread-Index: AQHYVASOybrxT0DjfUWoYWuGUlWOX6z4bouAgAnb/gCAIkALgA==
Date:   Wed, 18 May 2022 12:25:59 +0000
Message-ID: <20220518122515.aby5lbb4xusr6pdt@CAB-WSD-L081021.sigma.sbrf.ru>
References: <20220419154555.24191-1-ddrokosov@sberdevices.ru>
 <20220419154555.24191-3-ddrokosov@sberdevices.ru>
 <20220420115023.00006a25@Huawei.com>
 <20220426172406.s4h6g7nrpytaq263@CAB-WSD-L081021.sigma.sbrf.ru>
In-Reply-To: <20220426172406.s4h6g7nrpytaq263@CAB-WSD-L081021.sigma.sbrf.ru>
Accept-Language: ru-RU, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <AC753801DB3F2E4DBCBE5465066DECA8@sberdevices.ru>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Precedence: bulk

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 =3D -ENOMEM;
> > > +			goto err_lock_destroy;
> > > +		}
> > > +
> > > +		msa311->new_data_trig->dev.parent =3D &i2c->dev;
> > > +		msa311->new_data_trig->ops =3D &msa311_new_data_trig_ops;
> > > +		iio_trigger_set_drvdata(msa311->new_data_trig, indio_dev);
> > > +		indio_dev->trig =3D msa311->new_data_trig;
> >=20
> > This will create a double free if you were to change the trigger.
> > 		indio_dev->trig =3D iio_trigger_get(trig);
> >=20
>=20
> I didn't take into account other trigger usage.
> I'll rework this place for the v2.
>=20

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.
Syscall delete_module() tries to stop module first and after calls
driver exit() function (which executes devm_* handlers inside, including II=
O
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?

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.
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?

--=20
Thank you,
Dmitry