Re: [PATCH 3/9] staging: fsl-mc: add device release callback

[Laurentiu Tudor]

On 02/03/2017 02:02 AM, Stuart Yoder wrote:
>
>> -Original Message-
>> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-release-
>> boun...@linux.freescale.net] On Behalf Of laurentiu.tu...@nxp.com
>> Sent: Wednesday, February 01, 2017 5:43 AM
>> To: gre...@linuxfoundation.org
>> Cc: de...@driverdev.osuosl.org; a...@arndb.de; Ruxandra Ioana Radulescu 
>> <ruxandra.radule...@nxp.com>;
>> Roy Pledge <roy.ple...@nxp.com>; linux-kernel@vger.kernel.org; 
>> ag...@suse.de; Catalin Horghidan
>> <catalin.horghi...@nxp.com>; Leo Li <leoyang...@nxp.com>; Stuart Yoder 
>> <stuart.yo...@nxp.com>;
>> Laurentiu Tudor <laurentiu.tu...@nxp.com>
>> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release 
>> callback
>>
>> From: Laurentiu Tudor <laurentiu.tu...@nxp.com>
>>
>> When hot unplugging a mc-bus device the kernel displays
>> this pertinent message, followed by a stack dump:
>>  "Device 'foo.N' does not have a release() function,
>>   it is broken and must be fixed."
>> Add the required callback to fix.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
>> ---
>>   drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
>> b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> index 7c6a43b..6601bde 100644
>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> @@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
>>  return dev == root_dprc_dev;
>>   }
>>
>> +static void fsl_mc_device_release(struct device *dev)
>> +{
>> +struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
>> +struct fsl_mc_bus *mc_bus = NULL;
>> +
>> +kfree(mc_dev->regions);
>> +
>> +if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
>> +mc_bus = to_fsl_mc_bus(mc_dev);
>> +
>> +if (mc_bus)
>> +devm_kfree(mc_dev->dev.parent, mc_bus);
>> +else
>> +kmem_cache_free(mc_dev_cache, mc_dev);
>> +}
>> +
>>   /**
>>* Add a newly discovered fsl-mc device to be visible in Linux
>>*/
>> @@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>  device_initialize(_dev->dev);
>>  mc_dev->dev.parent = parent_dev;
>>  mc_dev->dev.bus = _mc_bus_type;
>> +mc_dev->dev.release = fsl_mc_device_release;
>>  dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
>>
>>  if (strcmp(obj_desc->type, "dprc") == 0) {
>> --
>
> With this patch applied, you still have this:
>
> void fsl_mc_device_remove(struct fsl_mc_device *mc_dev)
> {
>  struct fsl_mc_bus *mc_bus = NULL;
>
>  kfree(mc_dev->regions);
>
>  /*
>   * The device-specific remove callback will get invoked by 
> device_del()
>   */
>  device_del(_dev->dev);
>  put_device(_dev->dev);
>
>  if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
>  mc_bus = to_fsl_mc_bus(mc_dev);
>
>  if (mc_bus)
>  devm_kfree(mc_dev->dev.parent, mc_bus);
>  else
>  kmem_cache_free(mc_dev_cache, mc_dev);
> }
>
> ...i.e. you are doing the same thing in 2 places.  You
> need to remove the kfree/devm_kfree/ kmem_cache_free,
> here, no?
>

Right, thanks for spotting. I started working on a v2
of the series.

---
Best Regards, Laurentiu

Re: [PATCH 3/9] staging: fsl-mc: add device release callback

[Laurentiu Tudor]

On 02/03/2017 02:02 AM, Stuart Yoder wrote:
>
>> -Original Message-
>> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-release-
>> boun...@linux.freescale.net] On Behalf Of laurentiu.tu...@nxp.com
>> Sent: Wednesday, February 01, 2017 5:43 AM
>> To: gre...@linuxfoundation.org
>> Cc: de...@driverdev.osuosl.org; a...@arndb.de; Ruxandra Ioana Radulescu 
>> ;
>> Roy Pledge ; linux-kernel@vger.kernel.org; 
>> ag...@suse.de; Catalin Horghidan
>> ; Leo Li ; Stuart Yoder 
>> ;
>> Laurentiu Tudor 
>> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release 
>> callback
>>
>> From: Laurentiu Tudor 
>>
>> When hot unplugging a mc-bus device the kernel displays
>> this pertinent message, followed by a stack dump:
>>  "Device 'foo.N' does not have a release() function,
>>   it is broken and must be fixed."
>> Add the required callback to fix.
>>
>> Signed-off-by: Laurentiu Tudor 
>> ---
>>   drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
>> b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> index 7c6a43b..6601bde 100644
>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> @@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
>>  return dev == root_dprc_dev;
>>   }
>>
>> +static void fsl_mc_device_release(struct device *dev)
>> +{
>> +struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
>> +struct fsl_mc_bus *mc_bus = NULL;
>> +
>> +kfree(mc_dev->regions);
>> +
>> +if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
>> +mc_bus = to_fsl_mc_bus(mc_dev);
>> +
>> +if (mc_bus)
>> +devm_kfree(mc_dev->dev.parent, mc_bus);
>> +else
>> +kmem_cache_free(mc_dev_cache, mc_dev);
>> +}
>> +
>>   /**
>>* Add a newly discovered fsl-mc device to be visible in Linux
>>*/
>> @@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>  device_initialize(_dev->dev);
>>  mc_dev->dev.parent = parent_dev;
>>  mc_dev->dev.bus = _mc_bus_type;
>> +mc_dev->dev.release = fsl_mc_device_release;
>>  dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
>>
>>  if (strcmp(obj_desc->type, "dprc") == 0) {
>> --
>
> With this patch applied, you still have this:
>
> void fsl_mc_device_remove(struct fsl_mc_device *mc_dev)
> {
>  struct fsl_mc_bus *mc_bus = NULL;
>
>  kfree(mc_dev->regions);
>
>  /*
>   * The device-specific remove callback will get invoked by 
> device_del()
>   */
>  device_del(_dev->dev);
>  put_device(_dev->dev);
>
>  if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
>  mc_bus = to_fsl_mc_bus(mc_dev);
>
>  if (mc_bus)
>  devm_kfree(mc_dev->dev.parent, mc_bus);
>  else
>  kmem_cache_free(mc_dev_cache, mc_dev);
> }
>
> ...i.e. you are doing the same thing in 2 places.  You
> need to remove the kfree/devm_kfree/ kmem_cache_free,
> here, no?
>

Right, thanks for spotting. I started working on a v2
of the series.

---
Best Regards, Laurentiu

RE: [PATCH 3/9] staging: fsl-mc: add device release callback

[Stuart Yoder]

> -Original Message-
> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-release-
> boun...@linux.freescale.net] On Behalf Of laurentiu.tu...@nxp.com
> Sent: Wednesday, February 01, 2017 5:43 AM
> To: gre...@linuxfoundation.org
> Cc: de...@driverdev.osuosl.org; a...@arndb.de; Ruxandra Ioana Radulescu 
> <ruxandra.radule...@nxp.com>;
> Roy Pledge <roy.ple...@nxp.com>; linux-kernel@vger.kernel.org; ag...@suse.de; 
> Catalin Horghidan
> <catalin.horghi...@nxp.com>; Leo Li <leoyang...@nxp.com>; Stuart Yoder 
> <stuart.yo...@nxp.com>;
> Laurentiu Tudor <laurentiu.tu...@nxp.com>
> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release 
> callback
> 
> From: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> 
> When hot unplugging a mc-bus device the kernel displays
> this pertinent message, followed by a stack dump:
> "Device 'foo.N' does not have a release() function,
>  it is broken and must be fixed."
> Add the required callback to fix.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tu...@nxp.com>
> ---
>  drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
> b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> index 7c6a43b..6601bde 100644
> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> @@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
>   return dev == root_dprc_dev;
>  }
> 
> +static void fsl_mc_device_release(struct device *dev)
> +{
> + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> + struct fsl_mc_bus *mc_bus = NULL;
> +
> + kfree(mc_dev->regions);
> +
> + if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
> + mc_bus = to_fsl_mc_bus(mc_dev);
> +
> + if (mc_bus)
> + devm_kfree(mc_dev->dev.parent, mc_bus);
> + else
> + kmem_cache_free(mc_dev_cache, mc_dev);
> +}
> +
>  /**
>   * Add a newly discovered fsl-mc device to be visible in Linux
>   */
> @@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>   device_initialize(_dev->dev);
>   mc_dev->dev.parent = parent_dev;
>   mc_dev->dev.bus = _mc_bus_type;
> + mc_dev->dev.release = fsl_mc_device_release;
>   dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
> 
>   if (strcmp(obj_desc->type, "dprc") == 0) {
> --

With this patch applied, you still have this:

void fsl_mc_device_remove(struct fsl_mc_device *mc_dev)
{
struct fsl_mc_bus *mc_bus = NULL;

kfree(mc_dev->regions);

/*
 * The device-specific remove callback will get invoked by device_del()
 */
device_del(_dev->dev);
put_device(_dev->dev);

if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
mc_bus = to_fsl_mc_bus(mc_dev);

if (mc_bus)
devm_kfree(mc_dev->dev.parent, mc_bus);
else
kmem_cache_free(mc_dev_cache, mc_dev);
}

...i.e. you are doing the same thing in 2 places.  You
need to remove the kfree/devm_kfree/ kmem_cache_free,
here, no?

Stuart

RE: [PATCH 3/9] staging: fsl-mc: add device release callback

[Stuart Yoder]

> -Original Message-
> From: upstream-release-boun...@linux.freescale.net [mailto:upstream-release-
> boun...@linux.freescale.net] On Behalf Of laurentiu.tu...@nxp.com
> Sent: Wednesday, February 01, 2017 5:43 AM
> To: gre...@linuxfoundation.org
> Cc: de...@driverdev.osuosl.org; a...@arndb.de; Ruxandra Ioana Radulescu 
> ;
> Roy Pledge ; linux-kernel@vger.kernel.org; ag...@suse.de; 
> Catalin Horghidan
> ; Leo Li ; Stuart Yoder 
> ;
> Laurentiu Tudor 
> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release 
> callback
> 
> From: Laurentiu Tudor 
> 
> When hot unplugging a mc-bus device the kernel displays
> this pertinent message, followed by a stack dump:
> "Device 'foo.N' does not have a release() function,
>  it is broken and must be fixed."
> Add the required callback to fix.
> 
> Signed-off-by: Laurentiu Tudor 
> ---
>  drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
> b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> index 7c6a43b..6601bde 100644
> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> @@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
>   return dev == root_dprc_dev;
>  }
> 
> +static void fsl_mc_device_release(struct device *dev)
> +{
> + struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
> + struct fsl_mc_bus *mc_bus = NULL;
> +
> + kfree(mc_dev->regions);
> +
> + if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
> + mc_bus = to_fsl_mc_bus(mc_dev);
> +
> + if (mc_bus)
> + devm_kfree(mc_dev->dev.parent, mc_bus);
> + else
> + kmem_cache_free(mc_dev_cache, mc_dev);
> +}
> +
>  /**
>   * Add a newly discovered fsl-mc device to be visible in Linux
>   */
> @@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>   device_initialize(_dev->dev);
>   mc_dev->dev.parent = parent_dev;
>   mc_dev->dev.bus = _mc_bus_type;
> + mc_dev->dev.release = fsl_mc_device_release;
>   dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
> 
>   if (strcmp(obj_desc->type, "dprc") == 0) {
> --

With this patch applied, you still have this:

void fsl_mc_device_remove(struct fsl_mc_device *mc_dev)
{
struct fsl_mc_bus *mc_bus = NULL;

kfree(mc_dev->regions);

/*
 * The device-specific remove callback will get invoked by device_del()
 */
device_del(_dev->dev);
put_device(_dev->dev);

if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
mc_bus = to_fsl_mc_bus(mc_dev);

if (mc_bus)
devm_kfree(mc_dev->dev.parent, mc_bus);
else
kmem_cache_free(mc_dev_cache, mc_dev);
}

...i.e. you are doing the same thing in 2 places.  You
need to remove the kfree/devm_kfree/ kmem_cache_free,
here, no?

Stuart

[PATCH 3/9] staging: fsl-mc: add device release callback

[laurentiu.tudor]

From: Laurentiu Tudor 

When hot unplugging a mc-bus device the kernel displays
this pertinent message, followed by a stack dump:
"Device 'foo.N' does not have a release() function,
 it is broken and must be fixed."
Add the required callback to fix.

Signed-off-by: Laurentiu Tudor 
---
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 7c6a43b..6601bde 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
return dev == root_dprc_dev;
 }
 
