[RESEND][PATCH 1/2] console: add new CONSOLE_ACTIVATE_ALL_FALLBACK option

[RESEND][PATCH 1/2] console: add new CONSOLE_ACTIVATE_ALL_FALLBACK option

[Ahmad Fatoum]

We already have three CONSOLE_ACTIVATE options and every one of them has
drawbacks:

  - ACTIVATE_ALL: May write barebox log to external devices like MCUs
    that don't expect it

  - ACTIVATE_FIRST: Not applicable for most systems that probe from
    device tree, where the order of probe is not necessarily fixed,
    so what console is first may change over updates

  - ACTIVATE_NONE: has a misleading name and may leave the user without
    any consoles at all if nothing else activates a console

Let's add a new option and make it the default, which avoids all these
issues: Like ACTIVATE_NONE, it expects board code, DT or environment to
enable a console and if none of them do it falls back to activating all
consoles, so the user isn't kept in the dark with an error instructing
the user to fix this.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
Sent out second patch only by mistake instead of both...
---
 common/Kconfig   | 19 ++++++++++++++++++-
 common/console.c | 27 +++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/common/Kconfig b/common/Kconfig
index 31360892aeef..2dda5ce5743a 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -812,7 +812,7 @@ endchoice
 choice
 	prompt "Console activation strategy"
 	depends on CONSOLE_FULL
-	default CONSOLE_ACTIVATE_FIRST
+	default CONSOLE_ACTIVATE_ALL_FALLBACK
 
 config CONSOLE_ACTIVATE_FIRST
 	bool
@@ -831,6 +831,23 @@ config CONSOLE_ACTIVATE_ALL
 	  Only the first registered console will have the full startup
 	  log though.
 
+config CONSOLE_ACTIVATE_ALL_FALLBACK
+	bool
+	prompt "activate all consoles as fallback"
+	help
+	  This option is similar to CONFIG_CONSOLE_ACTIVATE_NONE in that it
+	  leaves consoles disabled on startup. If by the end of barebox
+	  startup, no consoles have been activated via board code, device
+	  tree or environment, barebox will enable all registered consoles
+	  as fallback, so the user has a chance to see output.
+
+	  This will be indicated by a fat error, so the user knows that
+	  the configuration needs to be fixed. If you don't see any
+	  output at all, consider trying again after enabling
+	  CONFIG_CONSOLE_ACTIVATE_ALL, so consoles are activated immediately
+	  at registration time and/or with CONFIG_DEBUG_LL, so barebox output
+	  is written even before console drivers were registered.
+
 config CONSOLE_ACTIVATE_NONE
 	prompt "leave all consoles disabled"
 	bool
diff --git a/common/console.c b/common/console.c
index 73b4c4d4db01..e83a3e1e2d7f 100644
--- a/common/console.c
+++ b/common/console.c
@@ -450,6 +450,33 @@ int console_unregister(struct console_device *cdev)
 }
 EXPORT_SYMBOL(console_unregister);
 
