Re: [PATCH] virtio: console: Prepare for making REMOTEPROC modular

[Amit Shah <amit@kernel.org>]

On Fri, 2025-02-14 at 12:14 +0100, Uwe Kleine-König wrote:
> Hello Amit,
> 
> 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@baylibre.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?

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.

		Amit