[PATCH] virtio_console: Convert to use devm funcs

[PATCH] virtio_console: Convert to use devm funcs

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

Convert to devm_* funcs so that no need to manual free in error path.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/char/virtio_console.c | 43 ++++++++++++++---------------------
 1 file changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index c62b208b42f1..657cf15dad55 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1811,15 +1811,17 @@ static int init_vqs(struct ports_device *portdev)
 	nr_ports = portdev->max_nr_ports;
 	nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
 
-	vqs = kmalloc_array(nr_queues, sizeof(struct virtqueue *), GFP_KERNEL);
-	vqs_info = kcalloc(nr_queues, sizeof(*vqs_info), GFP_KERNEL);
-	portdev->in_vqs = kmalloc_array(nr_ports, sizeof(struct virtqueue *),
-					GFP_KERNEL);
-	portdev->out_vqs = kmalloc_array(nr_ports, sizeof(struct virtqueue *),
-					 GFP_KERNEL);
+	vqs = devm_kmalloc_array(&portdev->vdev->dev, nr_queues,
+				 sizeof(struct virtqueue *), GFP_KERNEL);
+	vqs_info = devm_kcalloc(&portdev->vdev->dev, nr_queues,
+				sizeof(*vqs_info), GFP_KERNEL);
+	portdev->in_vqs = devm_kmalloc_array(&portdev->vdev->dev,
+					     nr_ports, sizeof(struct virtqueue *), GFP_KERNEL);
+	portdev->out_vqs = devm_kmalloc_array(&portdev->vdev->dev,
+					      nr_ports, sizeof(struct virtqueue *), GFP_KERNEL);
 	if (!vqs || !vqs_info || !portdev->in_vqs || !portdev->out_vqs) {
 		err = -ENOMEM;
-		goto free;
+		return err;
 	}
 
 	/*
@@ -1850,7 +1852,7 @@ static int init_vqs(struct ports_device *portdev)
 	/* Find the queues. */
 	err = virtio_find_vqs(portdev->vdev, nr_queues, vqs, vqs_info, NULL);
 	if (err)
-		goto free;
+		return err;
 
 	j = 0;
 	portdev->in_vqs[0] = vqs[0];
@@ -1866,18 +1868,10 @@ static int init_vqs(struct ports_device *portdev)
 			portdev->out_vqs[i] = vqs[j + 1];
 		}
 	}
-	kfree(vqs_info);
-	kfree(vqs);
+	devm_kfree(&portdev->vdev->dev, vqs_info);
+	devm_kfree(&portdev->vdev->dev, vqs);
 
 	return 0;
-
-free:
-	kfree(portdev->out_vqs);
-	kfree(portdev->in_vqs);
-	kfree(vqs_info);
-	kfree(vqs);
-
-	return err;
 }
 
 static const struct file_operations portdev_fops = {
@@ -1897,8 +1891,8 @@ static void remove_vqs(struct ports_device *portdev)
 		cond_resched();
 	}
 	portdev->vdev->config->del_vqs(portdev->vdev);
-	kfree(portdev->in_vqs);
-	kfree(portdev->out_vqs);
+	devm_kfree(&portdev->vdev->dev, portdev->in_vqs);
+	devm_kfree(&portdev->vdev->dev, portdev->out_vqs);
 }
 
 static void virtcons_remove(struct virtio_device *vdev)
@@ -1941,7 +1935,6 @@ static void virtcons_remove(struct virtio_device *vdev)
 	 * away.
 	 */
 	remove_vqs(portdev);
-	kfree(portdev);
 }
 
 /*
@@ -1967,7 +1960,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 		return -EINVAL;
 	}
 
-	portdev = kmalloc(sizeof(*portdev), GFP_KERNEL);
+	portdev = devm_kmalloc(&vdev->dev, sizeof(*portdev), GFP_KERNEL);
 	if (!portdev) {
 		err = -ENOMEM;
 		goto fail;
@@ -1984,7 +1977,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 			"Error %d registering chrdev for device %u\n",
 			portdev->chr_major, vdev->index);
 		err = portdev->chr_major;
-		goto free;
+		goto fail;
 	}
 
 	multiport = false;
@@ -2001,7 +1994,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 				"Invalidate max_nr_ports %d",
 				portdev->max_nr_ports);
 			err = -EINVAL;
-			goto free;
+			goto fail;
 		}
 		multiport = true;
 	}
@@ -2060,8 +2053,6 @@ static int virtcons_probe(struct virtio_device *vdev)
 
 free_chrdev:
 	unregister_chrdev(portdev->chr_major, "virtio-portsdev");
-free:
-	kfree(portdev);
 fail:
 	return err;
 }
-- 
2.43.0

Re: [PATCH] virtio_console: Convert to use devm funcs

[Amit Shah]

On Tue, 2025-01-28 at 13:52 +0800, oushixiong1025@163.com wrote:
> From: Shixiong Ou <oushixiong@kylinos.cn>
> 
> Convert to devm_* funcs so that no need to manual free in error path.
> 
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>  drivers/char/virtio_console.c | 43 ++++++++++++++-------------------
> --
>  1 file changed, 17 insertions(+), 26 deletions(-)

[...]

> -		goto free;
> +		return err;

[...]

> -
> -free:
> -	kfree(portdev->out_vqs);
> -	kfree(portdev->in_vqs);
> -	kfree(vqs_info);
> -	kfree(vqs);
> -
> -	return err;
>  }

Hm, I'm not entirely sure about this - the devm_ interface is better,
but to me that just says that it's an extra safety net that avoids
memleaks when we forget to kfree, and not that we deliberately do not
free and get lax about managing allocated memory.

So I'd prefer a patch that keeps all current frees as they are, but
with the added advantage of using the devm interfaces.

		Amit

Re: [PATCH] virtio_console: Convert to use devm funcs

[oushixiong]

在 2025/1/28 17:39, Amit Shah 写道:
> On Tue, 2025-01-28 at 13:52 +0800, oushixiong1025@163.com wrote:
>> From: Shixiong Ou <oushixiong@kylinos.cn>
>>
>> Convert to devm_* funcs so that no need to manual free in error path.
>>
>> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
>> ---
>>   drivers/char/virtio_console.c | 43 ++++++++++++++-------------------
>> --
>>   1 file changed, 17 insertions(+), 26 deletions(-)
> [...]
>
>> -		goto free;
>> +		return err;
> [...]
>
>> -
>> -free:
>> -	kfree(portdev->out_vqs);
>> -	kfree(portdev->in_vqs);
>> -	kfree(vqs_info);
>> -	kfree(vqs);
>> -
>> -	return err;
>>   }
> Hm, I'm not entirely sure about this - the devm_ interface is better,
> but to me that just says that it's an extra safety net that avoids
> memleaks when we forget to kfree, and not that we deliberately do not
> free and get lax about managing allocated memory.
>
> So I'd prefer a patch that keeps all current frees as they are, but
> with the added advantage of using the devm interfaces.
>
> 		Amit

Hi, I don't think driver need to  call kfree() by manual if driver use 
the devm_ interface

in the probe() phase. This can simplifies the code.

This is similar to the  commit  "55dd7378ba94 (fbdev: imxfb: Convert to 
devm_kmalloc_array())"

Thanks!

Shixiong Ou

Re: [PATCH] virtio_console: Convert to use devm funcs

[Arnd Bergmann]

On Wed, Jan 29, 2025, at 12:48, oushixiong wrote:
> 在 2025/1/28 17:39, Amit Shah 写道:
>> On Tue, 2025-01-28 at 13:52 +0800, oushixiong1025@163.com wrote:
>>
>> Hm, I'm not entirely sure about this - the devm_ interface is better,
>> but to me that just says that it's an extra safety net that avoids
>> memleaks when we forget to kfree, and not that we deliberately do not
>> free and get lax about managing allocated memory.
>>
>> So I'd prefer a patch that keeps all current frees as they are, but
>> with the added advantage of using the devm interfaces.
>>
>
> Hi, I don't think driver need to  call kfree() by manual if driver use 
> the devm_ interface

Calling the plain kfree() on memory allocated by devm_kmalloc()
is a bug, it has to be freed when the device is released or
explicitly using devm_kfree(). The explicit devm_kfree() in
turn is what you'd use only if memory has to be freed early
while the device is still going to be used.

> in the probe() phase. This can simplifies the code.
>
> This is similar to the  commit  "55dd7378ba94 (fbdev: imxfb: Convert to 
> devm_kmalloc_array())"

I don't think it's worth converting drivers that correctly use
kmalloc()/kfree() already. For new drivers, the devm_* functions
are usually the easiest to use and review, and it makes sense to
convert drivers that have known memory leaks from incorrect
cleanup paths, but I don't think it's a good use of developer
or reviewer time to convert drivers that are not wrong for the
sake of simplicity. There is a memory overhead in the devm_*
functions and there is a risk of introducing bugs if the conversion
is done incorrectly, e.g. when a regular kfree() is left behind,
or when changing allocations that are meant to be freed before
.release() is called.

       Arnd