All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Filip Hejsek <filip.hejsek@gmail.com>
Cc: Amit Shah <amit@kernel.org>, Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Rusty Russell <rusty@rustcorp.com.au>,
	virtualization@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RESEND] virtio_console: read size from config space during device init
Date: Wed, 10 Jun 2026 03:04:56 -0400	[thread overview]
Message-ID: <20260610030318-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20260223-virtio-console-fix-v1-1-0cf08303b428@gmail.com>

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>


      reply	other threads:[~2026-06-10  7:05 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-23 17:37 [PATCH RESEND] virtio_console: read size from config space during device init Filip Hejsek
2026-06-10  7:04 ` Michael S. Tsirkin [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260610030318-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=amit@kernel.org \
    --cc=arnd@arndb.de \
    --cc=filip.hejsek@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --cc=virtualization@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.