Re: [PATCH v1 3/4] acpi/x86: s2idle: call screen on and off as part of callbacks

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/19/2024 15:45, Antheas Kapenekakis wrote:
>> It seems like it would mostly be a programmer error if it was called twice.
>>
>> How about something like this:
>>
>> if (unlikely(WARN_ON(lsp0_dsm_in_screen_off)))
>>          return -EINVAL;
>>
>> You could do something similar with the inverse in the other call site
>> too then.
> 
> Indeed, under normal operation calling it twice is an error.
> 
> 2 concerns here: if I make this change just here, the code on patch 2
> will make the suspend sequence bail. Is this a desired outcome?
> 
> Shouldn't we just keep going? And if we should keep going, does that
> include calling Display Off twice?
> 
> I am inclined to say the desired outcome is to not call Display Off
> twice and to keep the suspend sequence going. In which case, I will
> merge your suggestion and remove the bail sequence from patch 2.

OK.  We'll have to see Rafael's thoughts on your next version.

FYI, he's at LPC, so expect delayed response.

> 
> 2nd concern is that I suspect it is very likely firmware vendors will
> want to call this sequence as part of developing devices, so I am
> inclined to allow calling the functions back to back. However, that
> could be achieved differently, e.g., with a module flag.

I think vendors developing devices could rebuild the kernel too.  I'm 
not really sure this would be a common enough need to justify a module 
parameter.

> 
>>> +     lsp0_dsm_in_screen_off = false;
>>
>> Doesn't it initialize to false already?  Is this really needed?
> 
> More for my peace of mind. I will remove it.
> 
> Antheas