RE: [EXT] Re: [PATCH] iio: accell: mma8452: free iio trigger pointer when cleanup

[Anson Huang]

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)