Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang 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.

seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
annotations went to stable, should this go there too?

> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>

OK but this relies on users knowing that register_virtio_device
calls device_register. I think we want to add a comment
to register_virtio_device.

Also the cleanup is uglified.

I really think the right thing would be to change device_register making
it safe to kfree. People have the right to expect register on failure to
have no effect.

That just might be too hard to implement though.

For now, my suggestion - add a variable.

> ---
>  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

I'd prefer something like the below.

--->

virtio_pci: don't kfree device on register failure

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1c4797e..995ab03 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,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
 	pci_set_master(pci_dev);
 
 	rc = register_virtio_device(&vp_dev->vdev);
+	/* NOTE: device is considered registered even if register failed. */
+	reg_dev = vp_dev;
 	if (rc)
 		goto err_register;
 
@@ -564,7 +566,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(dev);
+	else
+		kfree(vp_dev);
 	return rc;
 }