+static int console_activate_all_fallback(void)
+{
+	int activate = CONSOLE_STDIOE;
+	struct console_device *cdev;
+
+	for_each_console(cdev) {
+		if (cdev->f_active & (CONSOLE_STDOUT | CONSOLE_STDERR))
+			return 0;
+	}
+
+	if (IS_ENABLED(CONFIG_CONSOLE_DISABLE_INPUT))
+		activate &= ~CONSOLE_STDIN;
+
+	for_each_console(cdev)
+		console_set_active(cdev, activate);
+
+	/*
+	 * This is last resort, so the user is not kept in the dark.
+	 * Writing to all consoles is a bad idea as the devices at the
+	 * other side might get confused by it, thus the error log level.
+	 */
+	pr_err("No consoles were activated. Activating all consoles as fallback!\n");
+
+	return 0;
+}
+postenvironment_initcall(console_activate_all_fallback);
+
 static int getc_raw(void)
 {
 	struct console_device *cdev;
-- 
2.39.2

[RESEND][PATCH 2/2] common: clarify help text for CONFIG_CONSOLE_ACTIVATE_* options

[Ahmad Fatoum]

CONFIG_CONSOLE_ACTIVATE_NONE is an unfortunate name. The help text
explains how consoles are still activated, but omits the main way they
are activated: The device tree's /chosen/stdout-path property.

Similarly, CONFIG_CONSOLE_ACTIVATE_(FIRST/NONE) don't talk about the
implications of selecting them, so reword the help texts to better
inform the user.

Reported-by: Casey Reeves <contact@xogium.me>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/common/Kconfig b/common/Kconfig
index 2dda5ce5743a..fea26262da86 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -818,9 +818,10 @@ config CONSOLE_ACTIVATE_FIRST
 	bool
 	prompt "activate first console on startup"
 	help
-	  Normally on startup all consoles are disabled, so you won't
-	  see anything from barebox starting. Enabling this option
-	  enables the first console.
+	  Select this to activate the first registered console.
+	  On many systems, devices are probed in no strict order,
+	  so you probably should say n here if you are probing
+	  from device tree and have more than one console.
 
 config CONSOLE_ACTIVATE_ALL
 	bool
@@ -831,6 +832,10 @@ config CONSOLE_ACTIVATE_ALL
 	  Only the first registered console will have the full startup
 	  log though.
 
+	  If you have consoles that aren't meant for barebox log output
+	  (e.g. a connected MCU), you will want to say n here and use
+	  CONFIG_ACTIVATE_ALL_FALLBACK or CONFIG_CONSOLE_ACTIVATE_NONE instead.
+
 config CONSOLE_ACTIVATE_ALL_FALLBACK
 	bool
 	prompt "activate all consoles as fallback"
@@ -852,9 +857,10 @@ config CONSOLE_ACTIVATE_NONE
 	prompt "leave all consoles disabled"
 	bool
 	help
-	  Leave all consoles disabled on startup. Board code or environment
-	  is responsible for enabling a console. Otherwise you'll get a working
-	  barebox, you just won't see anything.
+	  Leave all consoles disabled on startup. Board code, environment
+	  or the device tree /chosen/stdout-path property will be responsible
+	  for enabling a console. Otherwise you'll get a working barebox,
+	  you just won't see anything.
 
 endchoice
 
-- 
2.39.2

Re: [RESEND][PATCH 1/2] console: add new CONSOLE_ACTIVATE_ALL_FALLBACK option

[Sascha Hauer]

On Fri, Jul 26, 2024 at 02:26:22PM +0200, Ahmad Fatoum wrote:
> We already have three CONSOLE_ACTIVATE options and every one of them has
> drawbacks:
> 
>   - ACTIVATE_ALL: May write barebox log to external devices like MCUs
>     that don't expect it
> 
>   - ACTIVATE_FIRST: Not applicable for most systems that probe from
>     device tree, where the order of probe is not necessarily fixed,
>     so what console is first may change over updates
> 
>   - ACTIVATE_NONE: has a misleading name and may leave the user without
>     any consoles at all if nothing else activates a console
> 
> Let's add a new option and make it the default, which avoids all these
> issues: Like ACTIVATE_NONE, it expects board code, DT or environment to
> enable a console and if none of them do it falls back to activating all
> consoles, so the user isn't kept in the dark with an error instructing
> the user to fix this.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
> Sent out second patch only by mistake instead of both...
> ---
>  common/Kconfig   | 19 ++++++++++++++++++-
>  common/console.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 31360892aeef..2dda5ce5743a 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -812,7 +812,7 @@ endchoice
>  choice
>  	prompt "Console activation strategy"
>  	depends on CONSOLE_FULL
> -	default CONSOLE_ACTIVATE_FIRST
> +	default CONSOLE_ACTIVATE_ALL_FALLBACK
>  
>  config CONSOLE_ACTIVATE_FIRST
>  	bool
> @@ -831,6 +831,23 @@ config CONSOLE_ACTIVATE_ALL
>  	  Only the first registered console will have the full startup
>  	  log though.
>  
> +config CONSOLE_ACTIVATE_ALL_FALLBACK
> +	bool
> +	prompt "activate all consoles as fallback"
> +	help
> +	  This option is similar to CONFIG_CONSOLE_ACTIVATE_NONE in that it
> +	  leaves consoles disabled on startup. If by the end of barebox
> +	  startup, no consoles have been activated via board code, device
> +	  tree or environment, barebox will enable all registered consoles
> +	  as fallback, so the user has a chance to see output.
> +
> +	  This will be indicated by a fat error, so the user knows that
> +	  the configuration needs to be fixed. If you don't see any
> +	  output at all, consider trying again after enabling
> +	  CONFIG_CONSOLE_ACTIVATE_ALL, so consoles are activated immediately
> +	  at registration time and/or with CONFIG_DEBUG_LL, so barebox output
> +	  is written even before console drivers were registered.
> +
>  config CONSOLE_ACTIVATE_NONE
>  	prompt "leave all consoles disabled"
>  	bool
> diff --git a/common/console.c b/common/console.c
> index 73b4c4d4db01..e83a3e1e2d7f 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -450,6 +450,33 @@ int console_unregister(struct console_device *cdev)
>  }
>  EXPORT_SYMBOL(console_unregister);
>  
> +static int console_activate_all_fallback(void)
> +{
> +	int activate = CONSOLE_STDIOE;
> +	struct console_device *cdev;
> +
> +	for_each_console(cdev) {
> +		if (cdev->f_active & (CONSOLE_STDOUT | CONSOLE_STDERR))
> +			return 0;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_CONSOLE_DISABLE_INPUT))
> +		activate &= ~CONSOLE_STDIN;
> +
> +	for_each_console(cdev)
> +		console_set_active(cdev, activate);
> +
> +	/*
> +	 * This is last resort, so the user is not kept in the dark.
> +	 * Writing to all consoles is a bad idea as the devices at the
> +	 * other side might get confused by it, thus the error log level.
> +	 */
> +	pr_err("No consoles were activated. Activating all consoles as fallback!\n");
> +
> +	return 0;
> +}
> +postenvironment_initcall(console_activate_all_fallback);

