[PATCH RESEND] virtio_console: read size from config space during device init

[PATCH RESEND] virtio_console: read size from config space during device init

[Filip Hejsek]

Previously, the size was only read upon receiving the config interrupt.
This interrupt is sent when the size changes. However, we also need to
read the initial size.

Also make sure to only read the size from config if F_SIZE is enabled.

Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
---
This is a resend of [1], which hasn't received any response.

I found this bug while developing patches for QEMU that add virtio
console resize support. If you want to test this, you can get my QEMU
patches from [2]. You will need to disable multiport
using `-device virtio-serial,max_ports=1`.

[1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
[2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/

I'll also repeat my questions from the previous submission here. These are
things that confused me when I was trying to understand the surrounding code,
but should in no way prevent merging this patch.

  - Why does use_multiport use __virtio_test_bit instead of
    virtio_has_feature?

  - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
    think makes no sense?
---
 drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 088182e54d..c355f6d392 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
 		schedule_work(&portdev->config_work);
 }
 
-static void config_work_handler(struct work_struct *work)
+static void update_size_from_config(struct ports_device *portdev)
 {
-	struct ports_device *portdev;
+	struct virtio_device *vdev;
+	struct port *port;
+	u16 rows, cols;
 
-	portdev = container_of(work, struct ports_device, config_work);
-	if (!use_multiport(portdev)) {
-		struct virtio_device *vdev;
-		struct port *port;
-		u16 rows, cols;
+	vdev = portdev->vdev;
 
-		vdev = portdev->vdev;
-		virtio_cread(vdev, struct virtio_console_config, cols, &cols);
-		virtio_cread(vdev, struct virtio_console_config, rows, &rows);
+	/*
+	 * We'll use this way of resizing only for legacy support.
+	 * For multiport devices, use control messages to indicate
+	 * console size changes so that it can be done per-port.
+	 *
+	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
+	 */
+	if (is_rproc_serial(vdev) ||
+	    use_multiport(portdev) ||
+	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+		return;
 
-		port = find_port_by_id(portdev, 0);
-		set_console_size(port, rows, cols);
+	virtio_cread(vdev, struct virtio_console_config, cols, &cols);
+	virtio_cread(vdev, struct virtio_console_config, rows, &rows);
 
-		/*
-		 * We'll use this way of resizing only for legacy
-		 * support.  For newer userspace
-		 * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
-		 * to indicate console size changes so that it can be
-		 * done per-port.
-		 */
-		resize_console(port);
-	}
+	port = find_port_by_id(portdev, 0);
+	set_console_size(port, rows, cols);
+	resize_console(port);
+}
+
+static void config_work_handler(struct work_struct *work)
+{
+	struct ports_device *portdev;
+
+	portdev = container_of(work, struct ports_device, config_work);
+	update_size_from_config(portdev);
 }
 
 static int init_vqs(struct ports_device *portdev)
@@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
 	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
 			   VIRTIO_CONSOLE_DEVICE_READY, 1);
 
+	update_size_from_config(portdev);
+
 	return 0;
 
 free_chrdev:

---
base-commit: b927546677c876e26eba308550207c2ddf812a43
change-id: 20251224-virtio-console-fix-3d46980ef569

Best regards,
-- 
Filip Hejsek <filip.hejsek@gmail.com>

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Michael S. Tsirkin]

