Re: [PATCH] drm/edid/firmware: stop using throwaway platform device

[Matthieu CHARETTE <matthieu.charette@gmail.com>]

Hi,

I've tested the patch and I can confirm that it fixed the issue.
Tested on Fedora 36 with kernel 6.0.8.

Thanks,
Matthieu

On Tue, Nov 8 2022 at 04:40:52 PM +0100, Matthieu CHARETTE 
<matthieu.charette@gmail.com> wrote:
> I didn't test the patch yet. I will do. But even without testing I 
> can tell you that it will work (It will not crash).
> Currently when the crash occurs, all screens remain black after 
> resume. I'm not able to login with ssh neither. And logs end before 
> the suspend. So the crash seems to be some kind of kernel panic.
> 
> Matthieu
> 
> On Tue, Nov 8 2022 at 01:27:33 PM +0200, Jani Nikula 
> <jani.nikula@intel.com> wrote:
>> On Sun, 06 Nov 2022, Matthieu CHARETTE <matthieu.charette@gmail.com> 
>> \x7fwrote:
>>>  Hi,
>>> 
>>>  Can you tell me what are we waiting for? Maybe I can help.
>> 
>> Have you tried the patch? Is it an improvement over the status quo?
>> 
>> The "crash" is still ambiguous to me. Do you observe it with the 
>> \x7fpatch?
>> Do you have logs? Etc.
>> 
>> BR,
>> Jani.
>> 
>> 
>>> 
>>>  Thanks.
>>> 
>>>  Matthieu
>>> 
>>>  On Wed, Oct 12 2022 at 07:16:29 PM +0200, Matthieu CHARETTE
>>>  <matthieu.charette@gmail.com> wrote:
>>>>  By crash, I mean that an error is returned here:
>>>>  
>>>> \x7f\x7f\x7fLINKIFYEFGabGCFEcabCEEfeCECDAcaebFGeICGHEAADGDb
>>>>  I don't really know what happens next, but on my machine the 
>>>> \x7f\x7f\x7fbuilt-in
>>>>  screen and the external remains dark. Also the kernel seems to
>>>>  freeze. I suspect a kernel panic, but I'm not sure. Anyway, the 
>>>> \x7f\x7f\x7ferror
>>>>  is definitely not well handled, and a fix would be great.
>>>>  Also, request_firmware() will crash if called for the first time 
>>>> on
>>>>  the resume path because the file system isn't reachable on the 
>>>> \x7f\x7f\x7fresume
>>>>  process. And no cache is available for this firmware. So I guess 
>>>> \x7f\x7f\x7fthat
>>>>  in this case, request_firmware() returns an error.
>>>>  Suspend-plug-resume case is not my priority nether as long as it
>>>>  doesn't make the system crash (Which is currently the case).
>>>> 
>>>>  On Wed, Oct 12 2022 at 11:25:59 AM +0300, Jani Nikula
>>>>  <jani.nikula@intel.com> wrote:
>>>>>  On Tue, 11 Oct 2022, Matthieu CHARETTE 
>>>>> \x7f\x7f\x7f\x7f<matthieu.charette@gmail.com>
>>>>>  \x7fwrote:
>>>>>>   Currently the EDID is requested during the resume. But since 
>>>>>> \x7f\x7f\x7f\x7f\x7fit's
>>>>>>   requested too early, this means before the filesystem is 
>>>>>> \x7f\x7f\x7f\x7f\x7fmounted,
>>>>>>  \x7f\x7fthe
>>>>>>   firmware request fails. This make the DRM driver crash when
>>>>>>  \x7f\x7fresuming.
>>>>>>   This kind of issue should be prevented by the firmware caching
>>>>>>  \x7f\x7fprocess
>>>>>>   which cache every firmware requested for the next resume. But
>>>>>>  since \x7f\x7fwe
>>>>>>   are using a temporary device, the firmware isn't cached on 
>>>>>> \x7f\x7f\x7f\x7f\x7fsuspend
>>>>>>   since the device doesn't work anymore.
>>>>>>   When using a non temporary device to get the EDID, the firmware
>>>>>>  will
>>>>>>   be cached on suspend for the next resume. So requesting the
>>>>>>  firmware
>>>>>>   during resume will succeed.
>>>>>>   But if the firmware has never been requested since the boot, 
>>>>>> \x7f\x7f\x7f\x7f\x7fthis
>>>>>>   means that the monitor isn't plugged since the boot. The kernel
>>>>>>  will
>>>>>>   not be caching the EDID. So if we plug the monitor while the
>>>>>>  machine
>>>>>>   is suspended. The resume will fail to load the firmware. And 
>>>>>> the
>>>>>>  DRM
>>>>>>   driver will crash.
>>>>>>   So basically, your fix should solve the issue except for the 
>>>>>> \x7f\x7f\x7f\x7f\x7fcase
>>>>>>   where the monitor hasn't been plugged since boot and is plugged
>>>>>>  \x7f\x7fwhile
>>>>>>   the machine is suspended.
>>>>>>   I hope I was clear. Tell me if I wasn't. I'm not really good at
>>>>>>  \x7f\x7fexplaining.
>>>>> 
>>>>>  That was a pretty good explanation. The only thing I'm missing is
>>>>>  what
>>>>>  the failure mode is exactly when you claim the driver will 
>>>>> crash. \x7f\x7f\x7f\x7fWhy
>>>>>  would request_firmware() "crash" if called for the first time on 
>>>>> \x7f\x7f\x7f\x7fthe
>>>>>  resume path?
>>>>> 
>>>>>  I'm not sure I care much about not being able to load the 
>>>>> firmware
>>>>>  \x7fEDID
>>>>>  in the suspend-plug-resume case (as this can be remedied with a
>>>>>  subsequent modeset), but obviously any errors need to be handled
>>>>>  gracefully, without crashing.
>>>>> 
>>>>>  BR,
>>>>>  Jani.
>>>>> 
>>>>> 
>>>>>  --
>>>>>  Jani Nikula, Intel Open Source Graphics Center
>>>> 
>>>> 
>>> 
>>> 
>> 
>> --
>> Jani Nikula, Intel Open Source Graphics Center
> 
>