* [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
@ 2017-12-01 17:51 weiping zhang
2017-12-04 10:24 ` Cornelia Huck
0 siblings, 1 reply; 3+ messages in thread
From: weiping zhang @ 2017-12-01 17:51 UTC (permalink / raw)
To: mst, jasowang, cohuck; +Cc: virtualization
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..3fd0e66 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,18 @@ 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 unmap;
+ return 0;
+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] 3+ messages in thread
* Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-01 17:51 [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
@ 2017-12-04 10:24 ` Cornelia Huck
2017-12-05 1:36 ` weiping zhang
0 siblings, 1 reply; 3+ messages in thread
From: Cornelia Huck @ 2017-12-04 10:24 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Sat, 2 Dec 2017 01:51:40 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> cleanup all resource allocated by virtio_mmio_probe.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 74dc717..3fd0e66 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;
Touching this would be a good time to remove the extra space in front
of the -ENOMEM :)
> + goto free_mem;
> + }
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
(...)
> @@ -573,7 +580,18 @@ 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 unmap;
> + return 0;
> +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);
I think this is problematic as vm_dev embeds a struct device (via
embedding a struct virtio_device). I think the right way to do this is
- call this only if register_virtio_device() has not been called
- put the devm_kfree() into the ->release callback for the
virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
function that calls this)
- do a put_device() if register_virtio_device() failed
I might be missing some interaction between the usual driver model
handling and devm resources, though.
> + return rc;
> }
>
> static int virtio_mmio_remove(struct platform_device *pdev)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-04 10:24 ` Cornelia Huck
@ 2017-12-05 1:36 ` weiping zhang
0 siblings, 0 replies; 3+ messages in thread
From: weiping zhang @ 2017-12-05 1:36 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
2017-12-04 18:24 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Sat, 2 Dec 2017 01:51:40 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
>> cleanup all resource allocated by virtio_mmio_probe.
>>
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> ---
>> drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index 74dc717..3fd0e66 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;
>
> Touching this would be a good time to remove the extra space in front
> of the -ENOMEM :)
Thanks, I fix this at V2.
>
>> + goto free_mem;
>> + }
>>
>> vm_dev->vdev.dev.parent = &pdev->dev;
>> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
>
> (...)
>
>> @@ -573,7 +580,18 @@ 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 unmap;
>> + return 0;
>> +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);
>
> I think this is problematic as vm_dev embeds a struct device (via
> embedding a struct virtio_device). I think the right way to do this is
> - call this only if register_virtio_device() has not been called
> - put the devm_kfree() into the ->release callback for the
> virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
> function that calls this)
> - do a put_device() if register_virtio_device() failed
>
> I might be missing some interaction between the usual driver model
> handling and devm resources, though.
>
if fail in virtio_mmio_probe, we need free @mem of @pdev, vdev->base,
so I prefer clean all this resource in virtio_mmio_probe itself, keep
vdev.dev.release do nothing.
also we need release these resources at virtio_mmio_remove.
I'll send V2.
--
thanks a ton
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-05 1:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-01 17:51 [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe weiping zhang
2017-12-04 10:24 ` Cornelia Huck
2017-12-05 1:36 ` weiping zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).