On Mon, Feb 23, 2026 at 06:37:02PM +0100, Filip Hejsek wrote:
> Previously, the size was only read upon receiving the config interrupt.
> This interrupt is sent when the size changes. However, we also need to
> read the initial size.
> 
> Also make sure to only read the size from config if F_SIZE is enabled.
> 
> Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
> Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> ---
> This is a resend of [1], which hasn't received any response.
> 
> I found this bug while developing patches for QEMU that add virtio
> console resize support. If you want to test this, you can get my QEMU
> patches from [2]. You will need to disable multiport
> using `-device virtio-serial,max_ports=1`.
> 
> [1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
> [2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/
> 
> I'll also repeat my questions from the previous submission here. These are
> things that confused me when I was trying to understand the surrounding code,
> but should in no way prevent merging this patch.
> 
>   - Why does use_multiport use __virtio_test_bit instead of
>     virtio_has_feature?
> 
>   - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
>     think makes no sense?
> ---
>  drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 088182e54d..c355f6d392 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
>  		schedule_work(&portdev->config_work);
>  }
>  
> -static void config_work_handler(struct work_struct *work)
> +static void update_size_from_config(struct ports_device *portdev)
>  {
> -	struct ports_device *portdev;
> +	struct virtio_device *vdev;
> +	struct port *port;
> +	u16 rows, cols;
>  
> -	portdev = container_of(work, struct ports_device, config_work);
> -	if (!use_multiport(portdev)) {
> -		struct virtio_device *vdev;
> -		struct port *port;
> -		u16 rows, cols;
> +	vdev = portdev->vdev;
>  
> -		vdev = portdev->vdev;
> -		virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> -		virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> +	/*
> +	 * We'll use this way of resizing only for legacy support.
> +	 * For multiport devices, use control messages to indicate
> +	 * console size changes so that it can be done per-port.
> +	 *
> +	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
> +	 */
> +	if (is_rproc_serial(vdev) ||

Wait a second. Why is there this rproc test here?
Was not in the original code and commit log says nothing about it.


> +	    use_multiport(portdev) ||
> +	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> +		return;
>  
> -		port = find_port_by_id(portdev, 0);
> -		set_console_size(port, rows, cols);
> +	virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> +	virtio_cread(vdev, struct virtio_console_config, rows, &rows);
>  
> -		/*
> -		 * We'll use this way of resizing only for legacy
> -		 * support.  For newer userspace
> -		 * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
> -		 * to indicate console size changes so that it can be
> -		 * done per-port.
> -		 */
> -		resize_console(port);
> -	}
> +	port = find_port_by_id(portdev, 0);
> +	set_console_size(port, rows, cols);
> +	resize_console(port);
> +}
> +
> +static void config_work_handler(struct work_struct *work)
> +{
> +	struct ports_device *portdev;
> +
> +	portdev = container_of(work, struct ports_device, config_work);
> +	update_size_from_config(portdev);
>  }
>  
>  static int init_vqs(struct ports_device *portdev)
> @@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
>  	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
>  			   VIRTIO_CONSOLE_DEVICE_READY, 1);
>  
> +	update_size_from_config(portdev);
> +
>  	return 0;
>  
>  free_chrdev:
> 
> ---
> base-commit: b927546677c876e26eba308550207c2ddf812a43
> change-id: 20251224-virtio-console-fix-3d46980ef569
> 
> Best regards,
> -- 
> Filip Hejsek <filip.hejsek@gmail.com>

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Filip Hejsek]

