Revert "mfd: cros_ec: Use devm_kzalloc for private data"
diff mbox series

Message ID 20181204155843.11411-1-enric.balletbo@collabora.com
State In Next
Commit 48a2ca0ee3994df53da230c7079a18a70ec914f9
Headers show
Series
  • Revert "mfd: cros_ec: Use devm_kzalloc for private data"
Related show

Commit Message

Enric Balletbo i Serra Dec. 4, 2018, 3:58 p.m. UTC
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(-)

Comments

Guenter Roeck Dec. 4, 2018, 4:07 p.m. UTC | #1
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
>
Dmitry Torokhov Dec. 4, 2018, 4:13 p.m. UTC | #2
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
> >
Lee Jones Dec. 5, 2018, 10:05 a.m. UTC | #3
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.

Patch
diff mbox series

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) {