virtio_mmio: add cleanup for virtio_mmio_probe

PATCH In review weiping zhang 2 participants 3 messages Dec 1, 2017 – Dec 5, 2017 virtio core

lore

Share & tools

lore

weiping zhang

author

2017-12-01 17:51 UTC #0 lore raw

PATCH 1/1 virtio_mmio: add cleanup for virtio_mmio_probe

cleanup all resource allocated by virtio_mmio_probe.

Signed-off-by weiping zhang <[email protected]>

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

drivers/virtio/virtio_mmio.c +26 -8

index 74dc717..3fd0e66 100644
@@ -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)

Signature · 1 line

2.9.4

Cornelia Huck

2017-12-04 10:24 UTC #1 lore raw

Quoted 12 lines · On Sat, 2 Dec 2017 01:51:40 +0800 weiping zhang <[email protected]> wrote:

cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by weiping zhang <[email protected]>
---
 drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)
drivers/virtio/virtio_mmio.c +0 -0
index 74dc717..3fd0e66 100644

drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 517

Quoted 7 lines

drivers/virtio/virtio_mmio.c +2 -2

@@ -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 :)

Quoted 5 lines

drivers/virtio/virtio_mmio.c +2 -0

+		goto free_mem;
+	}
 
 	vm_dev->vdev.dev.parent = &pdev->dev;
 	vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;

(...)

Quoted 6 lines

drivers/virtio/virtio_mmio.c +1 -1

@@ -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);

drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 593

Quoted 10 lines

drivers/virtio/virtio_mmio.c +10 -0

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

Quoted 4 lines

drivers/virtio/virtio_mmio.c +1 -0

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

weiping zhang

author

2017-12-05 01:36 UTC #2 lore raw

2017-12-04 18:24 GMT+08:00 Cornelia Huck <[email protected]>:

Quoted 17 lines

Quoted 14 lines · On Sat, 2 Dec 2017 01:51:40 +0800 weiping zhang <[email protected]> wrote:
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by weiping zhang <[email protected]>
---
 drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)
drivers/virtio/virtio_mmio.c +0 -0
index 74dc717..3fd0e66 100644
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
              return -EBUSY;
 

Quoted 8 lines

drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 517
Quoted 5 lines
drivers/virtio/virtio_mmio.c +2 -2
      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.

Quoted 34 lines

Quoted 6 lines
drivers/virtio/virtio_mmio.c +2 -0
+             goto free_mem;
+     }
 
      vm_dev->vdev.dev.parent = &pdev->dev;
      vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
(...)
Quoted 6 lines
drivers/virtio/virtio_mmio.c +1 -1
@@ -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);
drivers/virtio/virtio_mmio.c static int virtio_mmio_probe(struct platform_device *pdev) · line 593
Quoted 10 lines
drivers/virtio/virtio_mmio.c +10 -0
+     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

Quoted 3 lines

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