On Wed, 2026-06-10 at 03:04 -0400, Michael S. Tsirkin wrote:
> On Mon, Feb 23, 2026 at 06:37:02PM +0100, Filip Hejsek wrote:
> > Previously, the size was only read upon receiving the config interrupt.
> > This interrupt is sent when the size changes. However, we also need to
> > read the initial size.
> > 
> > Also make sure to only read the size from config if F_SIZE is enabled.
> > 
> > Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
> > Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> > ---
> > This is a resend of [1], which hasn't received any response.
> > 
> > I found this bug while developing patches for QEMU that add virtio
> > console resize support. If you want to test this, you can get my QEMU
> > patches from [2]. You will need to disable multiport
> > using `-device virtio-serial,max_ports=1`.
> > 
> > [1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
> > [2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/
> > 
> > I'll also repeat my questions from the previous submission here. These are
> > things that confused me when I was trying to understand the surrounding code,
> > but should in no way prevent merging this patch.
> > 
> >   - Why does use_multiport use __virtio_test_bit instead of
> >     virtio_has_feature?
> > 
> >   - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
> >     think makes no sense?
> > ---
> >  drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 31 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 088182e54d..c355f6d392 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
> >  		schedule_work(&portdev->config_work);
> >  }
> >  
> > -static void config_work_handler(struct work_struct *work)
> > +static void update_size_from_config(struct ports_device *portdev)
> >  {
> > -	struct ports_device *portdev;
> > +	struct virtio_device *vdev;
> > +	struct port *port;
> > +	u16 rows, cols;
> >  
> > -	portdev = container_of(work, struct ports_device, config_work);
> > -	if (!use_multiport(portdev)) {
> > -		struct virtio_device *vdev;
> > -		struct port *port;
> > -		u16 rows, cols;
> > +	vdev = portdev->vdev;
> >  
> > -		vdev = portdev->vdev;
> > -		virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > -		virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > +	/*
> > +	 * We'll use this way of resizing only for legacy support.
> > +	 * For multiport devices, use control messages to indicate
> > +	 * console size changes so that it can be done per-port.
> > +	 *
> > +	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
> > +	 */
> > +	if (is_rproc_serial(vdev) ||
> 
> Wait a second. Why is there this rproc test here?
> Was not in the original code and commit log says nothing about it.
> 

Previously, this code was in config_work_handler(), which was never
called for rproc_serial (it's scheduled from config_intr(), which is
the config_changed handler only for virtio_console).

Now update_size_from_config() is called unconditionally from
virtcons_probe(), so it will be called for rproc_serial too, which
doesn't have the F_SIZE feature.

> 
> > +	    use_multiport(portdev) ||
> > +	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > +		return;
> >  
> > -		port = find_port_by_id(portdev, 0);
> > -		set_console_size(port, rows, cols);
> > +	virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > +	virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> >  
> > -		/*
> > -		 * We'll use this way of resizing only for legacy
> > -		 * support.  For newer userspace
> > -		 * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
> > -		 * to indicate console size changes so that it can be
> > -		 * done per-port.
> > -		 */
> > -		resize_console(port);
> > -	}
> > +	port = find_port_by_id(portdev, 0);
> > +	set_console_size(port, rows, cols);
> > +	resize_console(port);
> > +}
> > +
> > +static void config_work_handler(struct work_struct *work)
> > +{
> > +	struct ports_device *portdev;
> > +
> > +	portdev = container_of(work, struct ports_device, config_work);
> > +	update_size_from_config(portdev);
> >  }
> >  
> >  static int init_vqs(struct ports_device *portdev)
> > @@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
> >  	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> >  			   VIRTIO_CONSOLE_DEVICE_READY, 1);
> >  
> > +	update_size_from_config(portdev);
> > +
> >  	return 0;
> >  
> >  free_chrdev:
> > 
> > ---
> > base-commit: b927546677c876e26eba308550207c2ddf812a43
> > change-id: 20251224-virtio-console-fix-3d46980ef569
> > 
> > Best regards,
> > -- 
> > Filip Hejsek <filip.hejsek@gmail.com>

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 08:57:57AM +0200, Filip Hejsek wrote:
> On Wed, 2026-06-10 at 03:04 -0400, Michael S. Tsirkin wrote:
> > On Mon, Feb 23, 2026 at 06:37:02PM +0100, Filip Hejsek wrote:
> > > Previously, the size was only read upon receiving the config interrupt.
> > > This interrupt is sent when the size changes. However, we also need to
> > > read the initial size.
> > > 
> > > Also make sure to only read the size from config if F_SIZE is enabled.
> > > 
> > > Fixes: 9778829cffd4 ("virtio: console: Store each console's size in the console structure")
> > > Signed-off-by: Filip Hejsek <filip.hejsek@gmail.com>
> > > ---
> > > This is a resend of [1], which hasn't received any response.
> > > 
> > > I found this bug while developing patches for QEMU that add virtio
> > > console resize support. If you want to test this, you can get my QEMU
> > > patches from [2]. You will need to disable multiport
> > > using `-device virtio-serial,max_ports=1`.
> > > 
> > > [1]: https://lore.kernel.org/all/20251224-virtio-console-fix-v1-1-69d0349692dc@gmail.com/
> > > [2]: https://lore.kernel.org/all/20250921-console-resize-v5-0-89e3c6727060@gmail.com/
> > > 
> > > I'll also repeat my questions from the previous submission here. These are
> > > things that confused me when I was trying to understand the surrounding code,
> > > but should in no way prevent merging this patch.
> > > 
> > >   - Why does use_multiport use __virtio_test_bit instead of
> > >     virtio_has_feature?
> > > 
> > >   - The VIRTIO_CONSOLE_RESIZE handler sets irq_requested to 1, which I
> > >     think makes no sense?
> > > ---
> > >  drivers/char/virtio_console.c | 52 ++++++++++++++++++++++++++-----------------
> > >  1 file changed, 31 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index 088182e54d..c355f6d392 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -1771,32 +1771,40 @@ static void config_intr(struct virtio_device *vdev)
> > >  		schedule_work(&portdev->config_work);
> > >  }
> > >  
> > > -static void config_work_handler(struct work_struct *work)
> > > +static void update_size_from_config(struct ports_device *portdev)
> > >  {
> > > -	struct ports_device *portdev;
> > > +	struct virtio_device *vdev;
> > > +	struct port *port;
> > > +	u16 rows, cols;
> > >  
> > > -	portdev = container_of(work, struct ports_device, config_work);
> > > -	if (!use_multiport(portdev)) {
> > > -		struct virtio_device *vdev;
> > > -		struct port *port;
> > > -		u16 rows, cols;
> > > +	vdev = portdev->vdev;
> > >  
> > > -		vdev = portdev->vdev;
> > > -		virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > > -		virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > > +	/*
> > > +	 * We'll use this way of resizing only for legacy support.
> > > +	 * For multiport devices, use control messages to indicate
> > > +	 * console size changes so that it can be done per-port.
> > > +	 *
> > > +	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
> > > +	 */
> > > +	if (is_rproc_serial(vdev) ||
> > 
> > Wait a second. Why is there this rproc test here?
> > Was not in the original code and commit log says nothing about it.
> > 
> 
> Previously, this code was in config_work_handler(), which was never
> called for rproc_serial (it's scheduled from config_intr(), which is
> the config_changed handler only for virtio_console).
> 
> Now update_size_from_config() is called unconditionally from
> virtcons_probe(), so it will be called for rproc_serial too, which
> doesn't have the F_SIZE feature.

So why not test it? What does "not a valid feature" mean?
I dislike transport code leaking into devices.


> > 
> > > +	    use_multiport(portdev) ||
> > > +	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > +		return;
> > >  
> > > -		port = find_port_by_id(portdev, 0);
> > > -		set_console_size(port, rows, cols);
> > > +	virtio_cread(vdev, struct virtio_console_config, cols, &cols);
> > > +	virtio_cread(vdev, struct virtio_console_config, rows, &rows);
> > >  
> > > -		/*
> > > -		 * We'll use this way of resizing only for legacy
> > > -		 * support.  For newer userspace
> > > -		 * (VIRTIO_CONSOLE_F_MULTPORT+), use control messages
> > > -		 * to indicate console size changes so that it can be
> > > -		 * done per-port.
> > > -		 */
> > > -		resize_console(port);
> > > -	}
> > > +	port = find_port_by_id(portdev, 0);
> > > +	set_console_size(port, rows, cols);
> > > +	resize_console(port);
> > > +}
> > > +
> > > +static void config_work_handler(struct work_struct *work)
> > > +{
> > > +	struct ports_device *portdev;
> > > +
> > > +	portdev = container_of(work, struct ports_device, config_work);
> > > +	update_size_from_config(portdev);
> > >  }
> > >  
> > >  static int init_vqs(struct ports_device *portdev)
> > > @@ -2054,6 +2062,8 @@ static int virtcons_probe(struct virtio_device *vdev)
> > >  	__send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> > >  			   VIRTIO_CONSOLE_DEVICE_READY, 1);
> > >  
> > > +	update_size_from_config(portdev);
> > > +
> > >  	return 0;
> > >  
> > >  free_chrdev:
> > > 
> > > ---
> > > base-commit: b927546677c876e26eba308550207c2ddf812a43
> > > change-id: 20251224-virtio-console-fix-3d46980ef569
> > > 
> > > Best regards,
> > > -- 
> > > Filip Hejsek <filip.hejsek@gmail.com>

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Filip Hejsek]

