[PATCH v2 0/2] Add cleanup for virtio_mmio driver

[PATCH v2 0/2] Add cleanup for virtio_mmio driver

[weiping zhang]

this patchset try to add cleanup for virtio_mmio driver, include
virtio_mmio_probe and virtio_mmio_remove

weiping zhang (2):
  virtio_mmio: add cleanup for virtio_mmio_probe
  virtio_mmio: add cleanup for virtio_mmio_remove

 drivers/virtio/virtio_mmio.c | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

-- 
2.9.4

[PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe

[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.
 */

Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
we need release @mem resource from @pdev and vm_dev->base. It make
@pdev->vm_dev.dev.release() too complicated, so put_device just put the
reference of register_virtio_device->device_register->device_initialize
and release all resource in virtio_mmio_probe.

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

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..f984510 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 		return -EBUSY;
 
 	vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
-	if (!vm_dev)
-		return  -ENOMEM;
+	if (!vm_dev) {
+		rc = -ENOMEM;
+		goto free_mem;
+	}
 
 	vm_dev->vdev.dev.parent = &pdev->dev;
 	vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 	spin_lock_init(&vm_dev->lock);
 
 	vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
-	if (vm_dev->base == NULL)
-		return -EFAULT;
+	if (vm_dev->base == NULL) {
+		rc = -EFAULT;
+		goto free_vmdev;
+	}
 
 	/* Check magic value */
 	magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
 	if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
 		dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
-		return -ENODEV;
+		rc = -ENODEV;
+		goto unmap;
 	}
 
 	/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 	if (vm_dev->version < 1 || vm_dev->version > 2) {
 		dev_err(&pdev->dev, "Version %ld not supported!\n",
 				vm_dev->version);
-		return -ENXIO;
+		rc = -ENXIO;
+		goto unmap;
 	}
 
 	vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 		 * virtio-mmio device with an ID 0 is a (dummy) placeholder
 		 * with no function. End probing now with no error reported.
 		 */
-		return -ENODEV;
+		rc = -ENODEV;
+		goto unmap;
 	}
 	vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
 
@@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, vm_dev);
 
-	return register_virtio_device(&vm_dev->vdev);
+	rc = register_virtio_device(&vm_dev->vdev);
+	if (rc)
+		goto put_dev;
+	return 0;
+put_dev:
+	put_device(&vm_dev->vdev.dev);
+unmap:
+	iounmap(vm_dev->base);
+free_mem:
+	devm_release_mem_region(&pdev->dev, mem->start,
+			resource_size(mem));
+free_vmdev:
+	devm_kfree(&pdev->dev, vm_dev);
+	return rc;
 }
 
 static int virtio_mmio_remove(struct platform_device *pdev)
-- 
2.9.4

[PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove

[weiping zhang]

cleanup all resource allocated by virtio_mmio_probe.

Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
 drivers/virtio/virtio_mmio.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index f984510..5e2ca34 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -599,8 +599,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 static int virtio_mmio_remove(struct platform_device *pdev)
 {
 	struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+	struct resource *mem;
 
 	unregister_virtio_device(&vm_dev->vdev);
+	iounmap(vm_dev->base);
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (mem)
+		devm_release_mem_region(&pdev->dev, mem->start,
+			resource_size(mem));
+	devm_kfree(&pdev->dev, vm_dev);
 
 	return 0;
 }
-- 
2.9.4

Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe

[Cornelia Huck]

On Tue, 5 Dec 2017 19:57:10 +0800
weiping zhang <zhangweiping@didichuxing.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.
>  */
> 
> Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
> we need release @mem resource from @pdev and vm_dev->base. It make
> @pdev->vm_dev.dev.release() too complicated, so put_device just put the
> reference of register_virtio_device->device_register->device_initialize
> and release all resource in virtio_mmio_probe.

Releasing the resources when unwinding on error can work, but I think
there still are some issues (more below). This is all very tangly
code :(

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

> @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, vm_dev);
>  
> -	return register_virtio_device(&vm_dev->vdev);
> +	rc = register_virtio_device(&vm_dev->vdev);
> +	if (rc)
> +		goto put_dev;
> +	return 0;
> +put_dev:
> +	put_device(&vm_dev->vdev.dev);

Here you give up the extra reference from device_initialize(), which
may or may not be the last reference (since you don't know if
device_add() had already exposed the struct device to other code that
might have acquired a reference). As the device has an empty release
function, touching the device structure after that is not a real
problem, but...

> +unmap:
> +	iounmap(vm_dev->base);
> +free_mem:
> +	devm_release_mem_region(&pdev->dev, mem->start,
> +			resource_size(mem));
> +free_vmdev:
> +	devm_kfree(&pdev->dev, vm_dev);

...unconditionally freeing the device here would be a problem if other
code had acquired a reference above. (Unlikely, but we should try to
get this right.)

> +	return rc;
>  }
>  
>  static int virtio_mmio_remove(struct platform_device *pdev)

So, I think there are basically two ways of doing that:
- Move the cleanup into the currently empty release callback. Then, you
  won't need to touch the remove function. The problem with that is
  that you can't trigger a cleanup via put_device() if you did not call
  register_virtio_device() yet.
- Move just devm_kfree() into the release function. Cleanup the
  resources here, do the put_device() last thing if had you called
  register_virtio_device() before and devm_kfree() if you didn't.

[Of course, I still might be missing some devm subtility, so other
comments are welcome.]

Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe

[weiping zhang]

2017-12-06 19:11 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Tue, 5 Dec 2017 19:57:10 +0800
> weiping zhang <zhangweiping@didichuxing.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.
>>  */
>>
>> Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
>> we need release @mem resource from @pdev and vm_dev->base. It make
>> @pdev->vm_dev.dev.release() too complicated, so put_device just put the
>> reference of register_virtio_device->device_register->device_initialize
>> and release all resource in virtio_mmio_probe.
>
> Releasing the resources when unwinding on error can work, but I think
> there still are some issues (more below). This is all very tangly
> code :(
>
>>
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> ---
>>  drivers/virtio/virtio_mmio.c | 36 ++++++++++++++++++++++++++++--------
>>  1 file changed, 28 insertions(+), 8 deletions(-)
>>
>
>> @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>
>>       platform_set_drvdata(pdev, vm_dev);
>>
>> -     return register_virtio_device(&vm_dev->vdev);
>> +     rc = register_virtio_device(&vm_dev->vdev);
>> +     if (rc)
>> +             goto put_dev;
>> +     return 0;
>> +put_dev:
>> +     put_device(&vm_dev->vdev.dev);
>
> Here you give up the extra reference from device_initialize(), which
> may or may not be the last reference (since you don't know if
> device_add() had already exposed the struct device to other code that
> might have acquired a reference). As the device has an empty release
> function, touching the device structure after that is not a real
> problem, but...
>
>> +unmap:
>> +     iounmap(vm_dev->base);
>> +free_mem:
>> +     devm_release_mem_region(&pdev->dev, mem->start,
>> +                     resource_size(mem));
>> +free_vmdev:
>> +     devm_kfree(&pdev->dev, vm_dev);
>
> ...unconditionally freeing the device here would be a problem if other
> code had acquired a reference above. (Unlikely, but we should try to
> get this right.)
>
that's true, so we don't free it until it's refer count decrease to 0 and
->release called.
>> +     return rc;
>>  }
>>
>>  static int virtio_mmio_remove(struct platform_device *pdev)
>
> So, I think there are basically two ways of doing that:
> - Move the cleanup into the currently empty release callback. Then, you
>   won't need to touch the remove function. The problem with that is
>   that you can't trigger a cleanup via put_device() if you did not call
>   register_virtio_device() yet.
> - Move just devm_kfree() into the release function. Cleanup the
>   resources here, do the put_device() last thing if had you called
>   register_virtio_device() before and devm_kfree() if you didn't.
>
I prefer go second way. I'll send v3 later.
> [Of course, I still might be missing some devm subtility, so other
> comments are welcome.]

--
Thanks very much
weiping
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization