* [PATCH v2 0/2] Add cleanup for virtio_mmio driver
@ 2017-12-05 11:56 weiping zhang
2017-12-05 11:57 ` [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
2017-12-05 11:57 ` [PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove weiping zhang
0 siblings, 2 replies; 5+ messages in thread
From: weiping zhang @ 2017-12-05 11:56 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-05 11:56 [PATCH v2 0/2] Add cleanup for virtio_mmio driver weiping zhang
@ 2017-12-05 11:57 ` weiping zhang
2017-12-06 11:11 ` Cornelia Huck
2017-12-05 11:57 ` [PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove weiping zhang
1 sibling, 1 reply; 5+ messages in thread
From: weiping zhang @ 2017-12-05 11:57 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove
2017-12-05 11:56 [PATCH v2 0/2] Add cleanup for virtio_mmio driver weiping zhang
2017-12-05 11:57 ` [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
@ 2017-12-05 11:57 ` weiping zhang
1 sibling, 0 replies; 5+ messages in thread
From: weiping zhang @ 2017-12-05 11:57 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-05 11:57 ` [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
@ 2017-12-06 11:11 ` Cornelia Huck
2017-12-06 13:53 ` weiping zhang
0 siblings, 1 reply; 5+ messages in thread
From: Cornelia Huck @ 2017-12-06 11:11 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
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.]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-06 11:11 ` Cornelia Huck
@ 2017-12-06 13:53 ` weiping zhang
0 siblings, 0 replies; 5+ messages in thread
From: weiping zhang @ 2017-12-06 13:53 UTC (permalink / raw)
To: Cornelia Huck; +Cc: mst, weiping zhang, virtualization
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-06 13:53 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-05 11:56 [PATCH v2 0/2] Add cleanup for virtio_mmio driver weiping zhang
2017-12-05 11:57 ` [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
2017-12-06 11:11 ` Cornelia Huck
2017-12-06 13:53 ` weiping zhang
2017-12-05 11:57 ` [PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove weiping zhang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.