Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
Signed-off-by: John Ogness <[email protected]>
---
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)) &&
(!opt || !opt[0] || strcmp(con->name, opt) == 0))
break;
}
--
2.30.2
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 <[email protected]>
Reviewed-by: Greg Kroah-Hartman <[email protected]>
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 <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
Hi,
On Wed, Oct 19, 2022 at 7:56 AM John Ogness <[email protected]> wrote:
>
> Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
>
> Signed-off-by: John Ogness <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> Hi,
>
> On Wed, Oct 19, 2022 at 7:56 AM John Ogness <[email protected]> wrote:
> >
> > Replace (console->flags & CON_ENABLED) usage with console_is_enabled().
> >
> > Signed-off-by: John Ogness <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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