[PATCH 1/2] Revert "mfd: cros_ec: Remove unused __remove function"

[PATCH 1/2] Revert "mfd: cros_ec: Remove unused __remove function"

[Dmitry Torokhov]

This reverts commit 556c242045f0c1613aac2e64dc5b2ff0e4bc89e1.

The patch that this change is purported to fix is broken and should be
reverted; thus we reverting this one as well.
---

Lee,

I see the broken code is already in Linus' tree, so please merge send
it on at your earliest convenience.

Thanks,
Dmitry


 drivers/mfd/cros_ec_dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 306e1fd109bd..4b1dbe81fcb6 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -262,6 +262,8 @@ static const struct file_operations fops = {
 #endif
 };
 
+static void __remove(struct device *dev) { }
+
 static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 {
 	/*
-- 
2.18.0.rc1.244.gcf134e6275-goog

[PATCH 2/2] Revert "mfd: cros_ec: Use devm_kzalloc for private data"

[Dmitry Torokhov]

This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.

Linux device objects are refcounted and thus should not be allocated
with devm API as they may outlive the timeframe of another device being
bound to a driver.
---
 drivers/mfd/cros_ec_dev.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index 4b1dbe81fcb6..bd4d4e7ec040 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -262,7 +262,12 @@ static const struct file_operations fops = {
 #endif
 };
 
-static void __remove(struct device *dev) { }
+static void __remove(struct device *dev)
+{
+	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
+					      class_dev);
+	kfree(ec);
+}
 
 static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 {
@@ -388,7 +393,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;
@@ -410,6 +415,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 = __remove;
 
 	retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name);
 	if (retval) {
-- 
2.18.0.rc1.244.gcf134e6275-goog

Re: [PATCH 1/2] Revert "mfd: cros_ec: Remove unused __remove function"

[Lee Jones]

On Wed, 20 Jun 2018, Dmitry Torokhov wrote:

> This reverts commit 556c242045f0c1613aac2e64dc5b2ff0e4bc89e1.
> 
> The patch that this change is purported to fix is broken and should be
> reverted; thus we reverting this one as well.

You need to provide more information.

How does the original patch break the build/code execution?
What is this patch doing to rectify the issue?
Any other information you think is relevant.

> ---
> 
> Lee,
> 
> I see the broken code is already in Linus' tree, so please merge send
> it on at your earliest convenience.
> 
> Thanks,
> Dmitry
> 
> 
>  drivers/mfd/cros_ec_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index 306e1fd109bd..4b1dbe81fcb6 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -262,6 +262,8 @@ static const struct file_operations fops = {
>  #endif
>  };
>  
> +static void __remove(struct device *dev) { }
> +
>  static void cros_ec_sensors_register(struct cros_ec_dev *ec)
>  {
>  	/*

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] Revert "mfd: cros_ec: Use devm_kzalloc for private data"

[Lee Jones]

On Wed, 20 Jun 2018, Dmitry Torokhov wrote:

> This reverts commit 3aa2177e47878f7e7616da8a2050c44f22301b6e.
> 
> Linux device objects are refcounted and thus should not be allocated
> with devm API as they may outlive the timeframe of another device being
> bound to a driver.
> ---
>  drivers/mfd/cros_ec_dev.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] Revert "mfd: cros_ec: Remove unused __remove function"

[Dmitry Torokhov]

Hi Lee,

On Wed, Jul 04, 2018 at 07:48:11AM +0100, Lee Jones wrote:
> On Wed, 20 Jun 2018, Dmitry Torokhov wrote:
> 
> > This reverts commit 556c242045f0c1613aac2e64dc5b2ff0e4bc89e1.
> > 
> > The patch that this change is purported to fix is broken and should be
> > reverted; thus we reverting this one as well.
> 
> You need to provide more information.
> 
> How does the original patch break the build/code execution?
> What is this patch doing to rectify the issue?
> Any other information you think is relevant.

We are reverting this patch because it is not needed: it tries to fix
another patch that is completely broken and needs to be reverted. The
justification for reverting the original broken patch is in the revert
of that patch, but just for reference:

We should not use devm_kzalloc() to allocate refcounted objects.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Revert "mfd: cros_ec: Remove unused __remove function"

[Lee Jones]

On Wed, 04 Jul 2018, Dmitry Torokhov wrote:
> On Wed, Jul 04, 2018 at 07:48:11AM +0100, Lee Jones wrote:
> > On Wed, 20 Jun 2018, Dmitry Torokhov wrote:
> > 
> > > This reverts commit 556c242045f0c1613aac2e64dc5b2ff0e4bc89e1.
> > > 
> > > The patch that this change is purported to fix is broken and should be
> > > reverted; thus we reverting this one as well.
> > 
> > You need to provide more information.
> > 
> > How does the original patch break the build/code execution?
> > What is this patch doing to rectify the issue?
> > Any other information you think is relevant.
> 
> We are reverting this patch because it is not needed: it tries to fix
> another patch that is completely broken and needs to be reverted. The
> justification for reverting the original broken patch is in the revert
> of that patch, but just for reference:
> 
> We should not use devm_kzalloc() to allocate refcounted objects.

The other patch looked good.  Please ensure the story is told in each
of the patches individually though.  That way they can be are viewed
by an outsider the history can be read and understood independently.

Thanks in advance.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog