Virtualization - Dec 2017 - [PATCH 0/3] fix cleanup for fail to register_virtio_device

This series fix the cleanup for the caller of register_virtio_device,
the main work is use put_device instead of kfree.

weiping zhang (3):
  virtio_pci: use put_device instead of kfree
  virtio: use put_device instead of kfree
  virtio: put reference count of virtio_device.dev

 drivers/misc/mic/vop/vop_main.c        | 16 +++++++++-------
 drivers/remoteproc/remoteproc_virtio.c |  2 +-
 drivers/virtio/virtio_pci_common.c     | 17 +++++++++--------
 3 files changed, 19 insertions(+), 16 deletions(-)

-- 
2.9.4

don't free vp_dev until vp_dev->vdev.dev.release be called.

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

diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index 1c4797e..91d20f7 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
-	if (rc)
-		goto err_register;
+	if (rc) {
+		if (vp_dev->ioaddr)
+		     virtio_pci_legacy_remove(vp_dev);
+		else
+		     virtio_pci_modern_remove(vp_dev);
+		pci_disable_device(pci_dev);
+		put_device(&vp_dev->vdev.dev);
+	}
 
-	return 0;
+	return rc;
 
-err_register:
-	if (vp_dev->ioaddr)
-	     virtio_pci_legacy_remove(vp_dev);
-	else
-	     virtio_pci_modern_remove(vp_dev);
 err_probe:
 	pci_disable_device(pci_dev);
 err_enable_device:
-- 
2.9.4

don't free vp_vdev until vp_vdev.dev.release be called.

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

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..8c716a0 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);
 }
 
 /*
@@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 		dev_err(_vop_dev(vdev),
 			"Failed to register vop device %u type %u\n",
 			offset, type);
-		goto free_irq;
+		vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
+		put_device(&vdev->vdev.dev);
+		return ret;
 	}
 	writeq((u64)vdev, &vdev->dc->vdev);
 	dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev
%p\n",
@@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
 
 	return 0;
 
-free_irq:
-	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
 kfree:
 	kfree(vdev);
 	return ret;
-- 
2.9.4

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

Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
---
 drivers/remoteproc/remoteproc_virtio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..b0633fd 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
 
 	ret = register_virtio_device(vdev);
 	if (ret) {
-		put_device(&rproc->dev);
+		put_device(&vdev->dev);
 		dev_err(dev, "failed to register vdev: %d\n", ret);
 		goto out;
 	}
-- 
2.9.4

On Mon, 11 Dec 2017 23:55:16 +0800
weiping zhang <zhangweiping at didichuxing.com> wrote:
> don't free vp_dev until vp_dev->vdev.dev.release be called.
Maybe add the same description as in your virtio_mmio patch so that it
is clear why the kfree() is not ok?
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..91d20f7 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>  	pci_set_master(pci_dev);
>  
>  	rc = register_virtio_device(&vp_dev->vdev);
> -	if (rc)
> -		goto err_register;
> +	if (rc) {
> +		if (vp_dev->ioaddr)
> +		     virtio_pci_legacy_remove(vp_dev);
> +		else
> +		     virtio_pci_modern_remove(vp_dev);
> +		pci_disable_device(pci_dev);
> +		put_device(&vp_dev->vdev.dev);
> +	}
>  
> -	return 0;
> +	return rc;
>  
> -err_register:
> -	if (vp_dev->ioaddr)
> -	     virtio_pci_legacy_remove(vp_dev);
> -	else
> -	     virtio_pci_modern_remove(vp_dev);
>  err_probe:
>  	pci_disable_device(pci_dev);
>  err_enable_device:
Otherwise, looks good.

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

On Mon, 11 Dec 2017 23:55:26 +0800
weiping zhang <zhangweiping at didichuxing.com> wrote:
> don't free vp_vdev until vp_vdev.dev.release be called.
Same comment as for the virtio_pci patch.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c
b/drivers/misc/mic/vop/vop_main.c
> index a341938..8c716a0 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);
>  }
>  
>  /*
> @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc
__iomem *d,
>  		dev_err(_vop_dev(vdev),
>  			"Failed to register vop device %u type %u\n",
>  			offset, type);
> -		goto free_irq;
> +		vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> +		put_device(&vdev->vdev.dev);
> +		return ret;
>  	}
>  	writeq((u64)vdev, &vdev->dc->vdev);
>  	dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev
%p\n",
> @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc
__iomem *d,
>  
>  	return 0;
>  
> -free_irq:
> -	vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
>  kfree:
>  	kfree(vdev);
>  	return ret;
BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be
a put_device() as well).

On Mon, 11 Dec 2017 23:55:38 +0800
weiping zhang <zhangweiping at didichuxing.com> wrote:
> rproc_virtio_dev_release will be called iff virtio_device.dev's
> refer count became to 0. Here we should put vdev.dev, and then
> rproc->dev's cleanup will be done in rproc_virtio_dev_release.
> 
> Signed-off-by: weiping zhang <zhangweiping at didichuxing.com>
> ---
>  drivers/remoteproc/remoteproc_virtio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
> index 2946348..b0633fd 100644
> --- a/drivers/remoteproc/remoteproc_virtio.c
> +++ b/drivers/remoteproc/remoteproc_virtio.c
> @@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int
id)
>  
>  	ret = register_virtio_device(vdev);
>  	if (ret) {
> -		put_device(&rproc->dev);
> +		put_device(&vdev->dev);
>  		dev_err(dev, "failed to register vdev: %d\n", ret);
>  		goto out;
>  	}
Uh, rproc_vdev is really weird. It embeds a struct virtio_device, and
in addition a struct kref. At a glance, it seems to be correct, though.
Your change is more obviously sane.

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