Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions | Date | Tue, 29 Aug 2023 14:29:51 +0206 |
| |
On 2023-08-10, Petr Mladek <pmladek@suse.com> wrote: >> +static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe) >> +{ >> + struct console *con = ctxt->console; >> + struct nbcon_state cur; >> + struct nbcon_state new; >> + >> + nbcon_state_read(con, &cur); >> + >> + /* The unsafe bit must not be cleared if @hostile_unsafe is set. */ >> + if (!unsafe && cur.hostile_unsafe) >> + return nbcon_context_can_proceed(ctxt, &cur); >> + >> + do { >> + if (!nbcon_context_can_proceed(ctxt, &cur)) >> + return false; > > nbcon_context_can_proceed() returns "true" even when there > is a pending request. It happens when the current state is "unsafe".
Correct.
>> + >> + new.atom = cur.atom; >> + new.unsafe = unsafe; >> + } while (!nbcon_state_try_cmpxchg(con, &cur, &new)); > > If the new state is "safe" and there is a pending request > then we should release the lock and return false here.
The function does release the lock and return false... after it clears state.unsafe.
> It does not make sense to block the waiter just to realize > that we can't enter "unsafe" state again.
Sorry, I do not understand what you mean.
>> + ctxt->unsafe = unsafe; >> + >> + return true; > > An easy solution would be to do here: > > ctxt->unsafe = unsafe; > return nbcon_context_can_proceed(ctxt, &cur);
ctxt->unsafe is an input. It does not need to be updated anyway. (And besides, it will be removed for v3.)
> But maybe, we can change the logic a bit. Something like:
I think with ctxt->unsafe removed, this v2 implementation is ok.
John
| |