[printk,v2,12/38] tty: serial: kgdboc: use console_is_enabled()

  Replace (console->flags & CON_ENABLED) usage with console_is_enabled().

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 drivers/tty/serial/kgdboc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

On Wed, Oct 19, 2022 at 05:01:34PM +0206, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
  

On Wed 2022-10-19 17:01:34, John Ogness wrote:
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
  

Hi,

On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
>
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  drivers/tty/serial/kgdboc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index e76f0186c335..b17aa7e49894 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -533,7 +533,7 @@ static int __init kgdboc_earlycon_init(char *opt)
>         console_lock();
>         for_each_console(con) {
>                 if (con->write && con->read &&
> -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> +                   (console_is_enabled(con) || (con->flags & CON_BOOT)) &&

<shrug>. I guess this is OK, but it feels a little pointless. If we're
still directly looking at the CON_BOOT bit in con->flags it seems
weird to be accessing CON_ENABLED through a special wrapper that's
marked as a `data_race`. In our case it's _not_ a data race, right,
since this function continues to hold the console_lock() even at the
end of the series? I personally would drop this patch but if you
really want it I won't object.

-Doug
  

Hi,

On Mon, Oct 24, 2022 at 3:46 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Oct 19, 2022 at 7:56 AM John Ogness <john.ogness@linutronix.de> wrote:
> >
> > Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> >
> > Signed-off-by: John Ogness <john.ogness@linutronix.de>
> > ---
> >  drivers/tty/serial/kgdboc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index e76f0186c335..b17aa7e49894 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -533,7 +533,7 @@ static int __init kgdboc_earlycon_init(char *opt)
> >         console_lock();
> >         for_each_console(con) {
> >                 if (con->write && con->read &&
> > -                   (con->flags & (CON_BOOT | CON_ENABLED)) &&
> > +                   (console_is_enabled(con) || (con->flags & CON_BOOT)) &&
>
> <shrug>. I guess this is OK, but it feels a little pointless. If we're
> still directly looking at the CON_BOOT bit in con->flags it seems
> weird to be accessing CON_ENABLED through a special wrapper that's
> marked as a `data_race`. In our case it's _not_ a data race, right,
> since this function continues to hold the console_lock() even at the
> end of the series? I personally would drop this patch but if you
> really want it I won't object.

I realized that my statement isn't quite true. It actually only holds
console_list_lock() even at the end of the series. Still, it seems
weird that we're declaring the `data_race` on CON_ENABLED but not
CON_BOOT ?
  

On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> It actually only holds console_list_lock() even at the end of the
> series. Still, it seems weird that we're declaring the `data_race` on
> CON_ENABLED but not CON_BOOT ?

You are correct that it is not a data race (because of the
console_list_lock being held.) Petr has suggested adding a separate
function for non-data-race callers. For v3 I will do this and use it
here, probably called console_is_enabled_locked().

Usually CON_ENABLED is the only flag that is interesting during normal
operation. The kgdboc case is an exception. I thought about if we should
have console_flags() and console_flags_locked() to be able to handle
general con->flags access. console_flags() would be marked with
data_race(), console_flags_locked() would use lockdep to ensure the
console_list_lock is held.

But I would also like to have the _is_enabled special variant because
how we check if a console is enabled will be different for the atomic
consoles. I would like to hide those details in the _is_enabled
implementation.

John Ogness
  

On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> It actually only holds console_list_lock() even at the end of the
> series. Still, it seems weird that we're declaring the `data_race` on
> CON_ENABLED but not CON_BOOT ?

For my upcoming v3 I decided to drop this patch and will keep the
existing direct reading of @flags. Instead of this patch, for v3 the
comment will additionally mention why @flags is allowed to be directly
read:

/*
 * Hold the console_lock to guarantee that no consoles are
 * unregistered until the kgdboc_earlycon setup is complete.
 * Trapping the exit() callback relies on exit() not being
 * called until the trap is setup. This also allows safe
 * traversal of the console list and race-free reading of @flags.
 */

John Ogness
  

On Fri 2022-11-04 17:29:15, John Ogness wrote:
> On 2022-10-24, Doug Anderson <dianders@chromium.org> wrote:
> > It actually only holds console_list_lock() even at the end of the
> > series. Still, it seems weird that we're declaring the `data_race` on
> > CON_ENABLED but not CON_BOOT ?
> 
> For my upcoming v3 I decided to drop this patch and will keep the
> existing direct reading of @flags. Instead of this patch, for v3 the
> comment will additionally mention why @flags is allowed to be directly
> read:
> 
> /*
>  * Hold the console_lock to guarantee that no consoles are
              ^^^^^^^^^^^^
>  * unregistered until the kgdboc_earlycon setup is complete.

My understanding is that this is synchronized by console_list_lock.
Or do I miss something?

>  * Trapping the exit() callback relies on exit() not being
>  * called until the trap is setup. This also allows safe
>  * traversal of the console list and race-free reading of @flags.
>  */

Best Regards,
Petr
  

On 2022-11-07, Petr Mladek <pmladek@suse.com> wrote:
>> /*
>>  * Hold the console_lock to guarantee that no consoles are
>               ^^^^^^^^^^^^
>>  * unregistered until the kgdboc_earlycon setup is complete.
>
> My understanding is that this is synchronized by console_list_lock.
> Or do I miss something?

Yes, in the end the comment will say console_list_lock. At this point in
the series, the console_lock is still used. The comment is updated later
(as in patch 34 of the v2 series).

John