Hi, Jonathan Best Regards! Anson Huang > -----Original Message----- > From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] > Sent: 2019年4月7日 18:40 > To: Anson Huang <anson.huang@xxxxxxx> > Cc: knaack.h@xxxxxx; lars@xxxxxxxxxx; pmeerw@xxxxxxxxxx; Leonard > Crestez <leonard.crestez@xxxxxxx>; gustavo@xxxxxxxxxxxxxx; > martink@xxxxxxxxx; rtresidd@xxxxxxxxxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: [EXT] Re: [PATCH] iio: accell: mma8452: free iio trigger pointer when > cleanup > > WARNING: This email was created outside of NXP. DO NOT CLICK links or > attachments unless you recognize the sender and know the content is safe. > > > > On Thu, 4 Apr 2019 08:02:05 +0000 > Anson Huang <anson.huang@xxxxxxx> wrote: > > > When mma8452 is built as module, once it is insmod and rmmod, below > > kernel dump will show out, the root cause is module being put twice if > > iio trigger pointer is NOT NULL, this patch frees iio trigger pointer > > after iio trigger is unregistered to avoid below kernel > > dump: > > > > ------------[ cut here ]------------ > > WARNING: CPU: 3 PID: 270 at kernel/module.c:1145 > module_put+0xd0/0x188 > > Modules linked in: mma8452(-) > > CPU: 3 PID: 270 Comm: rmmod Not tainted > > 5.1.0-rc3-next-20190403-00022-g5ede0c9-dirty #1596 Hardware name: > > Freescale i.MX6 Quad/DualLite (Device Tree) [<c011272c>] > > (unwind_backtrace) from [<c010d094>] (show_stack+0x10/0x14) > > [<c010d094>] (show_stack) from [<c0bdc160>] (dump_stack+0xd8/0x10c) > > [<c0bdc160>] (dump_stack) from [<c01275d8>] (__warn+0xf8/0x124) > > [<c01275d8>] (__warn) from [<c0127714>] > (warn_slowpath_null+0x3c/0x48) > > [<c0127714>] (warn_slowpath_null) from [<c01ca4d0>] > > (module_put+0xd0/0x188) [<c01ca4d0>] (module_put) from [<c08a5bb4>] > > (iio_device_unregister_trigger_consumer+0x18/0x24) > > [<c08a5bb4>] (iio_device_unregister_trigger_consumer) from > > [<c08a0ab4>] (iio_dev_release+0x20/0) [<c08a0ab4>] (iio_dev_release) > > from [<c065c0e8>] (device_release+0x24/0x8c) [<c065c0e8>] > > (device_release) from [<c0be1810>] (kobject_put+0x74/0xd4) > > [<c0be1810>] (kobject_put) from [<c0664dc0>] > > (release_nodes+0x16c/0x1f0) [<c0664dc0>] (release_nodes) from > > [<c0661b80>] (device_release_driver_internal+0xec/0x1a0) > > [<c0661b80>] (device_release_driver_internal) from [<c0661c90>] > > (driver_detach+0x44/0x80) [<c0661c90>] (driver_detach) from > > [<c066096c>] (bus_remove_driver+0x4c/0xa0) [<c066096c>] > > (bus_remove_driver) from [<c01cc1a4>] (sys_delete_module+0x130/0x1dc) > > [<c01cc1a4>] (sys_delete_module) from [<c0101000>] > (ret_fast_syscall+0x0/0x28) Exception stack(0xe8d87fa8 to 0xe8d87ff0) > > 7fa0: 0001ffc0 38616d6d be87bbf0 00000880 00000000 be87be98 > > 7fc0: 0001ffc0 38616d6d 00323534 00000081 000a9744 be87bf81 000a9790 > > 00000000 > > 7fe0: be87bbe8 be87bbd8 0001fe18 b6e381a0 irq event stamp: 4017 > > hardirqs last enabled at (4035): [<c0188a0c>] > > console_unlock+0x400/0x630 hardirqs last disabled at (4052): > > [<c018868c>] console_unlock+0x80/0x630 softirqs last enabled at > > (4050): [<c0102698>] __do_softirq+0x458/0x554 softirqs last disabled > > at (4069): [<c012ed6c>] irq_exit+0x130/0x180 ---[ end trace > > a7ba8f1823b1e073 ]--- > > > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx> > Good fine, but the fix is not in the best place. The key thing is that any > assignment inside a driver to iio_dev->trig should be done with > iio_trigger_get. > > indio_dev->trig = iio_trigger_get(trig). The intent is to avoid this exact double > free, but also handle it correctly if we are using devm_ for all the handling. > > I just did a grep and there are quite a few drivers not doing this though so it's > one we should be more careful about. > > Hmm. Anyone fancy doing an audit of existing drivers and checking which > ones have this problem? Some seem to do exactly what you have here, but > that isn't a the best pattern to encourage. > > For this one would you mind trying with the iio_trigger_get approach and if > I'm not missing something, send a v2 fixing it that way? With below patch to use iio_trigger_get, looks like try_release_module_ref() will return 1 and cause try_stop_module() return fail and we will see " rmmod: can't unload 'mma8452': Resource temporarily unavailable ", As the module ref count check is before iio_device_unregister_trigger_consumer() is called which will do iio_trigger_put(), so looks like there is still something wrong with the module ref count? +++ b/drivers/iio/accel/mma8452.c @@ -1468,17 +1468,15 @@ static int mma8452_trigger_setup(struct iio_dev *indio_dev) if (ret) return ret; - indio_dev->trig = trig; + indio_dev->trig = iio_trigger_get(trig); Thanks, Anson. > > Thanks > > Jonathan > > > > --- > > drivers/iio/accel/mma8452.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c > > index 3027811..6b18177 100644 > > --- a/drivers/iio/accel/mma8452.c > > +++ b/drivers/iio/accel/mma8452.c > > @@ -1475,8 +1475,10 @@ static int mma8452_trigger_setup(struct iio_dev > > *indio_dev) > > > > static void mma8452_trigger_cleanup(struct iio_dev *indio_dev) { > > - if (indio_dev->trig) > > + if (indio_dev->trig) { > > iio_trigger_unregister(indio_dev->trig); > > + indio_dev->trig = NULL; > > + } > > } > > > > static int mma8452_reset(struct i2c_client *client)
