rpmsg: Release rpmsg devices in backends

Message ID 20170316051835.10008-1-bjorn.andersson@linaro.org
State Accepted
Commit b0b03b8119633de0649da9bd506e4850c401ff2b
Headers show

Commit Message

Bjorn Andersson March 16, 2017, 5:18 a.m.
The rpmsg devices are allocated in the backends and as such must be
freed there as well.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

---
 drivers/rpmsg/qcom_smd.c         | 11 +++++++++++
 drivers/rpmsg/virtio_rpmsg_bus.c |  9 +++++++++
 2 files changed, 20 insertions(+)

-- 
2.12.0

Comments

Henri Roosen June 2, 2017, 10:07 a.m. | #1
> The rpmsg devices are allocated in the backends and as such must be

> freed there as well.

>

> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---

>  drivers/rpmsg/qcom_smd.c         | 11 +++++++++++

>  drivers/rpmsg/virtio_rpmsg_bus.c |  9 +++++++++

>  2 files changed, 20 insertions(+)

>

> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c

> index beaef5dd973e..a0a39a8821a3 100644

> --- a/drivers/rpmsg/qcom_smd.c

> +++ b/drivers/rpmsg/qcom_smd.c

> @@ -969,6 +969,14 @@ static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {

>  	.poll = qcom_smd_poll,

>  };

>

> +static void qcom_smd_release_device(struct device *dev)

> +{

> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);

> +	struct qcom_smd_device *qsdev = to_smd_device(rpdev);

> +

> +	kfree(qsdev);

> +}

> +

>  /*

>   * Create a smd client device for channel that is being opened.

>   */

> @@ -998,6 +1006,7 @@ static int qcom_smd_create_device(struct qcom_smd_channel *channel)

>

>  	rpdev->dev.of_node = qcom_smd_match_channel(edge->of_node, channel->name);

>  	rpdev->dev.parent = &edge->dev;

> +	rpdev->dev.release = qcom_smd_release_device;

>

>  	return rpmsg_register_device(rpdev);


This will not work: the registration of qcom_smd_release_device at 
rpdev->dev.release gets overwritten inside rpmsg_register_device() and 
will then point to the function rpmsg_release_device().

My suggestion would be to additionally change/fix 
rpmsg_register_device() so it will not overwrite the release callback.

>  }

> @@ -1013,6 +1022,8 @@ static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)

>  	qsdev->edge = edge;

>  	qsdev->rpdev.ops = &qcom_smd_device_ops;

>  	qsdev->rpdev.dev.parent = &edge->dev;

> +	qsdev->rpdev.dev.release = qcom_smd_release_device;

> +

>  	return rpmsg_chrdev_register_device(&qsdev->rpdev);


This will not work either: same reason as described above because 
rpmsg_chrdev_register_device() will call rpmsg_register_device().

>  }

>

> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c

> index 5e66e081027e..7f8c5cc1c118 100644

> --- a/drivers/rpmsg/virtio_rpmsg_bus.c

> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c

> @@ -360,6 +360,14 @@ static const struct rpmsg_device_ops virtio_rpmsg_ops = {

>  	.announce_destroy = virtio_rpmsg_announce_destroy,

>  };

>

> +static void virtio_rpmsg_release_device(struct device *dev)

> +{

> +	struct rpmsg_device *rpdev = to_rpmsg_device(dev);

> +	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);

> +

> +	kfree(vch);

> +}

> +

>  /*

>   * create an rpmsg channel using its name and address info.

>   * this function will be used to create both static and dynamic

> @@ -408,6 +416,7 @@ static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,

>  	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);

>

>  	rpdev->dev.parent = &vrp->vdev->dev;

> +	rpdev->dev.release = virtio_rpmsg_release_device;

>  	ret = rpmsg_register_device(rpdev);


The same issue as described above.

>  	if (ret)

>  		return NULL;
Suman Anna June 3, 2017, 12:28 a.m. | #2
Hi Bjorn,

On 06/02/2017 05:07 AM, Henri Roosen wrote:
>> The rpmsg devices are allocated in the backends and as such must be

>> freed there as well.

>>

>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

>> ---

>>  drivers/rpmsg/qcom_smd.c         | 11 +++++++++++

>>  drivers/rpmsg/virtio_rpmsg_bus.c |  9 +++++++++

>>  2 files changed, 20 insertions(+)

>>

>> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c

>> index beaef5dd973e..a0a39a8821a3 100644

>> --- a/drivers/rpmsg/qcom_smd.c

>> +++ b/drivers/rpmsg/qcom_smd.c

>> @@ -969,6 +969,14 @@ static const struct rpmsg_endpoint_ops

>> qcom_smd_endpoint_ops = {

>>      .poll = qcom_smd_poll,

>>  };

>>

>> +static void qcom_smd_release_device(struct device *dev)

>> +{

>> +    struct rpmsg_device *rpdev = to_rpmsg_device(dev);

>> +    struct qcom_smd_device *qsdev = to_smd_device(rpdev);

>> +

>> +    kfree(qsdev);

>> +}

>> +

>>  /*

>>   * Create a smd client device for channel that is being opened.

>>   */

>> @@ -998,6 +1006,7 @@ static int qcom_smd_create_device(struct

>> qcom_smd_channel *channel)

>>

>>      rpdev->dev.of_node = qcom_smd_match_channel(edge->of_node,

>> channel->name);

>>      rpdev->dev.parent = &edge->dev;

>> +    rpdev->dev.release = qcom_smd_release_device;

>>

>>      return rpmsg_register_device(rpdev);

> 

> This will not work: the registration of qcom_smd_release_device at

> rpdev->dev.release gets overwritten inside rpmsg_register_device() and

> will then point to the function rpmsg_release_device().

