Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   John Ogness <john.ogness@linutronix.de>
To:     Jiri Slaby <jirislaby@kernel.org>, gregkh@linuxfoundation.org
Cc:     linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org,
        Vlastimil Babka <vbabka@suse.cz>,
        Thomas Gleixner <tglx@linutronix.de>,
        Petr Mladek <pmladek@suse.com>
Subject: Re: [PATCH] serial: 8250: drop lockdep annotation from
 serial8250_clear_IER()
In-Reply-To: <7d8ae4f8-8900-5a06-5b7b-d4a3aea0673e@kernel.org>
References: <20230811064340.13400-1-jirislaby@kernel.org>
 <878rae175n.fsf@jogness.linutronix.de>
 <7d8ae4f8-8900-5a06-5b7b-d4a3aea0673e@kernel.org>
Date:   Mon, 14 Aug 2023 10:21:14 +0206
Message-ID: <87bkfa6nvx.fsf@jogness.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Hi Jiri,

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

On 2023-08-14, Jiri Slaby <jirislaby@kernel.org> 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