Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions

[John Ogness]

On 2023-08-10, Petr Mladek <pmladek@xxxxxxxx> 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