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

[Filip Hejsek <filip.hejsek@gmail.com>]

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