Message-ID: <606b16787920ff5e1807e4f8450add5889fdd1cb.camel@kernel.org>
Date: Fri, 14 Feb 2025 17:37:52 +0100
From: Amit Shah <amit@...nel.org>
To: Uwe Kleine-König <u.kleine-koenig@...libre.com>
Cc: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman
	 <gregkh@...uxfoundation.org>, virtualization@...ts.linux.dev, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virtio: console: Prepare for making REMOTEPROC modular

On Fri, 2025-02-14 at 17:13 +0100, Uwe Kleine-König wrote:
> Hello Amit,
> 
> On Fri, Feb 14, 2025 at 02:32:16PM +0100, Amit Shah wrote:
> > On Fri, 2025-02-14 at 12:14 +0100, Uwe Kleine-König wrote:
> > > On Fri, Feb 14, 2025 at 11:58:44AM +0100, Amit Shah wrote:
> > > > On Thu, 2025-02-13 at 12:55 +0100, Uwe Kleine-König wrote:
> > > > > virtio_console.c can make use of REMOTEPROC. Therefore it has
> > > > > several
> > > > > tests evaluating
> > > > > 
> > > > > 	IS_ENABLED(CONFIG_REMOTEPROC)
> > > > > 
> > > > > . This currently only does the right thing because
> > > > > CONFIG_REMOTEPROC
> > > > > cannot be modular. Otherwise the configuration
> > > > > 
> > > > > 	CONFIG_REMOTEPROC=m
> > > > > 	CONFIG_VIRTIO_CONSOLE=y
> > > > > 
> > > > > would result in a build failure because then
> > > > > IS_ENABLED(CONFIG_REMOTEPROC) evaluates to true but still the
> > > > > built-
> > > > > in
> > > > > virtio_console.o must not use symbols from the remoteproc
> > > > > module.
> > > > > 
> > > > > To prepare for making REMOTEPROC modular change the tests to
> > > > > use
> > > > > IS_REACHABLE() instead of IS_ENABLED() which copes correctly
> > > > > for
> > > > > the
> > > > > above case as it evaluates to false then.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König
> > > > > <u.kleine-koenig@...libre.com>
> > > > > ---
> > > > > Hello,
> > > > > 
> > > > > I didn't check what else needs to be done to make
> > > > > CONFIG_REMOTEPROC
> > > > > tristate but even if it stays a bool using IS_REACHABLE() is
> > > > > still
> > > > > the
> > > > > better choice.
> > > > 
> > > > It might lead to a false sense of "better" -- the value of
> > > > IS_ENABLED
> > > > is cached in a variable which is determined at compile-time.
> > > 
> > > Either I don't understand what you mean, or this is wrong.
> > > 
> > > 	$ make allmodconfig drivers/char/virtio_console.i
> > > 	$ grep CONFIG_REMOTEPROC= .config
> > > 	CONFIG_REMOTEPROC=m
> > > 	$ cat drivers/char/virtio_console.i
> > > 	...
> > > 	static bool is_rproc_serial(const struct virtio_device
> > > *vdev)
> > > 	{
> > > 	 return 1 && vdev->id.device == 11;
> > > 	}
> > > 	...
> > > 
> > > 
> > > so is_rproc_enabled is still constant and known at compile time.
> > 
> > Well - so I was replying to your comment about what else is
> > required. 
> > And if remoteproc becomes a module, this check will not happen at
> > compile-time?
> 
> The code I quoted is with both CONFIG_REMOTEPROC=m and
> CONFIG_VIRTIO_CONSOLE=m.
> 
> > In any case, the next bit is the more important one:
> > 
> > > > That
> > > > caching, after this change, moves to driver init-time.  If the
> > > > rproc
> > > > module is loaded after virtio-console is initialized, there's
> > > > no
> > > > way
> > > > it's going to be used.
> > > 
> > > If both are modular, modprobe should make sure that rproc is
> > > ready
> > > before virtio-console. If virtio-console is builtin and rproc is
> > > modular, IS_REACHABLE(CONFIG_REMOTEPROC) evaluates to false and
> > > so
> > > rproc
> > > won't be used. (As is the case already today with
> > > CONFIG_REMOTEPROC=n).
> > > 
> > > > Only if the rproc module is loaded before
> > > > virtio-console will the rproc functionality be used -- which
> > > > means
> > > > that
> > > > nothing changed in reality..
> > > 
> > > With that patch indeed nothing changed yet, because
> > > CONFIG_REMOTEPROC
> > > cannot be =m today. Until this changes IS_REACHABLE() and
> > > IS_ENABLED()
> > > are equivalent.
> > >  
> > > > To properly detect and use rproc if available would need the
> > > > rproc
> > > > initialization out of virtcons_probe() and into something that
> > > > happens
> > > > either via sysfs for existing ports, or when adding a new port
> > > > to a
> > > > device.  However, the current spec doesn't allow for that, so
> > > > some
> > > > more
> > > > changes will need to be made to ensure current backwards
> > > > compat,
> > > > and a
> > > > new specification that allows for a late init of rproc.
> > > 
> > > I didn't understand that and hope it's irrelevant with the things
> > > I
> > > wrote above.
> > 
> > See
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> > 
> > The device is configured at probe time on how it's going to be used
> > -
> > all that will have to be reworked for making the remoteproc driver
> > tristate.
> > 
> > So in essence, this patch isn't changing anything; but it's not
> > helping
> > the case you want to enable either.
> 
> I still don't understand. With both CONFIG_REMOTEPROC=m and
> CONFIG_VIRTIO_CONSOLE=m the expression is_rproc_serial(vdev) will be
> true iff vdev->id.device == VIRTIO_ID_RPROC_SERIAL. (And with
> CONFIG_REMOTEPROC=m and CONFIG_VIRTIO_CONSOLE=y it will be 0.)
> 
> Both IS_ENABLED(CONFIG_REMOTEPROC) and
> IS_REACHABLE(CONFIG_REMOTEPROC)
> evaluate to constants known at compile time.
> 
> So the device is still configured at probe time on how it's going to
> be
> used and I don't see what needs to be reworked.
> 
> If you're still convinced there is something broken, would you please
> point out in which case (CONFIG_REMOTEPROC=?,
> CONFIG_VIRTIO_CONSOLE=?)
> something broken happens, and what this is?

I'm thinking of the two combinations of interest: REMOTEPROC=m,
VIRTIO_CONSOLE can be y or m.  Say virtcons_probe() happens when the
remoteproc module isn't yet loaded.  Even after later loading
remoteproc, virtio console won't do anything interesting with
remoteproc.

		Amit

Powered by blists - more mailing lists