diff mbox series

[printk,v2,05/38] printk: use console_is_enabled()

Message ID 20221019145600.1282823-6-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Oct. 19, 2022, 2:55 p.m. UTC 
  Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Petr Mladek Oct. 21, 2022, 9:25 a.m. UTC | #1 
On Wed 2022-10-19 17:01:27, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  kernel/printk/printk.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index e8a56056cd50..7ff2fc75fc3b 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2658,7 +2658,7 @@ static bool abandon_console_lock_in_panic(void)
>   */
>  static inline bool console_is_usable(struct console *con)
>  {
> -	if (!(con->flags & CON_ENABLED))
> +	if (!console_is_enabled(con))
>  		return false;

This allows to use console_is_usable() without synchronization against
modification of the state. I guess that it will be needed for the
printk kthreads. But it is not needed at the moment.

We should document why it is needed and why it is safe.


>  
>  	if (!con->write)
> @@ -2944,7 +2944,7 @@ void console_unblank(void)
>  	console_locked = 1;
>  	console_may_schedule = 0;
>  	for_each_console(c)
> -		if ((c->flags & CON_ENABLED) && c->unblank)
> +		if (console_is_enabled(c) && c->unblank)
>  			c->unblank();
>  	console_unlock();

The reading of the flag is actually synchronized against modifications
here. I guess that we need this because c->unblank() probably is not safe
against other operations with the console, e.g. c->write().

Well, it probably does not matter if reading of CON_ENABLED flag is
serialized against modifications. So, it is perfectly fine to use
the "racy" function.


> @@ -3098,7 +3098,7 @@ static int try_enable_preferred_console(struct console *newcon,
>  	 * without matching. Accept the pre-enabled consoles only when match()
>  	 * and setup() had a chance to be called.
>  	 */
> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
> +	if (console_is_enabled(newcon) && (c->user_specified == user_specified))

This is not racy because newcon is not in console_list at this
point. So that the state can't be modified by another callers.

Anyway, it is not explicitly synchronized, so we need to use
console_is_enabled with data_race() annotation.


>  		return 0;
>  
>  	return -ENOENT;

Best Regards,
Petr
John Ogness Oct. 31, 2022, 3:39 p.m. UTC | #2 
On 2022-10-21, Petr Mladek <pmladek@suse.com> wrote:
>>  static inline bool console_is_usable(struct console *con)
>>  {
>> -	if (!(con->flags & CON_ENABLED))
>> +	if (!console_is_enabled(con))
>>  		return false;
>
> This allows to use console_is_usable() without synchronization against
> modification of the state. I guess that it will be needed for the
> printk kthreads. But it is not needed at the moment.

It will be needed once console_lock is no longer providing the
synchronization for console->flags (later in _this_ series).

I will add to the commit message that this is a preparatory change for
when console_lock no longer provides this synchronization.

>> @@ -2944,7 +2944,7 @@ void console_unblank(void)
>>  	console_locked = 1;
>>  	console_may_schedule = 0;
>>  	for_each_console(c)
>> -		if ((c->flags & CON_ENABLED) && c->unblank)
>> +		if (console_is_enabled(c) && c->unblank)
>>  			c->unblank();
>>  	console_unlock();
>
> The reading of the flag is actually synchronized against modifications
> here. I guess that we need this because c->unblank() probably is not safe
> against other operations with the console, e.g. c->write().

Again, we will need it later in this series when holding console_lock
does not provide the necessary synchronization.

>> @@ -3098,7 +3098,7 @@ static int try_enable_preferred_console(struct console *newcon,
>>  	 * without matching. Accept the pre-enabled consoles only when match()
>>  	 * and setup() had a chance to be called.
>>  	 */
>> -	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
>> +	if (console_is_enabled(newcon) && (c->user_specified == user_specified))
>
> This is not racy because newcon is not in console_list at this
> point. So that the state can't be modified by another callers.
>
> Anyway, it is not explicitly synchronized, so we need to use
> console_is_enabled with data_race() annotation.

For this case, it makes sense to _not_ use console_is_enabled(). This
code will be synchronized against writes even after console_lock has
been relieved of this responsibility.

Originally I wanted to replace _all_ checks with console_is_enabled(),
but since synchronization is rare, I'll keep the manual checks for those
(and add a comment that it is not a data race).

John
diff mbox series

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e8a56056cd50..7ff2fc75fc3b 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2658,7 +2658,7 @@  static bool abandon_console_lock_in_panic(void)
  */
 static inline bool console_is_usable(struct console *con)
 {
-	if (!(con->flags & CON_ENABLED))
+	if (!console_is_enabled(con))
 		return false;
 
 	if (!con->write)
@@ -2944,7 +2944,7 @@  void console_unblank(void)
 	console_locked = 1;
 	console_may_schedule = 0;
 	for_each_console(c)
-		if ((c->flags & CON_ENABLED) && c->unblank)
+		if (console_is_enabled(c) && c->unblank)
 			c->unblank();
 	console_unlock();
 
@@ -3098,7 +3098,7 @@  static int try_enable_preferred_console(struct console *newcon,
 	 * without matching. Accept the pre-enabled consoles only when match()
 	 * and setup() had a chance to be called.
 	 */
-	if (newcon->flags & CON_ENABLED && c->user_specified ==	user_specified)
+	if (console_is_enabled(newcon) && (c->user_specified == user_specified))
 		return 0;
 
 	return -ENOENT;