Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 3 Jun 2020 10:40:51 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Dmitry Safonov <0x7f454c46@gmail.com>
Cc:     Andy Shevchenko <andy.shevchenko@gmail.com>,
        Johan Hovold <johan@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        "open list:SERIAL DRIVERS" <linux-serial@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        stable <stable@vger.kernel.org>
Subject: Re: [PATCH 2/4] serial: core: fix broken sysrq port unlock
Message-ID: <20200603084051.GJ19480@localhost>
References: <20200602140058.3656-1-johan@kernel.org>
 <20200602140058.3656-3-johan@kernel.org>
 <CAHp75VeXYn46wQ5EXkk_MOQ49ybtyTeoQS6BS1X9DkC6hbeF-w@mail.gmail.com>
 <b016ad68-124a-5c98-f49b-f7286d995223@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b016ad68-124a-5c98-f49b-f7286d995223@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jun 02, 2020 at 04:34:16PM +0100, Dmitry Safonov wrote:
> On 6/2/20 3:48 PM, Andy Shevchenko wrote:
> > On Tue, Jun 2, 2020 at 5:03 PM Johan Hovold <johan@kernel.org> wrote:
> >>
> >> Commit d6e1935819db ("serial: core: Allow processing sysrq at port
> >> unlock time") worked around a circular locking dependency by adding
> >> helpers used to defer sysrq processing to when the port lock was
> >> released.
> >>
> >> A later commit unfortunately converted these inline helpers to exported
> >> functions despite the fact that the unlock helper was restoring irq
> >> flags, something which needs to be done in the same function that saved
> >> them (e.g. on SPARC).
> > 
> > I'm not familiar with sparc, can you elaborate a bit what is ABI /
> > architecture lock implementation background?
> 
> I remember that was a limitation a while ago to save/restore flags from
> the same function. Though, I vaguely remember the reason.
> I don't see this limitation in Documentation/*

It's described in both LDD3 and LKD, which is possibly where I first
picked it up too (admittedly a long time ago).

> Google suggests that it's related to storage location:
> https://stackoverflow.com/a/34279032

No, that was never the issue.

SPARC includes the current register window in those flags, which at
least had to be restored in the same stack frame.

> Looking into arch/sparc I also can't catch if it's still a limitation.

Yeah, looking closer at the current implementation it seems this is no
longer an issue on SPARC.

> Also, looking around, xa_unlock_irqrestore() is called not from the same
> function. Maybe this issue is in history?

xa_unlock_irqrestore() is just a macro for spin_unlock_irqsave() it
seems, so not a counter example.

> Also, some comments would be nice near functions in the header.

Agreed. Let me respin this and either merge this with the next patch or
at least amend the commit message.

Johan