Message-ID: <20190414150805.43420097@archlinux>
Date:   Sun, 14 Apr 2019 15:08:05 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Anson Huang <anson.huang@....com>
Cc:     "knaack.h@....de" <knaack.h@....de>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        Leonard Crestez <leonard.crestez@....com>,
        "gustavo@...eddedor.com" <gustavo@...eddedor.com>,
        "martink@...teo.de" <martink@...teo.de>,
        "rtresidd@...ctromag.com.au" <rtresidd@...ctromag.com.au>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: Re: [EXT] Re: [PATCH] iio: accell: mma8452: free iio trigger
 pointer when cleanup

On Mon, 8 Apr 2019 02:07:24 +0000
Anson Huang <anson.huang@....com> wrote:

> Hi, Jonathan
> 
> Best Regards!
> Anson Huang
> 
> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@...nel.org]
> > Sent: 2019年4月7日 18:40
> > To: Anson Huang <anson.huang@....com>
> > Cc: knaack.h@....de; lars@...afoo.de; pmeerw@...erw.net; Leonard
> > Crestez <leonard.crestez@....com>; gustavo@...eddedor.com;
> > martink@...teo.de; rtresidd@...ctromag.com.au; linux-iio@...r.kernel.org;
> > linux-kernel@...r.kernel.org; dl-linux-imx <linux-imx@....com>
> > 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@....com> 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@....com>  
> > 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? 
Hmm. Drivers that grab their own triggers are a bit of a pain.  We need the
infrastructure to try to prevent trigger drivers going away, but it can
cause self references.

For this particular driver the validation is only that the trigger is not used with
a different device.  You can use a different trigger with this device I believe?

As such, we shouldn't really be setting this default at all.  However, removing
it now will break userspace that is assuming the default trigger is set.
(setting a trigger should always be a userspace decision, rather than hard coded
 unless there is a clear one to one mapping - mind you we have this same problem
 if we have a fixed trigger, though in that case we have trig_readonly set
 so we can know it's happening and there is an easy fix).

The right option, I think, is to remove the trigger manually before removing the
driver.  This is admittedly a bit ugly.

echo '' > /sys/bus/iio/devices/iio:\devicesX/current_trigger.

rmmod mma8452

Can you confirm if that works as expected?

Thanks,

Jonathan
> 
> +++ 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)  
> 

Powered by blists - more mailing lists