On Thu, 2026-06-11 at 03:38 -0400, Michael S. Tsirkin wrote:
> [...]
> > > 
> > > Wait a second. Why is there this rproc test here?
> > > Was not in the original code and commit log says nothing about it.
> > > 
> > 
> > Previously, this code was in config_work_handler(), which was never
> > called for rproc_serial (it's scheduled from config_intr(), which is
> > the config_changed handler only for virtio_console).
> > 
> > Now update_size_from_config() is called unconditionally from
> > virtcons_probe(), so it will be called for rproc_serial too, which
> > doesn't have the F_SIZE feature.
> 
> So why not test it? 

The virtio_console driver implements two similar but distinct virtio
devices: VIRTIO_ID_CONSOLE and VIRTIO_ID_RPROC_SERIAL. Although some of
the implementation code is shared, the devices are different. In
particular, rproc_serial doesn't support multiport nor any of the tty
specific features. This means that the relevant feature bits are not
valid for this device and must not be tested.

I have to admit though that I don't quite understand what the
RPROC_SERIAL device is supposed to be used for. It was added by commit
1b6370463e88b0c1c317de16d7b962acc1dab4f2, which describes it as "a
simple serial connection driver called VIRTIO_ID_RPROC_SERIAL (11) for
communicating with a remote processor in an asymmetric multi-processing
configuration". It seems that it was never standardized, as the virtio
spec only says that its ID is reserved.

> What does "not a valid feature" mean?

