Re: [PATCH] serial: 8250: drop lockdep annotation from serial8250_clear_IER()

[John Ogness]

Hi Jiri,

Thanks for the follow-up. You responded faster than I could correct
myself.

On 2023-08-14, Jiri Slaby <jirislaby@xxxxxxxxxx> wrote:
>>> The port lock is not always held when calling serial8250_clear_IER().
>>> When an oops is in progress, the lock is tried to be taken and when it
>>> is not, a warning is issued:
>> 
>> Yes, and that is a potential deadlock. The warning is correct.
>
> Could you elaborate on how can not-taking a lock be a potential
> deadlock?

I was wrong to say deadlock. The lockdep annotation is about interrupts
being unintentionally left permanently disabled or being enabled while
another CPU is transmitting.

>>> Therefore, remove the annotation as it doesn't hold for all invocations.
>> 
>> ... because those invocations are broken by design.
>
> Perhaps. But the system is crashing. Better to emit something without 
> the lock rather than nothing (and wait for the lock infinitely).

I am not suggesting to wait infinitely. I am merely pointing out that
the lockdep warning is legitimate.

>>> The other option would be to make the lockdep test conditional on
>>> 'oops_in_progress'

Actually I find this suggestion more appropriate. It makes it clear that
we are willing to take such risks and do not want to see the warnings in
a panic situation. However, I would end up having to revert that change
as well, so it really does not matter to me at this point. Either way I
will be reverting this patch.

>> The proper thing to do is to fix the invocation. The upcoming atomic
>> console implementation for the 8250 does exactly that.
>
> So what does it do?

The upcoming atomic consoles use a new type of synchronization to guard
the IER register (priority-based spinning with timeouts). This allows us
to make intelligent decisions about how and when to flush in a panic,
rather than simply ignorning locks and hoping for the best.

>> If this patch gets accepted (which it appears it will be), I will revert
>> it in my series implementing the 8250 atomic console.
>
> That's fine as soon as the warning is not a problem.

Yes, I am also fine with re-introducing the annotation together with the
8250 series.

John