[PATCH v4 0/4] use put_device to cleanup resource

[PATCH v4 0/4] use put_device to cleanup resource

[weiping zhang]

Hi,

The main change is split device_register into 2 sperate calls:
device_initalize() and device_add, and then the caller can use
put_device safety when fail to register_virtio_device.

v3->v4:
 * split device_register into device_initialize and devicea_add that
   the caller can always use put_device when fail to register virtio
   device.

v2->v3:
 * virtio: add new helper do get device's status then determine use
   put_device or kfree.

v1->v2:
 * virtio_pci: add comments in commit message for why using put_device
 * virtio_vop: also use put_device int _vop_remove_device
weiping zhang (4):
  virtio: split device_register into device_initialize and device_add
  virtio_pci: don't kfree device on register failure
  virtio_vop: don't kfree device on register failure
  virtio_remoteproc: don't kfree device on register failure

 drivers/misc/mic/vop/vop_main.c        | 20 +++++++++++++-------
 drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
 drivers/virtio/virtio.c                | 18 +++++++++++++++---
 drivers/virtio/virtio_pci_common.c     |  8 ++++++--
 4 files changed, 45 insertions(+), 14 deletions(-)

-- 
2.9.4

[PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

[weiping zhang]

In order to make caller do a simple cleanup, we split device_register
into device_initialize and device_add. device_initialize always sucess,
the caller can always use put_device when fail to register virtio_device
no matter fail at ida_simple_get or at device_add.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
Suggested-by: Cornelia Huck <cohuck@redhat.com>
---
 drivers/virtio/virtio.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index bf7ff39..3c9f211 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver)
 }
 EXPORT_SYMBOL_GPL(unregister_virtio_driver);
 
+/**
+ * register_virtio_device - register virtio device
+ * @dev        : virtio device interested
+ *
+ * If an error occurs, the caller must use put_device, instead of kfree, because
+ * device_initialize and device_add will increase @dev->dev's reference count.
+ *
+ * Returns: 0 on suceess, -error on failure
+ */
 int register_virtio_device(struct virtio_device *dev)
 {
 	int err;
 
 	dev->dev.bus = &virtio_bus;
+	device_initialize(&dev->dev);
 
 	/* Assign a unique device index and hence name. */
 	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
@@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
 
 	INIT_LIST_HEAD(&dev->vqs);
 
-	/* device_register() causes the bus infrastructure to look for a
-	 * matching driver. */
-	err = device_register(&dev->dev);
+	/*
+	 * device_add() causes the bus infrastructure to look for a matching
+	 * driver.
+	 */
+	err = device_add(&dev->dev);
 	if (err)
 		ida_simple_remove(&virtio_index_ida, dev->index);
 out:
-- 
2.9.4

[PATCH v4 2/4] virtio_pci: don't kfree device on register failure

[weiping zhang]

As mentioned at drivers/base/core.c:
/*
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
so we don't free vp_dev until vp_dev->vdev.dev.release be called.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/virtio/virtio_pci_common.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..48d4d1c 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
 static int virtio_pci_probe(struct pci_dev *pci_dev,
 			    const struct pci_device_id *id)
 {
-	struct virtio_pci_device *vp_dev;
+	struct virtio_pci_device *vp_dev, *reg_dev = NULL;
 	int rc;
 
 	/* allocate our structure and fill it out */
@@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
+	reg_dev = vp_dev;
 	if (rc)
 		goto err_register;
 
@@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 err_probe:
 	pci_disable_device(pci_dev);
 err_enable_device:
-	kfree(vp_dev);
+	if (reg_dev)
+		put_device(&vp_dev->vdev.dev);
+	else
+		kfree(vp_dev);
 	return rc;
 }
 
-- 
2.9.4

[PATCH v4 3/4] virtio_vop: don't kfree device on register failure

[weiping zhang]

As mentioned at drivers/base/core.c:
/*
 * NOTE: _Never_ directly free @dev after calling this function, even
 * if it returned an error! Always use put_device() to give up the
 * reference initialized in this function instead.
 */
so we don't free vdev until vdev->vdev.dev.release be called.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..3633202 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
 
 static void vop_virtio_release_dev(struct device *_d)
 {
-	/*
-	 * No need for a release method similar to virtio PCI.
-	 * Provide an empty one to avoid getting a warning from core.
-	 */
+	struct virtio_device *vdev =
+			container_of(_d, struct virtio_device, dev);
+	struct _vop_vdev *vop_vdev =
+			container_of(vdev, struct _vop_vdev, vdev);
+
+	kfree(vop_vdev);
 }
 
 /*
@@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
 			   unsigned int offset, struct vop_device *vpdev,
 			   int dnode)
 {
-	struct _vop_vdev *vdev;
+	struct _vop_vdev *vdev, *reg_dev = NULL;
 	int ret;
 	u8 type = ioread8(&d->type);
 
@@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
 	vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
 
 	ret = register_virtio_device(&vdev->vdev);
+	reg_dev = vdev;
 	if (ret) {
 		dev_err(_vop_dev(vdev),
 			"Failed to register vop device %u type %u\n",
@@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
 free_irq:
 	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
 kfree:
-	kfree(vdev);
+	if (reg_dev)
+		put_device(&vdev->vdev.dev);
+	else
+		kfree(vdev);
 	return ret;
 }
 
@@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d,
 		iowrite8(-1, &dc->h2c_vdev_db);
 		if (status & VIRTIO_CONFIG_S_DRIVER_OK)
 			wait_for_completion(&vdev->reset_done);
-		kfree(vdev);
+		put_device(&vdev->vdev.dev);
 		iowrite8(1, &dc->guest_ack);
 		dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
 			__func__, __LINE__, ioread8(&dc->guest_ack));
-- 
2.9.4

[PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure

[weiping zhang]

rproc_virtio_dev_release will be called iff virtio_device.dev's
refer count became to 0. Here we should check if we call device_register
or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
done in rproc_virtio_dev_release, otherwise we do cleanup directly.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..1073ea3 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 {
 	struct rproc *rproc = rvdev->rproc;
 	struct device *dev = &rproc->dev;
-	struct virtio_device *vdev = &rvdev->vdev;
+	struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
 	int ret;
 
 	vdev->id.device	= id,
@@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 	kref_get(&rvdev->refcount);
 
 	ret = register_virtio_device(vdev);
+	reg_dev = vdev;
 	if (ret) {
-		put_device(&rproc->dev);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
 		goto out;
 	}
 
 	dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
 
+	return 0;
+
 out:
+	if (reg_dev)
+		put_device(&vdev->dev);
+	else {
+		kref_put(&rvdev->refcount, rproc_vdev_release);
+		put_device(&rproc->dev);
+	}
+
 	return ret;
 }
 
-- 
2.9.4

Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

[Cornelia Huck]

On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758@gmail.com> wrote:

[you used a different mail address in your From: than in your s-o-b:;
same for the other patches]

> In order to make caller do a simple cleanup, we split device_register
> into device_initialize and device_add. device_initialize always sucess,

s/success/succeeds/

> the caller can always use put_device when fail to register virtio_device

"so the caller can always use put_device when register_virtio_device
failed,"

> no matter fail at ida_simple_get or at device_add.

"no matter whether it failed..."

> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  drivers/virtio/virtio.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index bf7ff39..3c9f211 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver)
>  }
>  EXPORT_SYMBOL_GPL(unregister_virtio_driver);
>  
> +/**
> + * register_virtio_device - register virtio device
> + * @dev        : virtio device interested

"virtio device to be registered"

> + *
> + * If an error occurs, the caller must use put_device, instead of kfree, because
> + * device_initialize and device_add will increase @dev->dev's reference count.

That's not correct: It's not because of device_add increasing the
reference count (it releases it again on failure), but because another
code path may have obtained a reference.

What about:

"On error, the caller must call put_device on &@dev->dev (and not
kfree), as another code path may have obtained a reference to @dev."

> + *
> + * Returns: 0 on suceess, -error on failure
> + */
>  int register_virtio_device(struct virtio_device *dev)
>  {
>  	int err;
>  
>  	dev->dev.bus = &virtio_bus;
> +	device_initialize(&dev->dev);
>  
>  	/* Assign a unique device index and hence name. */
>  	err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL);
> @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
>  
>  	INIT_LIST_HEAD(&dev->vqs);
>  
> -	/* device_register() causes the bus infrastructure to look for a
> -	 * matching driver. */
> -	err = device_register(&dev->dev);
> +	/*
> +	 * device_add() causes the bus infrastructure to look for a matching
> +	 * driver.

FWIW, I would just have done s/device_register/device_add/ in the
comment, but this is ok as well.

> +	 */
> +	err = device_add(&dev->dev);
>  	if (err)
>  		ida_simple_remove(&virtio_index_ida, dev->index);
>  out:

Your code change is fine.

Re: [PATCH v4 2/4] virtio_pci: don't kfree device on register failure

[Cornelia Huck]

On Wed, 20 Dec 2017 12:26:43 +0800
weiping zhang <zwp10758@gmail.com> wrote:

> As mentioned at drivers/base/core.c:
> /*
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> so we don't free vp_dev until vp_dev->vdev.dev.release be called.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..48d4d1c 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d)
>  static int virtio_pci_probe(struct pci_dev *pci_dev,
>  			    const struct pci_device_id *id)
>  {
> -	struct virtio_pci_device *vp_dev;
> +	struct virtio_pci_device *vp_dev, *reg_dev = NULL;

Not sure if I would have used a pointer for this purpose, but as that
is what Michael had proposed...

>  	int rc;
>  
>  	/* allocate our structure and fill it out */
> @@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	pci_set_master(pci_dev);
>  
>  	rc = register_virtio_device(&vp_dev->vdev);
> +	reg_dev = vp_dev;
>  	if (rc)
>  		goto err_register;
>  
> @@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  err_probe:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
> -	kfree(vp_dev);
> +	if (reg_dev)
> +		put_device(&vp_dev->vdev.dev);
> +	else
> +		kfree(vp_dev);
>  	return rc;
>  }
>  

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v4 3/4] virtio_vop: don't kfree device on register failure

[Cornelia Huck]

On Wed, 20 Dec 2017 12:27:04 +0800
weiping zhang <zwp10758@gmail.com> wrote:

> As mentioned at drivers/base/core.c:
> /*
>  * NOTE: _Never_ directly free @dev after calling this function, even
>  * if it returned an error! Always use put_device() to give up the
>  * reference initialized in this function instead.
>  */
> so we don't free vdev until vdev->vdev.dev.release be called.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> index a341938..3633202 100644
> --- a/drivers/misc/mic/vop/vop_main.c
> +++ b/drivers/misc/mic/vop/vop_main.c
> @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
>  
>  static void vop_virtio_release_dev(struct device *_d)
>  {
> -	/*
> -	 * No need for a release method similar to virtio PCI.
> -	 * Provide an empty one to avoid getting a warning from core.
> -	 */
> +	struct virtio_device *vdev =
> +			container_of(_d, struct virtio_device, dev);
> +	struct _vop_vdev *vop_vdev =
> +			container_of(vdev, struct _vop_vdev, vdev);
> +
> +	kfree(vop_vdev);
>  }
>  
>  /*
> @@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
>  			   unsigned int offset, struct vop_device *vpdev,
>  			   int dnode)
>  {
> -	struct _vop_vdev *vdev;
> +	struct _vop_vdev *vdev, *reg_dev = NULL;

Similarly, not a fan of that pointer variable, but it's fine.

>  	int ret;
>  	u8 type = ioread8(&d->type);
>  
> @@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
>  	vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db);
>  
>  	ret = register_virtio_device(&vdev->vdev);
> +	reg_dev = vdev;
>  	if (ret) {
>  		dev_err(_vop_dev(vdev),
>  			"Failed to register vop device %u type %u\n",
> @@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
>  free_irq:
>  	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
>  kfree:
> -	kfree(vdev);
> +	if (reg_dev)
> +		put_device(&vdev->vdev.dev);
> +	else
> +		kfree(vdev);
>  	return ret;
>  }
>  
> @@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d,
>  		iowrite8(-1, &dc->h2c_vdev_db);
>  		if (status & VIRTIO_CONFIG_S_DRIVER_OK)
>  			wait_for_completion(&vdev->reset_done);
> -		kfree(vdev);
> +		put_device(&vdev->vdev.dev);
>  		iowrite8(1, &dc->guest_ack);
>  		dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n",
>  			__func__, __LINE__, ioread8(&dc->guest_ack));

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure

[Cornelia Huck]

On Wed, 20 Dec 2017 12:27:33 +0800
weiping zhang <zwp10758@gmail.com> wrote:

> rproc_virtio_dev_release will be called iff virtio_device.dev's
> refer count became to 0. Here we should check if we call device_register

"reference count drops to 0"

s/call/called/

> or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
> done in rproc_virtio_dev_release, otherwise we do cleanup directly.
> 
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
>  drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
> index 2946348..1073ea3 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  {
>  	struct rproc *rproc = rvdev->rproc;
>  	struct device *dev = &rproc->dev;
> -	struct virtio_device *vdev = &rvdev->vdev;
> +	struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
>  	int ret;
>  
>  	vdev->id.device	= id,
> @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>  	kref_get(&rvdev->refcount);
>  
>  	ret = register_virtio_device(vdev);
> +	reg_dev = vdev;
>  	if (ret) {
> -		put_device(&rproc->dev);
>  		dev_err(dev, "failed to register vdev: %d\n", ret);
>  		goto out;
>  	}
>  
>  	dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
>  
> +	return 0;
> +
>  out:
> +	if (reg_dev)
> +		put_device(&vdev->dev);
> +	else {
> +		kref_put(&rvdev->refcount, rproc_vdev_release);
> +		put_device(&rproc->dev);
> +	}
> +
>  	return ret;
>  }
>  

I think in this case using the marker makes a straightforward cleanup
way too complicated. There's a single way we can get to the out label,
and that's when register_virtio_device() failed. Switching
put_device(&rproc->dev) to put_device(@vdev->dev) (what your first
patch did) seems like the way to go.

(It also may be good to cc: the maintainers for this driver.)

Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

[weiping zhang]

2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Wed, 20 Dec 2017 12:26:25 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
> [you used a different mail address in your From: than in your s-o-b:;
> same for the other patches]
>
>> In order to make caller do a simple cleanup, we split device_register
>> into device_initialize and device_add. device_initialize always sucess,
>
> s/success/succeeds/
>
>> the caller can always use put_device when fail to register virtio_device
>
> "so the caller can always use put_device when register_virtio_device
> failed,"
>
>> no matter fail at ida_simple_get or at device_add.
>
> "no matter whether it failed..."
>
>> + *
>> + * If an error occurs, the caller must use put_device, instead of kfree, because
>> + * device_initialize and device_add will increase @dev->dev's reference count.
>
> That's not correct: It's not because of device_add increasing the
> reference count (it releases it again on failure), but because another
> code path may have obtained a reference.
>
> What about:
>
> "On error, the caller must call put_device on &@dev->dev (and not
> kfree), as another code path may have obtained a reference to @dev."
>
It's good to understand, further more dev->name may has a bit mem leak.
anyway, I'll correct all comments at V5. Thanks very much.
>> + *
>> + * Returns: 0 on suceess, -error on failure
>> + */
>>  int register_virtio_device(struct virtio_device *dev)

Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add