+static void fsl_mc_device_release(struct device *dev)
+{
+   struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+   struct fsl_mc_bus *mc_bus = NULL;
+
+   kfree(mc_dev->regions);
+
+   if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
+   mc_bus = to_fsl_mc_bus(mc_dev);
+
+   if (mc_bus)
+   devm_kfree(mc_dev->dev.parent, mc_bus);
+   else
+   kmem_cache_free(mc_dev_cache, mc_dev);
+}
+
 /**
  * Add a newly discovered fsl-mc device to be visible in Linux
  */
@@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
device_initialize(_dev->dev);
mc_dev->dev.parent = parent_dev;
mc_dev->dev.bus = _mc_bus_type;
+   mc_dev->dev.release = fsl_mc_device_release;
dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
 
if (strcmp(obj_desc->type, "dprc") == 0) {
-- 
1.8.3.1

[PATCH 3/9] staging: fsl-mc: add device release callback

[laurentiu.tudor]

From: Laurentiu Tudor 

When hot unplugging a mc-bus device the kernel displays
this pertinent message, followed by a stack dump:
"Device 'foo.N' does not have a release() function,
 it is broken and must be fixed."
Add the required callback to fix.

Signed-off-by: Laurentiu Tudor 
---
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c 
b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 7c6a43b..6601bde 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -419,6 +419,22 @@ bool fsl_mc_is_root_dprc(struct device *dev)
return dev == root_dprc_dev;
 }
 
+static void fsl_mc_device_release(struct device *dev)
+{
+   struct fsl_mc_device *mc_dev = to_fsl_mc_device(dev);
+   struct fsl_mc_bus *mc_bus = NULL;
+
+   kfree(mc_dev->regions);
+
+   if (strcmp(mc_dev->obj_desc.type, "dprc") == 0)
+   mc_bus = to_fsl_mc_bus(mc_dev);
+
+   if (mc_bus)
+   devm_kfree(mc_dev->dev.parent, mc_bus);
+   else
+   kmem_cache_free(mc_dev_cache, mc_dev);
+}
+
 /**
  * Add a newly discovered fsl-mc device to be visible in Linux
  */
@@ -460,6 +476,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
device_initialize(_dev->dev);
mc_dev->dev.parent = parent_dev;
mc_dev->dev.bus = _mc_bus_type;
+   mc_dev->dev.release = fsl_mc_device_release;
dev_set_name(_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
 
if (strcmp(obj_desc->type, "dprc") == 0) {
-- 
1.8.3.1