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

[Michael S. Tsirkin]

On Thu, Jun 11, 2026 at 11:09:30AM +0200, Filip Hejsek wrote:
> 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.)


Ah. Yes exactly. That's exactly what __virtio_test_bit is for
in use_multiport.
Want to try reworking the patch below using this?

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