Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
Signed-off-by: John Ogness <[email protected]>
---
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;
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;
--
2.30.2
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 <[email protected]>
> ---
> 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
On 2022-10-21, Petr Mladek <[email protected]> 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