Shouldn't this code only run when CONSOLE_ACTIVATE_ALL_FALLBACK is
enabled?

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RESEND][PATCH 1/2] console: add new CONSOLE_ACTIVATE_ALL_FALLBACK option

[Ahmad Fatoum]

On 7/30/24 10:09, Sascha Hauer wrote:
> On Fri, Jul 26, 2024 at 02:26:22PM +0200, Ahmad Fatoum wrote:
>> We already have three CONSOLE_ACTIVATE options and every one of them has
>> drawbacks:
>>
>>   - ACTIVATE_ALL: May write barebox log to external devices like MCUs
>>     that don't expect it
>>
>>   - ACTIVATE_FIRST: Not applicable for most systems that probe from
>>     device tree, where the order of probe is not necessarily fixed,
>>     so what console is first may change over updates
>>
>>   - ACTIVATE_NONE: has a misleading name and may leave the user without
>>     any consoles at all if nothing else activates a console
>>
>> Let's add a new option and make it the default, which avoids all these
>> issues: Like ACTIVATE_NONE, it expects board code, DT or environment to
>> enable a console and if none of them do it falls back to activating all
>> consoles, so the user isn't kept in the dark with an error instructing
>> the user to fix this.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>> Sent out second patch only by mistake instead of both...
>> ---
>>  common/Kconfig   | 19 ++++++++++++++++++-
>>  common/console.c | 27 +++++++++++++++++++++++++++
>>  2 files changed, 45 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index 31360892aeef..2dda5ce5743a 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -812,7 +812,7 @@ endchoice
>>  choice
>>  	prompt "Console activation strategy"
>>  	depends on CONSOLE_FULL
>> -	default CONSOLE_ACTIVATE_FIRST
>> +	default CONSOLE_ACTIVATE_ALL_FALLBACK
>>  
>>  config CONSOLE_ACTIVATE_FIRST
>>  	bool
>> @@ -831,6 +831,23 @@ config CONSOLE_ACTIVATE_ALL
>>  	  Only the first registered console will have the full startup
>>  	  log though.
>>  
>> +config CONSOLE_ACTIVATE_ALL_FALLBACK
>> +	bool
>> +	prompt "activate all consoles as fallback"
>> +	help
>> +	  This option is similar to CONFIG_CONSOLE_ACTIVATE_NONE in that it
>> +	  leaves consoles disabled on startup. If by the end of barebox
>> +	  startup, no consoles have been activated via board code, device
>> +	  tree or environment, barebox will enable all registered consoles
>> +	  as fallback, so the user has a chance to see output.
>> +
>> +	  This will be indicated by a fat error, so the user knows that
>> +	  the configuration needs to be fixed. If you don't see any
>> +	  output at all, consider trying again after enabling
>> +	  CONFIG_CONSOLE_ACTIVATE_ALL, so consoles are activated immediately
>> +	  at registration time and/or with CONFIG_DEBUG_LL, so barebox output
>> +	  is written even before console drivers were registered.
>> +
>>  config CONSOLE_ACTIVATE_NONE
>>  	prompt "leave all consoles disabled"
>>  	bool
>> diff --git a/common/console.c b/common/console.c
>> index 73b4c4d4db01..e83a3e1e2d7f 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -450,6 +450,33 @@ int console_unregister(struct console_device *cdev)
>>  }
>>  EXPORT_SYMBOL(console_unregister);
>>  
>> +static int console_activate_all_fallback(void)
>> +{
>> +	int activate = CONSOLE_STDIOE;
>> +	struct console_device *cdev;
>> +
>> +	for_each_console(cdev) {
>> +		if (cdev->f_active & (CONSOLE_STDOUT | CONSOLE_STDERR))
>> +			return 0;
>> +	}
>> +
>> +	if (IS_ENABLED(CONFIG_CONSOLE_DISABLE_INPUT))
>> +		activate &= ~CONSOLE_STDIN;
>> +
>> +	for_each_console(cdev)
>> +		console_set_active(cdev, activate);
>> +
>> +	/*
>> +	 * This is last resort, so the user is not kept in the dark.
>> +	 * Writing to all consoles is a bad idea as the devices at the
>> +	 * other side might get confused by it, thus the error log level.
>> +	 */
>> +	pr_err("No consoles were activated. Activating all consoles as fallback!\n");
>> +
>> +	return 0;
>> +}
>> +postenvironment_initcall(console_activate_all_fallback);
> 
> Shouldn't this code only run when CONSOLE_ACTIVATE_ALL_FALLBACK is
> enabled?

Of course, sorry about that. Just sent out v3.

> 
> Sascha
>