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;
Message-ID: <85683284-db85-7e3a-57bd-750e1c204e3e@igalia.com>
Date:   Thu, 1 Sep 2022 14:46:48 -0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH V3] firmware: google: Test spinlock on panic path to avoid
 lockups
Content-Language: en-US
To:     Greg KH <gregkh@linuxfoundation.org>, evgreen@chromium.org
Cc:     arnd@arndb.de, linux-efi@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel@gpiccoli.net, ardb@kernel.org,
        davidgow@google.com, jwerner@chromium.org,
        Petr Mladek <pmladek@suse.com>
References: <20220819155059.451674-1-gpiccoli@igalia.com>
 <YxDVPqVkdgQbAIvY@kroah.com>
 <f89cd87c-7d1c-d8e6-ed95-6876f0201872@igalia.com>
 <YxDX9+p+58q2sip2@kroah.com>
 <6bc5dbc3-2cdd-5cb8-1632-11de2008a85a@igalia.com>
 <YxDhiSDs4YcUrqV5@kroah.com>
From:   "Guilherme G. Piccoli" <gpiccoli@igalia.com>
In-Reply-To: <YxDhiSDs4YcUrqV5@kroah.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 01/09/2022 13:44, Greg KH wrote:
> [...]
>>> How are we supposed to know this here?
>>>
>>
>> Reading the code?
>> Or you mean, in the commit description this should be mentioned?
> 
> Yes, and in the comment as this type of call is very rare and should
> almost never be used.

OK, I can add that, for sure.


>> [...]
>> I don't think it is so simple - we are in the panic path.
> 
> Great, then the lock doesn't matter :)
> 
>> So, imagine the lock was taken in CPU0, where GSMI is doing some
>> operation. During that operation, CPU1 panics!
>>
>> When that happens, panic() executes in CPU1, disabling CPU0 through
>> "strong" mechanisms (NMI). So, CPU0 had the lock, it is now off, and
>> when CPU1 goes through the panic notifiers, it'll eventually wait
>> forever for this lock in the GSMI handler, unless we have this patch
>> that would prevent the handler to run in such case.
>> Makes sense?
> 
> I'm trying to say "if you are in panic, never grab the lock in the first
> place".  So change the place when you grab the lock, not here.
> 

Evan, any comment here?
I think the patch is still well suited for this case. Suggestions on how
to improve it are welcome, of course.

I honestly didn't understand exactly what you're suggesting Greg...
Mind clarifying?

Cheers,


Guilherme