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