[weiping zhang]

2017-12-21 10:37 GMT+08:00 weiping zhang <zwp10758@gmail.com>:
> 2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
>> On Wed, 20 Dec 2017 12:26:25 +0800
>> weiping zhang <zwp10758@gmail.com> wrote:
>>
>> [you used a different mail address in your From: than in your s-o-b:;
>> same for the other patches]
a wrong setting of my email client, correct it next time, thanks.
>>> In order to make caller do a simple cleanup, we split device_register
>>> into device_initialize and device_add. device_initialize always sucess,
>>
>> s/success/succeeds/
>>
>>> the caller can always use put_device when fail to register virtio_device
>>
>> "so the caller can always use put_device when register_virtio_device
>> failed,"
>>
>>> no matter fail at ida_simple_get or at device_add.
>>
>> "no matter whether it failed..."
>>
>>> + *
>>> + * If an error occurs, the caller must use put_device, instead of kfree, because
>>> + * device_initialize and device_add will increase @dev->dev's reference count.
>>
>> That's not correct: It's not because of device_add increasing the
>> reference count (it releases it again on failure), but because another
>> code path may have obtained a reference.
>>
>> What about:
>>
>> "On error, the caller must call put_device on &@dev->dev (and not
>> kfree), as another code path may have obtained a reference to @dev."
>>
> It's good to understand, further more dev->name may has a bit mem leak.
> anyway, I'll correct all comments at V5. Thanks very much.
>>> + *
>>> + * Returns: 0 on suceess, -error on failure
>>> + */
>>>  int register_virtio_device(struct virtio_device *dev)

Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure

[weiping zhang]

2017-12-21 0:12 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Wed, 20 Dec 2017 12:27:33 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
>> rproc_virtio_dev_release will be called iff virtio_device.dev's
>> refer count became to 0. Here we should check if we call device_register
>
> "reference count drops to 0"
>
> s/call/called/
rproc_add_virtio_dev is not like other virtio driver, it always call
register_virtio_device,
so commit message would be:

virtio_remoteproc: correct put_device virtio_device.dev

rproc_virtio_dev_release will be called iff virtio_device.dev's
reference count drops to 0. Here we just put vdev.dev, and then
rproc->dev's cleanup will be done in rproc_virtio_dev_release.


>> or not, if called, put vdev.dev, and then rproc->dev's cleanup will be
>> done in rproc_virtio_dev_release, otherwise we do cleanup directly.
>>
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> ---
>>  drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
>> index 2946348..1073ea3 100644
>> --- a/drivers/remoteproc/remoteproc_virtio.c
>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>> @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>  {
>>       struct rproc *rproc = rvdev->rproc;
>>       struct device *dev = &rproc->dev;
>> -     struct virtio_device *vdev = &rvdev->vdev;
>> +     struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL;
>>       int ret;
>>
>>       vdev->id.device = id,
>> @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
>>       kref_get(&rvdev->refcount);
>>
>>       ret = register_virtio_device(vdev);
>> +     reg_dev = vdev;
>>       if (ret) {
>> -             put_device(&rproc->dev);
>>               dev_err(dev, "failed to register vdev: %d\n", ret);
>>               goto out;
>>       }
>>
>>       dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id);
>>
>> +     return 0;
>> +
>>  out:
>> +     if (reg_dev)
>> +             put_device(&vdev->dev);
>> +     else {
>> +             kref_put(&rvdev->refcount, rproc_vdev_release);
>> +             put_device(&rproc->dev);
>> +     }
>> +
>>       return ret;
>>  }
>>
>
> I think in this case using the marker makes a straightforward cleanup
> way too complicated. There's a single way we can get to the out label,
> and that's when register_virtio_device() failed. Switching
> put_device(&rproc->dev) to put_device(@vdev->dev) (what your first
> patch did) seems like the way to go.
OK, put it directly at v5.
> (It also may be good to cc: the maintainers for this driver.)
OK, thanks