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

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

[Uwe Kleine-König]

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.

Best regards
Uwe

 drivers/char/virtio_console.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 24442485e73e..7d7463d6f218 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -28,7 +28,7 @@
 #include <linux/dma-mapping.h>
 #include "../tty/hvc/hvc_console.h"
 
-#define is_rproc_enabled IS_ENABLED(CONFIG_REMOTEPROC)
+#define is_rproc_enabled IS_REACHABLE(CONFIG_REMOTEPROC)
 #define VIRTCONS_MAX_PORTS 0x8000
 
 /*
@@ -2078,7 +2078,7 @@ static const unsigned int features[] = {
 };
 
 static const struct virtio_device_id rproc_serial_id_table[] = {
-#if IS_ENABLED(CONFIG_REMOTEPROC)
+#if IS_REACHABLE(CONFIG_REMOTEPROC)
 	{ VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
 #endif
 	{ 0 },

base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3
-- 
2.47.1

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

[Amit Shah]

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.  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.  Only if the rproc module is loaded before
virtio-console will the rproc functionality be used -- which means that
nothing changed in reality..

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.


		Amit

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

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.

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

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Amit Shah]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4957 bytes --]

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

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?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Amit Shah]

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

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

Hello Amit,

On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> 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.

Where does the interesting thing happen if remoteproc is already loaded
at that time? I'm not seeing anything interesting in that case either
...

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Amit Shah]

On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> Hello Amit,
> 
> On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > 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.
> 
> Where does the interesting thing happen if remoteproc is already
> loaded
> at that time? I'm not seeing anything interesting in that case either
> ...

The code I pointed to,

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993

either enables remoteproc if the module is present; or it enables
multiport, but not both at the same time.  If remoteproc isn't present
when this probe routine is executed, multiport might get enabled.  And
then there's no chance for remoteproc to get enabled.

		Amit

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On Fri, Feb 14, 2025 at 05:55:41PM +0100, Amit Shah wrote:
> On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> > Hello Amit,
> > 
> > On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > > 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.
> > 
> > Where does the interesting thing happen if remoteproc is already
> > loaded
> > at that time? I'm not seeing anything interesting in that case either
> > ...
> 
> The code I pointed to,
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> 
> either enables remoteproc if the module is present; or it enables
> multiport, but not both at the same time.  If remoteproc isn't present
> when this probe routine is executed, multiport might get enabled.  And
> then there's no chance for remoteproc to get enabled.

The only case where there is a difference between IS_REACHABLE and
IS_ENABLED is:

	CONFIG_REMOTEPROC=m
	CONFIG_VIRTIO_CONSOLE=y

Iff in this case you never want to test for MULTIPORT (even though the
remoteproc module might never get loaded), then my patch is wrong.

When creating the patch I thought there is a hard dependency on
remoteproc (like calling a function that is provided by
CONFIG_REMOTEPROC). I don't understand how the remoteproc stuff
interacts with the virtio_console driver, is this something in userspace
which then would also autoload the remoteproc module?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Amit Shah]

On Fri, 2025-02-14 at 18:47 +0100, Uwe Kleine-König wrote:
> On Fri, Feb 14, 2025 at 05:55:41PM +0100, Amit Shah wrote:
> > On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> > > Hello Amit,
> > > 
> > > On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > > > 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.
> > > 
> > > Where does the interesting thing happen if remoteproc is already
> > > loaded
> > > at that time? I'm not seeing anything interesting in that case
> > > either
> > > ...
> > 
> > The code I pointed to,
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> > 
> > either enables remoteproc if the module is present; or it enables
> > multiport, but not both at the same time.  If remoteproc isn't
> > present
> > when this probe routine is executed, multiport might get enabled. 
> > And
> > then there's no chance for remoteproc to get enabled.
> 
> The only case where there is a difference between IS_REACHABLE and
> IS_ENABLED is:
> 
> 	CONFIG_REMOTEPROC=m
> 	CONFIG_VIRTIO_CONSOLE=y

Well, also if CONFIG_VIRTIO_CONSOLE=m; and virtio_console.ko is loaded
before remoteproc.ko.

> Iff in this case you never want to test for MULTIPORT (even though
> the
> remoteproc module might never get loaded), then my patch is wrong.
> 
> When creating the patch I thought there is a hard dependency on
> remoteproc (like calling a function that is provided by
> CONFIG_REMOTEPROC). I don't understand how the remoteproc stuff
> interacts with the virtio_console driver, is this something in
> userspace
> which then would also autoload the remoteproc module?

What's happening is that multiport and remoteproc are mutually
exclusive in the virtio_console code.

And, I'm also not sure of how remoteproc loads and configures itself. 
Does loading remoteproc cause a load of virtio_console?  How?

So - based on our discussions, I think your assumptions are:

1. remoteproc will load virtio_console when remoteproc is required
2. virtio_console will never be loaded by itself
3. General virtio_console functionality (including tty and multiport)
is never used when remoteproc is used

I think at least 3 needs more thought/justification why it's a valid
assumption.  Documenting it in the commit msg is fine.

At least assumptions 1 and 2 will cause remoteproc to not function
correctly with virtio_console, despite both of them being loaded
(because they can be loaded in the unexpected order -- virtio_console
before remoteproc).  Do you want to adjust the code so that remoteproc
queries for already-existing virtio_console.ko, triggers the code that
would otherwise be not triggered in virtcons_probe(), and makes
remoteproc functional in that case?

		Amit

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

[Amit Shah]

On Mon, 2025-02-17 at 11:53 +0100, Amit Shah wrote:
> On Fri, 2025-02-14 at 18:47 +0100, Uwe Kleine-König wrote:
> > On Fri, Feb 14, 2025 at 05:55:41PM +0100, Amit Shah wrote:
> > > On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> > > > Hello Amit,
> > > > 
> > > > On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > > > > 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.
> > > > 
> > > > Where does the interesting thing happen if remoteproc is
> > > > already
> > > > loaded
> > > > at that time? I'm not seeing anything interesting in that case
> > > > either
> > > > ...
> > > 
> > > The code I pointed to,
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> > > 
> > > either enables remoteproc if the module is present; or it enables
> > > multiport, but not both at the same time.  If remoteproc isn't
> > > present
> > > when this probe routine is executed, multiport might get
> > > enabled. 
> > > And
> > > then there's no chance for remoteproc to get enabled.
> > 
> > The only case where there is a difference between IS_REACHABLE and
> > IS_ENABLED is:
> > 
> > 	CONFIG_REMOTEPROC=m
> > 	CONFIG_VIRTIO_CONSOLE=y
> 
> Well, also if CONFIG_VIRTIO_CONSOLE=m; and virtio_console.ko is
> loaded
> before remoteproc.ko.
> 
> > Iff in this case you never want to test for MULTIPORT (even though
> > the
> > remoteproc module might never get loaded), then my patch is wrong.
> > 
> > When creating the patch I thought there is a hard dependency on
> > remoteproc (like calling a function that is provided by
> > CONFIG_REMOTEPROC). I don't understand how the remoteproc stuff
> > interacts with the virtio_console driver, is this something in
> > userspace
> > which then would also autoload the remoteproc module?
> 
> What's happening is that multiport and remoteproc are mutually
> exclusive in the virtio_console code.
> 
> And, I'm also not sure of how remoteproc loads and configures itself.
> Does loading remoteproc cause a load of virtio_console?  How?
> 
> So - based on our discussions, I think your assumptions are:
> 
> 1. remoteproc will load virtio_console when remoteproc is required
> 2. virtio_console will never be loaded by itself
> 3. General virtio_console functionality (including tty and multiport)
> is never used when remoteproc is used
> 
> I think at least 3 needs more thought/justification why it's a valid
> assumption.  Documenting it in the commit msg is fine.
> 
> At least assumptions 1 and 2 will cause remoteproc to not function
> correctly with virtio_console, despite both of them being loaded
> (because they can be loaded in the unexpected order -- virtio_console
> before remoteproc).  Do you want to adjust the code so that
> remoteproc
> queries for already-existing virtio_console.ko, triggers the code
> that
> would otherwise be not triggered in virtcons_probe(), and makes
> remoteproc functional in that case?

... I just saw that virtcons_probe() doesn't have any setup in case of
rproc.  So this last paragraph doesn't apply.

So maybe just adding some notes in the commit log about why this will
end up working, and why rproc usage and tty+multiport usage are
mutually exclusive (and fine) will help.

Thanks,

		Amit

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

[Amit Shah]

On Mon, 2025-02-17 at 11:59 +0100, Amit Shah wrote:
> On Mon, 2025-02-17 at 11:53 +0100, Amit Shah wrote:
> > On Fri, 2025-02-14 at 18:47 +0100, Uwe Kleine-König wrote:
> > > On Fri, Feb 14, 2025 at 05:55:41PM +0100, Amit Shah wrote:
> > > > On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> > > > > Hello Amit,
> > > > > 
> > > > > On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > > > > > 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.
> > > > > 
> > > > > Where does the interesting thing happen if remoteproc is
> > > > > already
> > > > > loaded
> > > > > at that time? I'm not seeing anything interesting in that
> > > > > case
> > > > > either
> > > > > ...
> > > > 
> > > > The code I pointed to,
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> > > > 
> > > > either enables remoteproc if the module is present; or it
> > > > enables
> > > > multiport, but not both at the same time.  If remoteproc isn't
> > > > present
> > > > when this probe routine is executed, multiport might get
> > > > enabled. 
> > > > And
> > > > then there's no chance for remoteproc to get enabled.
> > > 
> > > The only case where there is a difference between IS_REACHABLE
> > > and
> > > IS_ENABLED is:
> > > 
> > > 	CONFIG_REMOTEPROC=m
> > > 	CONFIG_VIRTIO_CONSOLE=y
> > 
> > Well, also if CONFIG_VIRTIO_CONSOLE=m; and virtio_console.ko is
> > loaded
> > before remoteproc.ko.
> > 
> > > Iff in this case you never want to test for MULTIPORT (even
> > > though
> > > the
> > > remoteproc module might never get loaded), then my patch is
> > > wrong.
> > > 
> > > When creating the patch I thought there is a hard dependency on
> > > remoteproc (like calling a function that is provided by
> > > CONFIG_REMOTEPROC). I don't understand how the remoteproc stuff
> > > interacts with the virtio_console driver, is this something in
> > > userspace
> > > which then would also autoload the remoteproc module?
> > 
> > What's happening is that multiport and remoteproc are mutually
> > exclusive in the virtio_console code.
> > 
> > And, I'm also not sure of how remoteproc loads and configures
> > itself.
> > Does loading remoteproc cause a load of virtio_console?  How?
> > 
> > So - based on our discussions, I think your assumptions are:
> > 
> > 1. remoteproc will load virtio_console when remoteproc is required
> > 2. virtio_console will never be loaded by itself
> > 3. General virtio_console functionality (including tty and
> > multiport)
> > is never used when remoteproc is used
> > 
> > I think at least 3 needs more thought/justification why it's a
> > valid
> > assumption.  Documenting it in the commit msg is fine.
> > 
> > At least assumptions 1 and 2 will cause remoteproc to not function
> > correctly with virtio_console, despite both of them being loaded
> > (because they can be loaded in the unexpected order --
> > virtio_console
> > before remoteproc).  Do you want to adjust the code so that
> > remoteproc
> > queries for already-existing virtio_console.ko, triggers the code
> > that
> > would otherwise be not triggered in virtcons_probe(), and makes
> > remoteproc functional in that case?
> 
> ... I just saw that virtcons_probe() doesn't have any setup in case
> of
> rproc.  So this last paragraph doesn't apply.

... I knew I was missing something yesterday. There's the call to
add_port() from virtcons_probe() that sets up the port for remoteproc.
So that part will be entirely skipped in case the virtcons probe
happens before remoteproc is loaded.

There's much too confusion here, better to start anew.

> So maybe just adding some notes in the commit log about why this will
> end up working, and why rproc usage and tty+multiport usage are
> mutually exclusive (and fine) will help.
> 
> Thanks,
> 
> 		Amit

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Mon, Feb 17, 2025 at 11:53:01AM +0100, Amit Shah wrote:
> On Fri, 2025-02-14 at 18:47 +0100, Uwe Kleine-König wrote:
> > On Fri, Feb 14, 2025 at 05:55:41PM +0100, Amit Shah wrote:
> > > On Fri, 2025-02-14 at 17:52 +0100, Uwe Kleine-König wrote:
> > > > Hello Amit,
> > > > 
> > > > On Fri, Feb 14, 2025 at 05:37:52PM +0100, Amit Shah wrote:
> > > > > 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.
> > > > 
> > > > Where does the interesting thing happen if remoteproc is already
> > > > loaded
> > > > at that time? I'm not seeing anything interesting in that case
> > > > either
> > > > ...
> > > 
> > > The code I pointed to,
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/char/virtio_console.c#n1993
> > > 
> > > either enables remoteproc if the module is present; or it enables
> > > multiport, but not both at the same time.  If remoteproc isn't
> > > present
> > > when this probe routine is executed, multiport might get enabled. 
> > > And
> > > then there's no chance for remoteproc to get enabled.
> > 
> > The only case where there is a difference between IS_REACHABLE and
> > IS_ENABLED is:
> > 
> > 	CONFIG_REMOTEPROC=m
> > 	CONFIG_VIRTIO_CONSOLE=y
> 
> Well, also if CONFIG_VIRTIO_CONSOLE=m; and virtio_console.ko is loaded
> before remoteproc.ko.

There is nothing about module load ordering in my patch. What
virtio_console does or does not doesn't depend on the remoteproc module
being loaded or not. It only depends on .config.

This is true for both IS_ENABLED() and IS_REACHABLE() which both
evaluate to a constant known at compile time.

And already now it can happen that the virtio_console init code runs
before remoteproc is properly loaded.

Having said that your mail just confuses me more than it helps.

The problem I thought there is and that made me propose my patch doesn't
exist. So I suggest we just drop the patch and the discussion.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]