Message-ID: <CABXOdTehDNf_8DHjua6BTJ-_AA6wcfcis1DPFS5=93sEh-6LQA@mail.gmail.com>
Date:   Wed, 28 Nov 2018 17:17:22 -0800
From:   Guenter Roeck <groeck@...gle.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Lee Jones <lee.jones@...aro.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Nicolas Boichat <drinkcat@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Guenter Roeck <groeck@...omium.org>, kernel@...labora.com,
        Benson Leung <bleung@...omium.org>
Subject: Re: [PATCH v3 8/8] mfd: cros_ec: add a dev_release empty method.

Hi Greg,

On Tue, Nov 27, 2018 at 9:52 AM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Tue, Nov 27, 2018 at 09:29:38AM -0800, Guenter Roeck wrote:
> > Hi Enric,
> >
> > On Tue, Nov 27, 2018 at 4:19 AM Enric Balletbo i Serra
> > <enric.balletbo@...labora.com> wrote:
> > >
> > > Devices are required to provide a release method. This patch fixes the
> > > following WARN():
> > >
> > > [   47.218707] ------------[ cut here ]------------
> > > [   47.223901] Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
> > > [   47.234430] WARNING: CPU: 0 PID: 3585 at drivers/base/core.c:895 device_release+0x80/0x90
> > > [   47.243560] Modules linked in: btusb btrtl btintel btbcm bluetooth ecdh_generic [...]
> > > [   47.323851] CPU: 0 PID: 3585 Comm: rmmod Not tainted 4.20.0-rc2+ #29
> > > [   47.330947] Hardware name: Google Kevin (DT)
> > > [   47.335714] pstate: 40000005 (nZcv daif -PAN -UAO)
> > > [   47.341063] pc : device_release+0x80/0x90
> > > [   47.345537] lr : device_release+0x80/0x90
> > > [   47.350001] sp : ffff00000b17bc70
> > > [   47.353698] x29: ffff00000b17bc70 x28: ffff8000e48e9a80
> > > [   47.359629] x27: 0000000000000000 x26: 0000000000000000
> > > [   47.365561] x25: 0000000056000000 x24: 0000000000000015
> > > [   47.371492] x23: ffff8000f0248060 x22: ffff000000b700a0
> > > [   47.377414] x21: ffff8000edf56100 x20: ffff8000edd13028
> > > [   47.383346] x19: ffff8000edd13018 x18: 0000000000000095
> > > [   47.389278] x17: 0000000000000000 x16: 0000000000000000
> > > [   47.395209] x15: 0000000000000400 x14: 0000000000000400
> > > [   47.401131] x13: 00000000000001a7 x12: 0000000000000000
> > > [   47.407053] x11: 0000000000000001 x10: 0000000000000960
> > > [   47.412976] x9 : ffff00000b17b9b0 x8 : ffff8000e48ea440
> > > [   47.418898] x7 : ffff8000ee9090c0 x6 : ffff8000f7d0b0b8
> > > [   47.424830] x5 : ffff8000f7d0b0b8 x4 : 0000000000000000
> > > [   47.430752] x3 : ffff8000f7d11e68 x2 : ffff8000e48e9a80
> > > [   47.436674] x1 : 37d859939c964800 x0 : 0000000000000000
> > > [   47.442597] Call trace:
> > > [   47.445324]  device_release+0x80/0x90
> > > [   47.449414]  kobject_put+0x74/0xe8
> > > [   47.453210]  device_unregister+0x20/0x30
> > > [   47.457592]  ec_device_remove+0x34/0x48 [cros_ec_dev]
> > > [   47.463233]  platform_drv_remove+0x28/0x48
> > > [   47.467805]  device_release_driver_internal+0x1a8/0x240
> > > [   47.473630]  driver_detach+0x40/0x80
> > > [   47.477609]  bus_remove_driver+0x54/0xa8
> > > [   47.481986]  driver_unregister+0x2c/0x58
> > > [   47.486355]  platform_driver_unregister+0x10/0x18
> > > [   47.491599]  cros_ec_dev_exit+0x1c/0x258 [cros_ec_dev]
> > > [   47.497338]  __arm64_sys_delete_module+0x16c/0x1f8
> > > [   47.502689]  el0_svc_common+0x84/0xd8
> > > [   47.506776]  el0_svc_handler+0x2c/0x80
> > > [   47.510960]  el0_svc+0x8/0xc
> > > [   47.514171] ---[ end trace 9087279fc8c03450 ]---
> > >
> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> > > ---
> > >
> > > Changes in v3: None
> > > Changes in v2:
> > > - Fix WARN when unloading. This is new in these series.
> > >
> > >  drivers/mfd/cros_ec_dev.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > index 1ba98a32715e..cdb941c6db98 100644
> > > --- a/drivers/mfd/cros_ec_dev.c
> > > +++ b/drivers/mfd/cros_ec_dev.c
> > > @@ -35,9 +35,14 @@
> > >  #define CROS_MAX_DEV 128
> > >  static int ec_major;
> > >
> > > +static void cros_ec_dev_release(struct device *dev)
> > > +{
> > > +}
>
> Yeah, as part of the in-kernel documentation, I now get to make fun of
> you in public!
>
> You did read the documentation, right?
>

To be fair, the problem is difficult to understand. Maybe it is easy
for you, but that is not true for everyone, including me. Remember the
block discussion we just had ? As for the in-kernel documentation,
maybe there is a comprehensive explanation someone, one that clueless
people like me can understand, but all I found was

"If a bus driver unregisters a device, it should not immediately free
  it. It should instead wait for the driver model core to call the
  device's release method, then free the bus-specific object.
  (There may be other code that is currently referencing the device
  structure, and it would be rude to free the device while that is
  happening)"

Does that apply to mfd devices ? What other code may that be that
accesses the structure ? What else does it mean, or in other words,
what other cleanup code besides releasing the data structure needs to
reside in the release function ?

Sure, it would be easy to use kzalloc() instead of devm_kzalloc(), and
to release the memory in the class release function, but does that
actually _fix_ the problem ? Is dummy_sysfs_dev_release() in the
extcon core buggy, or is there a reason why the dummy function works
there ? I for my part have no idea whatsoever, and, sorry, the
documentation doesn't really help me to find out.

Guenter

> {sigh}
>
> Step back and think about _why_ the kernel is telling you this is not
> ok, and ponder the process that came to you that resulted in, "I know,
> let me trick the kernel, I know better than it!".  I didn't just put
> that warning there for no good reason, I was trying to save yourself
> from a major bug.  And here you go working around it, and making that
> bug almost impossible to now find.
>
> The kernel told you this, look:
>
> > > [   47.223901] Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
>
> How much more blatent and obvious can I make it in a single sentance
> without it being rude?
>
> Anyway, no, this is not what to do.  You have to actually realease the
> memory you created for your device in this function.  If you do not do
> that, you have a major bug.  Please fix this properly and next time, do
> not ignore what the kernel is trying to tell you.
>
> thanks,
>
> greg k-h

Powered by blists - more mailing lists