> 

> My suggestion would be to additionally change/fix

> rpmsg_register_device() so it will not overwrite the release callback.

> 

>>  }

>> @@ -1013,6 +1022,8 @@ static int qcom_smd_create_chrdev(struct

>> qcom_smd_edge *edge)

>>      qsdev->edge = edge;

>>      qsdev->rpdev.ops = &qcom_smd_device_ops;

>>      qsdev->rpdev.dev.parent = &edge->dev;

>> +    qsdev->rpdev.dev.release = qcom_smd_release_device;

>> +

>>      return rpmsg_chrdev_register_device(&qsdev->rpdev);

> 

> This will not work either: same reason as described above because

> rpmsg_chrdev_register_device() will call rpmsg_register_device().

> 

>>  }

>>

>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c

>> b/drivers/rpmsg/virtio_rpmsg_bus.c

>> index 5e66e081027e..7f8c5cc1c118 100644

>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c

>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c

>> @@ -360,6 +360,14 @@ static const struct rpmsg_device_ops

>> virtio_rpmsg_ops = {

>>      .announce_destroy = virtio_rpmsg_announce_destroy,

>>  };

>>

>> +static void virtio_rpmsg_release_device(struct device *dev)

>> +{

>> +    struct rpmsg_device *rpdev = to_rpmsg_device(dev);

>> +    struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);

>> +

>> +    kfree(vch);

>> +}

>> +

>>  /*

>>   * create an rpmsg channel using its name and address info.

>>   * this function will be used to create both static and dynamic

>> @@ -408,6 +416,7 @@ static struct rpmsg_device

>> *rpmsg_create_channel(struct virtproc_info *vrp,

>>      strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);

>>

>>      rpdev->dev.parent = &vrp->vdev->dev;

>> +    rpdev->dev.release = virtio_rpmsg_release_device;

>>      ret = rpmsg_register_device(rpdev);

> 

> The same issue as described above.


FWIW, I didn't run into any rpmsg device memory leaks even without this
patch with booting and shutting down of remoteproc devices. The
virtio_rpmsg_channel structure inherits the struct rpmsg_device and is
the one that gets allocated, and the release function plugged in
rpmsg_release_device is operating on the rpmsg_device pointer, but both
are actually the same pointer.

Did you run into any memory leaks that required you to have this patch?

regards
Suman
Bjorn Andersson June 25, 2017, 9:28 p.m. | #3
On Fri 02 Jun 17:28 PDT 2017, Suman Anna wrote:

> Hi Bjorn,

> 

> On 06/02/2017 05:07 AM, Henri Roosen wrote:

> > My suggestion would be to additionally change/fix

> > rpmsg_register_device() so it will not overwrite the release callback.

[..]
> FWIW, I didn't run into any rpmsg device memory leaks even without this

> patch with booting and shutting down of remoteproc devices. The

> virtio_rpmsg_channel structure inherits the struct rpmsg_device and is

> the one that gets allocated, and the release function plugged in

> rpmsg_release_device is operating on the rpmsg_device pointer, but both

> are actually the same pointer.

> 

> Did you run into any memory leaks that required you to have this patch?

> 


I did not see any memory leaks, because it happens that the pointers are
the same in all current cases.

But the code is wrong and should be fixed, thanks for pointing this out
Henri!

Regards,
Bjorn

Patch hide | download patch | download mbox

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index beaef5dd973e..a0a39a8821a3 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -969,6 +969,14 @@  static const struct rpmsg_endpoint_ops qcom_smd_endpoint_ops = {
 	.poll = qcom_smd_poll,
 };
 
+static void qcom_smd_release_device(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct qcom_smd_device *qsdev = to_smd_device(rpdev);
+
+	kfree(qsdev);
+}
+
 /*
  * Create a smd client device for channel that is being opened.
  */
@@ -998,6 +1006,7 @@  static int qcom_smd_create_device(struct qcom_smd_channel *channel)
 
 	rpdev->dev.of_node = qcom_smd_match_channel(edge->of_node, channel->name);
 	rpdev->dev.parent = &edge->dev;
+	rpdev->dev.release = qcom_smd_release_device;
 
 	return rpmsg_register_device(rpdev);
 }
@@ -1013,6 +1022,8 @@  static int qcom_smd_create_chrdev(struct qcom_smd_edge *edge)
 	qsdev->edge = edge;
 	qsdev->rpdev.ops = &qcom_smd_device_ops;
 	qsdev->rpdev.dev.parent = &edge->dev;
+	qsdev->rpdev.dev.release = qcom_smd_release_device;
+
 	return rpmsg_chrdev_register_device(&qsdev->rpdev);
 }
 
diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 5e66e081027e..7f8c5cc1c118 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -360,6 +360,14 @@  static const struct rpmsg_device_ops virtio_rpmsg_ops = {
 	.announce_destroy = virtio_rpmsg_announce_destroy,
 };
 
+static void virtio_rpmsg_release_device(struct device *dev)
+{
+	struct rpmsg_device *rpdev = to_rpmsg_device(dev);
+	struct virtio_rpmsg_channel *vch = to_virtio_rpmsg_channel(rpdev);
+
+	kfree(vch);
+}
+
 /*
  * create an rpmsg channel using its name and address info.
  * this function will be used to create both static and dynamic
@@ -408,6 +416,7 @@  static struct rpmsg_device *rpmsg_create_channel(struct virtproc_info *vrp,
 	strncpy(rpdev->id.name, chinfo->name, RPMSG_NAME_SIZE);
 
 	rpdev->dev.parent = &vrp->vdev->dev;
+	rpdev->dev.release = virtio_rpmsg_release_device;
 	ret = rpmsg_register_device(rpdev);
 	if (ret)
 		return NULL;