From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f42.google.com (mail-wm1-f42.google.com [209.85.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5F594285CB9 for ; Thu, 11 Jun 2026 09:09:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.42 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781168974; cv=none; b=Kjvoc06mIcYIK1jPfZsotTAuSU0P0iZMZWaSb9MjXnlm9RqKC6b6CANNlcnohkZPIBDw/TmeV93+RYfx8Ft7gkjsgcSYxbnlkoMnIvDviBq96v7a3qbJVwxUBJ4w3PvfIgxIBnkkzeQtw3vKK7e9OkRonKoYSuOElm2MXSIma5k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781168974; c=relaxed/simple; bh=uXw4kUrKvGdpvA7TizOZIWTxRiIoQDCHNqxd/QzJqJA=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=f3eVOrhOGtIU0mOPs9USBKLuaIn/2WcP0byFbNq9mGjSqB3BsEnuJIlM6xQSfeGIDbISfLvNsSemJJvWX9WaNYAw/QezkpBNuTYCkY++Ur0feOE32FfldKndzKscgvRVRKNweu7+3zTSQ8LJvAdkZDlXk9/IE3ShWkQc4qPeZo8= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=jdp3H4re; arc=none smtp.client-ip=209.85.128.42 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="jdp3H4re" Received: by mail-wm1-f42.google.com with SMTP id 5b1f17b1804b1-490abf12f0fso3694335e9.0 for ; Thu, 11 Jun 2026 02:09:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1781168972; x=1781773772; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject :date:message-id:reply-to; bh=5oPLEjtKrkT2zg/kgRAfIr3x/KHKzvjTCXuRNjefBxw=; b=jdp3H4reLZiLJifaCrAklNibaR9eMKk97nWkeNNGBraYNRyBuk26v1FX5e1nVoydt5 agdL0rQHqaNmO8NrHDM+6cLf9ooOSeLLgzabjKsU5llvwTfNN4vvQacHB/qtFTSqc3DR atWTr0eY57+5sZZqWmeYRRSaLON/En3ADkJNtuMP8q+lvGr8SyXmdExRphgAE9ZWIQN+ yN8Qn2HZ/FEzR6OPVHgNvQmZah0jppP30ijMBovbGO0XUb775MO4ufC+e4IAcISrPnFT RcmbILmhF1xagtCyh0CyYHC2EyP82A/k09qdq1ikGGn/5JvorwDF4SSsddn8aSA4D7Ek 97Dw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781168972; x=1781773772; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=5oPLEjtKrkT2zg/kgRAfIr3x/KHKzvjTCXuRNjefBxw=; b=OTGV9o86Zl9fi4ELT8P4vsZWZz7jdYIIMsKcI4VT8nmQ/pWQnUUxsntksmsgnoj5Q/ +3sH4ttFGUAA8uFxaUOShjONUgfJ5NedoCbYkPjURmsbDPSVwLG1m+K+s4t14XBww0o0 LPENkLcENrelQUmhx7R+pfzny/tE3799DeQoqsn+sUiax8jFCZw0OHeeUhGXkf+NVo4Z EiNyr18VTH4Q4LPnH0GKtvxtlRMC1W2it6BkfUP5QUBS36U3kpDIL6KjKhV/qG685kYm +P1vhL6loiWxagOsgaIlDVqtUJEQhqTTf3HXrGsuCGuSZWkBxljmp8a8jlBcHZUAw6zM 6oww== X-Forwarded-Encrypted: i=1; AFNElJ/apGQP4P9YPp6RDh9Rcxf1iVSNki9bcUzZ6pGQelI5WV8vx7iBTtMfa+OrNiG1pcl54/lbDCtzQDR6BLg=@vger.kernel.org X-Gm-Message-State: AOJu0YxZMNbrlLX/1DbusXO9E8o0ln+PmFPrXJSMnDQ29Y13Yeix5LKf u+wCeR3tq72bcL/ZX1k1A1ieseDhzBRVwhhcm0YsReethKceARL1w2fr X-Gm-Gg: Acq92OF7mxw4zxsEjfI76WjL/V46vkzl9PA2Gn1KfFxTOnhROYr1JwQrPLxDxC9HrfN FH8tMZWMcl6opUK/3WX0DXT0L5km6rVR3MBSBEvgy+MwVoRZKRJA/9HJ6rB2ou7+IXTcbr1X4k9 ZyXINQS1dLBeXxBjS5eaWhJ1K72zK70YrjZ3d72+KOOB0EKWPWXaE//zUJy1BDMxNnbZjZcDjIN r1H/CDe3v3iKbn8H2EhzF3v0iYgUg6K44ziBpjul6BMnTLtrp/+6W7iag7ENrUUZTrT+YCVA7+t zTJtNh/QxYnkYILC+bLtZsWphl/lWRPqxd/v3BnWM+ShYbfWG+594m0fdMTQ3jakzydXtbVM8ht CTLjjL4GT3IvW1F2YlqKdFJkf0fPljOYfH049Ui/CknduxRmo14rOTCavcsYX1GZXsnGX3nAAO7 CRU2GUGJpR5oN7/ayzs+5qA6b7YV+mS9TzvXp21Rd0PQEazna+seeqqvGFif1QZCRDNmW8 X-Received: by 2002:a7b:c40e:0:b0:490:b5ab:b41f with SMTP id 5b1f17b1804b1-490e5139f2bmr13700595e9.13.1781168971521; Thu, 11 Jun 2026 02:09:31 -0700 (PDT) Received: from [10.33.80.40] (mem-185.47.220.165.jmnet.cz. [185.47.220.165]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-4601f345209sm85649650f8f.17.2026.06.11.02.09.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jun 2026 02:09:31 -0700 (PDT) Message-ID: <13ed71f8e2bc22476fb0dd48eb17af4917f4f03b.camel@gmail.com> Subject: Re: [PATCH RESEND] virtio_console: read size from config space during device init From: Filip Hejsek To: "Michael S. Tsirkin" Cc: Amit Shah , Arnd Bergmann , Greg Kroah-Hartman , Rusty Russell , virtualization@lists.linux.dev, linux-kernel@vger.kernel.org Date: Thu, 11 Jun 2026 11:09:30 +0200 In-Reply-To: <20260611044443-mutt-send-email-mst@kernel.org> References: <20260223-virtio-console-fix-v1-1-0cf08303b428@gmail.com> <20260610030318-mutt-send-email-mst@kernel.org> <20260611033747-mutt-send-email-mst@kernel.org> <831dca185e55f56a14c0c580ab33ce84361eb67b.camel@gmail.com> <20260611044443-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 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: > > > [...] > > > > >=20 > > > > > Wait a second. Why is there this rproc test here? > > > > > Was not in the original code and commit log says nothing about it= . > > > > >=20 > > > >=20 > > > > Previously, this code was in config_work_handler(), which was never > > > > called for rproc_serial (it's scheduled from config_intr(), which i= s > > > > the config_changed handler only for virtio_console). > > > >=20 > > > > 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. > > >=20 > > > So why not test it?=C2=A0 > >=20 > > 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. >=20 >=20 >=20 > > 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. > >=20 > > > What does "not a valid feature" mean? > >=20 > > 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(): > >=20 > > /* 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); > >=20 > >=20 > > Best regards, > > Filip Hejsek >=20 > I get it, it's existing code. It still makes no sense. >=20 > rproc has: >=20 > static const unsigned int rproc_serial_features[] =3D { > }; =20 >=20 > 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.) >=20 > there's no reason to special case anything. >=20 > So I'm testing this, but I'm only compiling rproc, so pls holler if it se= ems wrong: >=20 >=20 > 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); > } > =20 > static DEFINE_SPINLOCK(dma_bufs_lock); > @@ -1156,9 +1156,7 @@ static void resize_console(struct port *port) > =20 > vdev =3D port->portdev->vdev; > =20 > - /* 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); > } > =20 > @@ -1783,11 +1781,8 @@ static void update_size_from_config(struct ports_d= evice *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; > =20 > @@ -1994,9 +1989,7 @@ static int virtcons_probe(struct virtio_device *vde= v) > multiport =3D false; > portdev->max_nr_ports =3D 1; > =20 > - /* 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) =3D=3D 0) { > if (portdev->max_nr_ports =3D=3D 0 || >=20 >=20 >=20 >=20