* [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data"
@ 2018-12-04 15:58 Enric Balletbo i Serra
2018-12-04 16:07 ` Guenter Roeck
2018-12-05 10:05 ` Lee Jones
0 siblings, 2 replies; 4+ messages in thread
From: Enric Balletbo i Serra @ 2018-12-04 15:58 UTC (permalink / raw)
To: lee.jones
Cc: gwendal, gregkh, linux-kernel, groeck, kernel, bleung, dtor, stable
This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.
That commit triggered a new WARN when unloading the module (see at the
end of the commit message). When a class_dev is embedded in a structure
then that class_dev is the thing that controls the lifetime of that
structure, for that reason device managed allocations can't be used here.
See Documentation/kobject.txt.
Revert the above patch, so the struct is allocated using kzalloc and we
have a release function for it that frees the allocated memory, otherwise
it is broken.
------------[ cut here ]------------
Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
WARNING: CPU: 3 PID: 3675 at drivers/base/core.c:895 device_release+0x80/0x90
Modules linked in: btusb btrtl btintel btbcm bluetooth ...
CPU: 3 PID: 3675 Comm: rmmod Not tainted 4.20.0-rc4 #76
Hardware name: Google Kevin (DT)
pstate: 40000005 (nZcv daif -PAN -UAO)
pc : device_release+0x80/0x90
lr : device_release+0x80/0x90
sp : ffff00000c47bc70
x29: ffff00000c47bc70 x28: ffff8000e86b0d40
x27: 0000000000000000 x26: 0000000000000000
x25: 0000000056000000 x24: 0000000000000015
x23: ffff8000f0bbf860 x22: ffff000000d320a0
x21: ffff8000ee93e100 x20: ffff8000ed931428
x19: ffff8000ed931418 x18: 0000000000000020
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000000400 x14: 0000000000000143
x13: 0000000000000000 x12: 0000000000000400
x11: 0000000000000157 x10: 0000000000000960
x9 : ffff00000c47b9b0 x8 : ffff8000e86b1700
x7 : 0000000000000000 x6 : ffff8000f7d520b8
x5 : ffff8000f7d520b8 x4 : 0000000000000000
x3 : ffff8000f7d58e68 x2 : ffff8000e86b0d40
x1 : 37d859939c964800 x0 : 0000000000000000
Call trace:
device_release+0x80/0x90
kobject_put+0x74/0xe8
device_unregister+0x20/0x30
ec_device_remove+0x34/0x48 [cros_ec_dev]
platform_drv_remove+0x28/0x48
device_release_driver_internal+0x1a8/0x240
driver_detach+0x40/0x80
bus_remove_driver+0x54/0xa8
driver_unregister+0x2c/0x58
platform_driver_unregister+0x10/0x18
cros_ec_dev_exit+0x1c/0x2d8 [cros_ec_dev]
__arm64_sys_delete_module+0x16c/0x1f8
el0_svc_common+0x84/0xd8
el0_svc_handler+0x2c/0x80
el0_svc+0x8/0xc
---[ end trace a57c4625f3c60ae8 ]---
Cc: stable@vger.kernel.org
Fixes: 3aa2177e4787 ("mfd: cros_ec: Use devm_kzalloc for private data")
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/mfd/cros_ec_dev.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 8f9d6964173e..b99a194ce5a4 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -263,6 +263,11 @@ static const struct file_operations fops = {
#endif
};
+static void cros_ec_class_release(struct device *dev)
+{
+ kfree(to_cros_ec_dev(dev));
+}
+
static void cros_ec_sensors_register(struct cros_ec_dev *ec)
{
/*
@@ -395,7 +400,7 @@ static int ec_device_probe(struct platform_device *pdev)
int retval = -ENOMEM;
struct device *dev = &pdev->dev;
struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
- struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
+ struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
if (!ec)
return retval;
@@ -417,6 +422,7 @@ static int ec_device_probe(struct platform_device *pdev)
ec->class_dev.devt = MKDEV(ec_major, pdev->id);
ec->class_dev.class = &cros_class;
ec->class_dev.parent = dev;
+ ec->class_dev.release = cros_ec_class_release;
retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name);
if (retval) {
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data"
2018-12-04 15:58 [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data" Enric Balletbo i Serra
@ 2018-12-04 16:07 ` Guenter Roeck
2018-12-04 16:13 ` Dmitry Torokhov
2018-12-05 10:05 ` Lee Jones
1 sibling, 1 reply; 4+ messages in thread
From: Guenter Roeck @ 2018-12-04 16:07 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Lee Jones, Gwendal Grignou, Greg Kroah-Hartman, linux-kernel,
Guenter Roeck, kernel, Benson Leung, Dmitry Torokhov, # v4 . 10+
On Tue, Dec 4, 2018 at 7:58 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.
>
> That commit triggered a new WARN when unloading the module (see at the
> end of the commit message). When a class_dev is embedded in a structure
> then that class_dev is the thing that controls the lifetime of that
> structure, for that reason device managed allocations can't be used here.
> See Documentation/kobject.txt.
>
> Revert the above patch, so the struct is allocated using kzalloc and we
> have a release function for it that frees the allocated memory, otherwise
> it is broken.
>
> ------------[ cut here ]------------
> Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
> WARNING: CPU: 3 PID: 3675 at drivers/base/core.c:895 device_release+0x80/0x90
> Modules linked in: btusb btrtl btintel btbcm bluetooth ...
> CPU: 3 PID: 3675 Comm: rmmod Not tainted 4.20.0-rc4 #76
> Hardware name: Google Kevin (DT)
> pstate: 40000005 (nZcv daif -PAN -UAO)
> pc : device_release+0x80/0x90
> lr : device_release+0x80/0x90
> sp : ffff00000c47bc70
> x29: ffff00000c47bc70 x28: ffff8000e86b0d40
> x27: 0000000000000000 x26: 0000000000000000
> x25: 0000000056000000 x24: 0000000000000015
> x23: ffff8000f0bbf860 x22: ffff000000d320a0
> x21: ffff8000ee93e100 x20: ffff8000ed931428
> x19: ffff8000ed931418 x18: 0000000000000020
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000000400 x14: 0000000000000143
> x13: 0000000000000000 x12: 0000000000000400
> x11: 0000000000000157 x10: 0000000000000960
> x9 : ffff00000c47b9b0 x8 : ffff8000e86b1700
> x7 : 0000000000000000 x6 : ffff8000f7d520b8
> x5 : ffff8000f7d520b8 x4 : 0000000000000000
> x3 : ffff8000f7d58e68 x2 : ffff8000e86b0d40
> x1 : 37d859939c964800 x0 : 0000000000000000
> Call trace:
> device_release+0x80/0x90
> kobject_put+0x74/0xe8
> device_unregister+0x20/0x30
> ec_device_remove+0x34/0x48 [cros_ec_dev]
> platform_drv_remove+0x28/0x48
> device_release_driver_internal+0x1a8/0x240
> driver_detach+0x40/0x80
> bus_remove_driver+0x54/0xa8
> driver_unregister+0x2c/0x58
> platform_driver_unregister+0x10/0x18
> cros_ec_dev_exit+0x1c/0x2d8 [cros_ec_dev]
> __arm64_sys_delete_module+0x16c/0x1f8
> el0_svc_common+0x84/0xd8
> el0_svc_handler+0x2c/0x80
> el0_svc+0x8/0xc
> ---[ end trace a57c4625f3c60ae8 ]---
>
> Cc: stable@vger.kernel.org
> Fixes: 3aa2177e4787 ("mfd: cros_ec: Use devm_kzalloc for private data")
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
> ---
>
> drivers/mfd/cros_ec_dev.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 8f9d6964173e..b99a194ce5a4 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -263,6 +263,11 @@ static const struct file_operations fops = {
> #endif
> };
>
> +static void cros_ec_class_release(struct device *dev)
> +{
> + kfree(to_cros_ec_dev(dev));
> +}
> +
> static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> {
> /*
> @@ -395,7 +400,7 @@ static int ec_device_probe(struct platform_device *pdev)
> int retval = -ENOMEM;
> struct device *dev = &pdev->dev;
> struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
> - struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> + struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
>
> if (!ec)
> return retval;
> @@ -417,6 +422,7 @@ static int ec_device_probe(struct platform_device *pdev)
> ec->class_dev.devt = MKDEV(ec_major, pdev->id);
> ec->class_dev.class = &cros_class;
> ec->class_dev.parent = dev;
> + ec->class_dev.release = cros_ec_class_release;
>
> retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name);
> if (retval) {
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data"
2018-12-04 16:07 ` Guenter Roeck
@ 2018-12-04 16:13 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2018-12-04 16:13 UTC (permalink / raw)
To: Guenter Roeck
Cc: enric.balletbo, Lee Jones, gwendal, Greg Kroah-Hartman, lkml,
Guenter Roeck, kernel, Benson Leung, stable
On Tue, Dec 4, 2018 at 8:07 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Tue, Dec 4, 2018 at 7:58 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.
> >
> > That commit triggered a new WARN when unloading the module (see at the
> > end of the commit message). When a class_dev is embedded in a structure
> > then that class_dev is the thing that controls the lifetime of that
> > structure, for that reason device managed allocations can't be used here.
> > See Documentation/kobject.txt.
> >
> > Revert the above patch, so the struct is allocated using kzalloc and we
> > have a release function for it that frees the allocated memory, otherwise
> > it is broken.
> >
> > ------------[ cut here ]------------
> > Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
> > WARNING: CPU: 3 PID: 3675 at drivers/base/core.c:895 device_release+0x80/0x90
> > Modules linked in: btusb btrtl btintel btbcm bluetooth ...
> > CPU: 3 PID: 3675 Comm: rmmod Not tainted 4.20.0-rc4 #76
> > Hardware name: Google Kevin (DT)
> > pstate: 40000005 (nZcv daif -PAN -UAO)
> > pc : device_release+0x80/0x90
> > lr : device_release+0x80/0x90
> > sp : ffff00000c47bc70
> > x29: ffff00000c47bc70 x28: ffff8000e86b0d40
> > x27: 0000000000000000 x26: 0000000000000000
> > x25: 0000000056000000 x24: 0000000000000015
> > x23: ffff8000f0bbf860 x22: ffff000000d320a0
> > x21: ffff8000ee93e100 x20: ffff8000ed931428
> > x19: ffff8000ed931418 x18: 0000000000000020
> > x17: 0000000000000000 x16: 0000000000000000
> > x15: 0000000000000400 x14: 0000000000000143
> > x13: 0000000000000000 x12: 0000000000000400
> > x11: 0000000000000157 x10: 0000000000000960
> > x9 : ffff00000c47b9b0 x8 : ffff8000e86b1700
> > x7 : 0000000000000000 x6 : ffff8000f7d520b8
> > x5 : ffff8000f7d520b8 x4 : 0000000000000000
> > x3 : ffff8000f7d58e68 x2 : ffff8000e86b0d40
> > x1 : 37d859939c964800 x0 : 0000000000000000
> > Call trace:
> > device_release+0x80/0x90
> > kobject_put+0x74/0xe8
> > device_unregister+0x20/0x30
> > ec_device_remove+0x34/0x48 [cros_ec_dev]
> > platform_drv_remove+0x28/0x48
> > device_release_driver_internal+0x1a8/0x240
> > driver_detach+0x40/0x80
> > bus_remove_driver+0x54/0xa8
> > driver_unregister+0x2c/0x58
> > platform_driver_unregister+0x10/0x18
> > cros_ec_dev_exit+0x1c/0x2d8 [cros_ec_dev]
> > __arm64_sys_delete_module+0x16c/0x1f8
> > el0_svc_common+0x84/0xd8
> > el0_svc_handler+0x2c/0x80
> > el0_svc+0x8/0xc
> > ---[ end trace a57c4625f3c60ae8 ]---
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 3aa2177e4787 ("mfd: cros_ec: Use devm_kzalloc for private data")
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
Indeed, we need to be mindful that refcounted objects should not be
allocated with devm_kzalloc().
Reviewed-by: Dmitry Torokhov <dtor@chromium.org>
>
> > ---
> >
> > drivers/mfd/cros_ec_dev.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > index 8f9d6964173e..b99a194ce5a4 100644
> > --- a/drivers/mfd/cros_ec_dev.c
> > +++ b/drivers/mfd/cros_ec_dev.c
> > @@ -263,6 +263,11 @@ static const struct file_operations fops = {
> > #endif
> > };
> >
> > +static void cros_ec_class_release(struct device *dev)
> > +{
> > + kfree(to_cros_ec_dev(dev));
> > +}
> > +
> > static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > {
> > /*
> > @@ -395,7 +400,7 @@ static int ec_device_probe(struct platform_device *pdev)
> > int retval = -ENOMEM;
> > struct device *dev = &pdev->dev;
> > struct cros_ec_platform *ec_platform = dev_get_platdata(dev);
> > - struct cros_ec_dev *ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
> > + struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL);
> >
> > if (!ec)
> > return retval;
> > @@ -417,6 +422,7 @@ static int ec_device_probe(struct platform_device *pdev)
> > ec->class_dev.devt = MKDEV(ec_major, pdev->id);
> > ec->class_dev.class = &cros_class;
> > ec->class_dev.parent = dev;
> > + ec->class_dev.release = cros_ec_class_release;
> >
> > retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name);
> > if (retval) {
> > --
> > 2.19.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data"
2018-12-04 15:58 [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data" Enric Balletbo i Serra
2018-12-04 16:07 ` Guenter Roeck
@ 2018-12-05 10:05 ` Lee Jones
1 sibling, 0 replies; 4+ messages in thread
From: Lee Jones @ 2018-12-05 10:05 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: gwendal, gregkh, linux-kernel, groeck, kernel, bleung, dtor, stable
On Tue, 04 Dec 2018, Enric Balletbo i Serra wrote:
> This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.
>
> That commit triggered a new WARN when unloading the module (see at the
> end of the commit message). When a class_dev is embedded in a structure
> then that class_dev is the thing that controls the lifetime of that
> structure, for that reason device managed allocations can't be used here.
> See Documentation/kobject.txt.
>
> Revert the above patch, so the struct is allocated using kzalloc and we
> have a release function for it that frees the allocated memory, otherwise
> it is broken.
>
> ------------[ cut here ]------------
> Device 'cros_ec' does not have a release() function, it is broken and must be fixed.
> WARNING: CPU: 3 PID: 3675 at drivers/base/core.c:895 device_release+0x80/0x90
> Modules linked in: btusb btrtl btintel btbcm bluetooth ...
> CPU: 3 PID: 3675 Comm: rmmod Not tainted 4.20.0-rc4 #76
> Hardware name: Google Kevin (DT)
> pstate: 40000005 (nZcv daif -PAN -UAO)
> pc : device_release+0x80/0x90
> lr : device_release+0x80/0x90
> sp : ffff00000c47bc70
> x29: ffff00000c47bc70 x28: ffff8000e86b0d40
> x27: 0000000000000000 x26: 0000000000000000
> x25: 0000000056000000 x24: 0000000000000015
> x23: ffff8000f0bbf860 x22: ffff000000d320a0
> x21: ffff8000ee93e100 x20: ffff8000ed931428
> x19: ffff8000ed931418 x18: 0000000000000020
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000000400 x14: 0000000000000143
> x13: 0000000000000000 x12: 0000000000000400
> x11: 0000000000000157 x10: 0000000000000960
> x9 : ffff00000c47b9b0 x8 : ffff8000e86b1700
> x7 : 0000000000000000 x6 : ffff8000f7d520b8
> x5 : ffff8000f7d520b8 x4 : 0000000000000000
> x3 : ffff8000f7d58e68 x2 : ffff8000e86b0d40
> x1 : 37d859939c964800 x0 : 0000000000000000
> Call trace:
> device_release+0x80/0x90
> kobject_put+0x74/0xe8
> device_unregister+0x20/0x30
> ec_device_remove+0x34/0x48 [cros_ec_dev]
> platform_drv_remove+0x28/0x48
> device_release_driver_internal+0x1a8/0x240
> driver_detach+0x40/0x80
> bus_remove_driver+0x54/0xa8
> driver_unregister+0x2c/0x58
> platform_driver_unregister+0x10/0x18
> cros_ec_dev_exit+0x1c/0x2d8 [cros_ec_dev]
> __arm64_sys_delete_module+0x16c/0x1f8
> el0_svc_common+0x84/0xd8
> el0_svc_handler+0x2c/0x80
> el0_svc+0x8/0xc
> ---[ end trace a57c4625f3c60ae8 ]---
>
> Cc: stable@vger.kernel.org
> Fixes: 3aa2177e4787 ("mfd: cros_ec: Use devm_kzalloc for private data")
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> drivers/mfd/cros_ec_dev.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-05 10:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-04 15:58 [PATCH] Revert "mfd: cros_ec: Use devm_kzalloc for private data" Enric Balletbo i Serra
2018-12-04 16:07 ` Guenter Roeck
2018-12-04 16:13 ` Dmitry Torokhov
2018-12-05 10:05 ` Lee Jones
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).