I copied the "not a valid feature" comment form other instances in the
same file where a feature is tested, e.g. in resize_console():

	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
	if (!is_rproc_serial(vdev) &&
	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
	  hvc_resize(port->cons.hvc, port->cons.ws);


Best regards,
Filip Hejsek
> > 

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 10:29:50AM +0200, Filip Hejsek wrote:
> On Thu, 2026-06-11 at 03:38 -0400, Michael S. Tsirkin wrote:
> > [...]
> > > > 
> > > > Wait a second. Why is there this rproc test here?
> > > > Was not in the original code and commit log says nothing about it.
> > > > 
> > > 
> > > Previously, this code was in config_work_handler(), which was never
> > > called for rproc_serial (it's scheduled from config_intr(), which is
> > > the config_changed handler only for virtio_console).
> > > 
> > > Now update_size_from_config() is called unconditionally from
> > > virtcons_probe(), so it will be called for rproc_serial too, which
> > > doesn't have the F_SIZE feature.
> > 
> > So why not test it? 
> 
> The virtio_console driver implements two similar but distinct virtio
> devices: VIRTIO_ID_CONSOLE and VIRTIO_ID_RPROC_SERIAL. Although some of
> the implementation code is shared, the devices are different. In
> particular, rproc_serial doesn't support multiport nor any of the tty
> specific features. This means that the relevant feature bits are not
> valid for this device and must not be tested.



> I have to admit though that I don't quite understand what the
> RPROC_SERIAL device is supposed to be used for. It was added by commit
> 1b6370463e88b0c1c317de16d7b962acc1dab4f2, which describes it as "a
> simple serial connection driver called VIRTIO_ID_RPROC_SERIAL (11) for
> communicating with a remote processor in an asymmetric multi-processing
> configuration". It seems that it was never standardized, as the virtio
> spec only says that its ID is reserved.
> 
> > What does "not a valid feature" mean?
> 
> I copied the "not a valid feature" comment form other instances in the
> same file where a feature is tested, e.g. in resize_console():
> 
> 	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
> 	if (!is_rproc_serial(vdev) &&
> 	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> 	  hvc_resize(port->cons.hvc, port->cons.ws);
> 
> 
> Best regards,
> Filip Hejsek

I get it, it's existing code.  It still makes no sense.

rproc has:

static const unsigned int rproc_serial_features[] = {
};      

No features.
So testing any feature bit at all always returns 0.

there's no reason to special case anything.

So I'm testing this, but I'm only compiling rproc, so pls holler if it seems wrong:


diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 198b97314168..2261862d4b4c 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -340,7 +340,7 @@ static inline bool use_multiport(struct ports_device *portdev)
 	 */
 	if (!portdev->vdev)
 		return false;
-	return __virtio_test_bit(portdev->vdev, VIRTIO_CONSOLE_F_MULTIPORT);
+	return virtio_has_feature(portdev->vdev, VIRTIO_CONSOLE_F_MULTIPORT);
 }
 
 static DEFINE_SPINLOCK(dma_bufs_lock);
@@ -1156,9 +1156,7 @@ static void resize_console(struct port *port)
 
 	vdev = port->portdev->vdev;
 
-	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
-	if (!is_rproc_serial(vdev) &&
-	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
+	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
 		hvc_resize(port->cons.hvc, port->cons.ws);
 }
 
@@ -1783,11 +1781,8 @@ static void update_size_from_config(struct ports_device *portdev)
 	 * We'll use this way of resizing only for legacy support.
 	 * For multiport devices, use control messages to indicate
 	 * console size changes so that it can be done per-port.
-	 *
-	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
 	 */
-	if (is_rproc_serial(vdev) ||
-	    use_multiport(portdev) ||
+	if (use_multiport(portdev) ||
 	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
 		return;
 
@@ -1994,9 +1989,7 @@ static int virtcons_probe(struct virtio_device *vdev)
 	multiport = false;
 	portdev->max_nr_ports = 1;
 
-	/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
-	if (!is_rproc_serial(vdev) &&
-	    virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
+	if (virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
 				 struct virtio_console_config, max_nr_ports,
 				 &portdev->max_nr_ports) == 0) {
 		if (portdev->max_nr_ports == 0 ||





-- 
MST

Re: [PATCH RESEND] virtio_console: read size from config space during device init

[Filip Hejsek]

On Thu, 2026-06-11 at 05:01 -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 11, 2026 at 10:29:50AM +0200, Filip Hejsek wrote:
> > On Thu, 2026-06-11 at 03:38 -0400, Michael S. Tsirkin wrote:
> > > [...]
> > > > > 
> > > > > Wait a second. Why is there this rproc test here?
> > > > > Was not in the original code and commit log says nothing about it.
> > > > > 
> > > > 
> > > > Previously, this code was in config_work_handler(), which was never
> > > > called for rproc_serial (it's scheduled from config_intr(), which is
> > > > the config_changed handler only for virtio_console).
> > > > 
> > > > Now update_size_from_config() is called unconditionally from
> > > > virtcons_probe(), so it will be called for rproc_serial too, which
> > > > doesn't have the F_SIZE feature.
> > > 
> > > So why not test it? 
> > 
> > The virtio_console driver implements two similar but distinct virtio
> > devices: VIRTIO_ID_CONSOLE and VIRTIO_ID_RPROC_SERIAL. Although some of
> > the implementation code is shared, the devices are different. In
> > particular, rproc_serial doesn't support multiport nor any of the tty
> > specific features. This means that the relevant feature bits are not
> > valid for this device and must not be tested.
> 
> 
> 
> > I have to admit though that I don't quite understand what the
> > RPROC_SERIAL device is supposed to be used for. It was added by commit
> > 1b6370463e88b0c1c317de16d7b962acc1dab4f2, which describes it as "a
> > simple serial connection driver called VIRTIO_ID_RPROC_SERIAL (11) for
> > communicating with a remote processor in an asymmetric multi-processing
> > configuration". It seems that it was never standardized, as the virtio
> > spec only says that its ID is reserved.
> > 
> > > What does "not a valid feature" mean?
> > 
> > I copied the "not a valid feature" comment form other instances in the
> > same file where a feature is tested, e.g. in resize_console():
> > 
> > 	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
> > 	if (!is_rproc_serial(vdev) &&
> > 	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > 	  hvc_resize(port->cons.hvc, port->cons.ws);
> > 
> > 
> > Best regards,
> > Filip Hejsek
> 
> I get it, it's existing code.  It still makes no sense.
> 
> rproc has:
> 
> static const unsigned int rproc_serial_features[] = {
> };      
> 
> No features.
> So testing any feature bit at all always returns 0.

virtio_has_feature() will BUG() if called with a feature that hasn't
been offered by the driver (see virtio_check_driver_offered_feature).

(Maybe that was why __virtio_test_bit was used? But that seems pretty
hacky to me.)

> 
> there's no reason to special case anything.
> 
> So I'm testing this, but I'm only compiling rproc, so pls holler if it seems wrong:
> 
> 
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 198b97314168..2261862d4b4c 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -340,7 +340,7 @@ static inline bool use_multiport(struct ports_device *portdev)
>  	 */
>  	if (!portdev->vdev)
>  		return false;
> -	return __virtio_test_bit(portdev->vdev, VIRTIO_CONSOLE_F_MULTIPORT);
> +	return virtio_has_feature(portdev->vdev, VIRTIO_CONSOLE_F_MULTIPORT);
>  }
>  
>  static DEFINE_SPINLOCK(dma_bufs_lock);
> @@ -1156,9 +1156,7 @@ static void resize_console(struct port *port)
>  
>  	vdev = port->portdev->vdev;
>  
> -	/* Don't test F_SIZE at all if we're rproc: not a valid feature! */
> -	if (!is_rproc_serial(vdev) &&
> -	    virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> +	if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
>  		hvc_resize(port->cons.hvc, port->cons.ws);
>  }
>  
> @@ -1783,11 +1781,8 @@ static void update_size_from_config(struct ports_device *portdev)
>  	 * We'll use this way of resizing only for legacy support.
>  	 * For multiport devices, use control messages to indicate
>  	 * console size changes so that it can be done per-port.
> -	 *
> -	 * Don't test F_SIZE at all if we're rproc: not a valid feature.
>  	 */
> -	if (is_rproc_serial(vdev) ||
> -	    use_multiport(portdev) ||
> +	if (use_multiport(portdev) ||
>  	    !virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
>  		return;
>  
> @@ -1994,9 +1989,7 @@ static int virtcons_probe(struct virtio_device *vdev)
>  	multiport = false;
>  	portdev->max_nr_ports = 1;
>  
> -	/* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
> -	if (!is_rproc_serial(vdev) &&
> -	    virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
> +	if (virtio_cread_feature(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
>  				 struct virtio_console_config, max_nr_ports,
>  				 &portdev->max_nr_ports) == 0) {
>  		if (portdev->max_nr_ports == 0 ||
> 
> 
> 
>