Re: [PATCH v3 8/8] mfd: cros_ec: add a dev_release empty method.

[Greg Kroah-Hartman]

On Wed, Nov 28, 2018 at 05:17:22PM -0800, Guenter Roeck wrote:
> Hi Greg,
> 
> On Tue, Nov 27, 2018 at 9:52 AM Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > ---
> > > >
> > > > 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 ?

To quote Documentation/kobject.txt:
	One important point cannot be overstated: every kobject must
	have a release() method, and the kobject must persist (in a
	consistent state) until that method is called. If these
	constraints are not met, the code is flawed.  Note that the
	kernel will warn you if you forget to provide a release()
	method.  Do not try to get rid of this warning by providing an
	"empty" release function; you will be mocked mercilessly by the
	kobject maintainer if you attempt this.

The fact that you couldn't even find this means that it probably is in
the wrong place, but then, where is the "right" place for where everyone
can see it?  Should I refer to this file in the kernel error message?

That file also should answer your other questions about lifetime rules
of kobjects, which is really the same thing as 'struct device' here.  If
not, please let me know and I can fix it up.

thanks,

greg k-h