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

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

[Stuart Yoder]

> -----Original Message-----
> From: upstream-release-bounces@linux.freescale.net [mailto:upstream-release-
> bounces@linux.freescale.net] On Behalf Of laurentiu.tudor@nxp.com
> Sent: Wednesday, February 01, 2017 5:43 AM
> To: gregkh@linuxfoundation.org
> Cc: devel@driverdev.osuosl.org; arnd@arndb.de; Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> Roy Pledge <roy.pledge@nxp.com>; linux-kernel@vger.kernel.org; agraf@suse.de; Catalin Horghidan
> <catalin.horghidan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release callback
> 
> From: Laurentiu Tudor <laurentiu.tudor@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.tudor@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(&mc_dev->dev);
>  	mc_dev->dev.parent = parent_dev;
>  	mc_dev->dev.bus = &fsl_mc_bus_type;
> +	mc_dev->dev.release = fsl_mc_device_release;
>  	dev_set_name(&mc_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(&mc_dev->dev);
        put_device(&mc_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

[Laurentiu Tudor]

On 02/03/2017 02:02 AM, Stuart Yoder wrote:
>
>> -----Original Message-----
>> From: upstream-release-bounces@linux.freescale.net [mailto:upstream-release-
>> bounces@linux.freescale.net] On Behalf Of laurentiu.tudor@nxp.com
>> Sent: Wednesday, February 01, 2017 5:43 AM
>> To: gregkh@linuxfoundation.org
>> Cc: devel@driverdev.osuosl.org; arnd@arndb.de; Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
>> Roy Pledge <roy.pledge@nxp.com>; linux-kernel@vger.kernel.org; agraf@suse.de; Catalin Horghidan
>> <catalin.horghidan@nxp.com>; Leo Li <leoyang.li@nxp.com>; Stuart Yoder <stuart.yoder@nxp.com>;
>> Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> Subject: [upstream-release] [PATCH 3/9] staging: fsl-mc: add device release callback
>>
>> From: Laurentiu Tudor <laurentiu.tudor@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.tudor@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(&mc_dev->dev);
>>   	mc_dev->dev.parent = parent_dev;
>>   	mc_dev->dev.bus = &fsl_mc_bus_type;
>> +	mc_dev->dev.release = fsl_mc_device_release;
>>   	dev_set_name(&mc_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(&mc_dev->dev);
>          put_device(&mc_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

[PATCH 0/9] staging: fsl-mc: fixes and cleanups

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

First 4 patches fix several driver model related
issues and drop an useless atomic global.
The rest of the patches are cleanups mostly
consisting in removing dead code.

For context, see these threads:
https://lkml.org/lkml/2016/12/7/382
https://lkml.org/lkml/2017/1/3/437

Laurentiu Tudor (9):
  staging: fsl-mc: drop root dprc counting
  staging: fsl-mc: fix device ref counting
  staging: fsl-mc: add device release callback
  staging: fsl-mc: don't use devres api for refcounted objects
  staging: fsl-mc: dpmcp: drop unused APIs
  staging: fsl-mc: dpmng: drop unused prototype
  staging: fsl-mc: dpbp: drop unused APIs
  staging: fsl-mc: dpbp: add a few missing EXPORT_SYMBOL()s
  staging: fsl-mc: dprc: drop unused APIs

 drivers/staging/fsl-mc/bus/dpbp-cmd.h    | 116 ------
 drivers/staging/fsl-mc/bus/dpbp.c        | 452 +--------------------
 drivers/staging/fsl-mc/bus/dpmcp-cmd.h   |  95 -----
 drivers/staging/fsl-mc/bus/dpmcp.c       | 382 ------------------
 drivers/staging/fsl-mc/bus/dpmcp.h       | 100 +----
 drivers/staging/fsl-mc/bus/dprc-cmd.h    |  18 -
 drivers/staging/fsl-mc/bus/dprc-driver.c |   1 +
 drivers/staging/fsl-mc/bus/dprc.c        | 666 -------------------------------
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c  |  71 +---
 drivers/staging/fsl-mc/include/dpbp.h    | 129 ------
 drivers/staging/fsl-mc/include/dpmng.h   |   4 -
 drivers/staging/fsl-mc/include/dprc.h    | 244 +----------
 12 files changed, 25 insertions(+), 2253 deletions(-)

-- 
1.8.3.1

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

[laurentiu.tudor]

From: Laurentiu Tudor <laurentiu.tudor@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.tudor@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(&mc_dev->dev);
 	mc_dev->dev.parent = parent_dev;
 	mc_dev->dev.bus = &fsl_mc_bus_type;
+	mc_dev->dev.release = fsl_mc_device_release;
 	dev_set_name(&mc_dev->dev, "%s.%d", obj_desc->type, obj_desc->id);
 
 	if (strcmp(obj_desc->type, "dprc") == 0) {
-- 
1.8.3.1