<div><br></div><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 11, 2022 at 4:37 PM Joel Fernandes <<a href="mailto:joel@joelfernandes.org">joel@joelfernandes.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
> On Nov 11, 2022, at 4:28 PM, Akhil P Oommen <<a href="mailto:quic_akhilpo@quicinc.com" target="_blank">quic_akhilpo@quicinc.com</a>> wrote:<br>
> <br>
> On 11/12/2022 1:19 AM, Joel Fernandes (Google) wrote:<br>
>> Even though the GPU is shut down, during kexec reboot we can have userspace<br>
>> still running. This is especially true if KEXEC_JUMP is not enabled, because we<br>
>> do not freeze userspace in this case.<br>
>> <br>
>> To prevent crashes, track that the GPU is shutdown and prevent get_param() from<br>
>> accessing GPU resources if we find it shutdown.<br>
>> <br>
>> This fixes the following crash during kexec reboot on an ARM64 device with adreno GPU:<br>
>> <br>
>> [ 292.534314] Kernel panic - not syncing: Asynchronous SError Interrupt<br>
>> [ 292.534323] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)<br>
>> [ 292.534326] Call trace:<br>
>> [ 292.534328] dump_backtrace+0x0/0x1d4<br>
>> [ 292.534337] show_stack+0x20/0x2c<br>
>> [ 292.534342] dump_stack_lvl+0x60/0x78<br>
>> [ 292.534347] dump_stack+0x18/0x38<br>
>> [ 292.534352] panic+0x148/0x3b0<br>
>> [ 292.534357] nmi_panic+0x80/0x94<br>
>> [ 292.534364] arm64_serror_panic+0x70/0x7c<br>
>> [ 292.534369] do_serror+0x0/0x7c<br>
>> [ 292.534372] do_serror+0x54/0x7c<br>
>> [ 292.534377] el1h_64_error_handler+0x34/0x4c<br>
>> [ 292.534381] el1h_64_error+0x7c/0x80<br>
>> [ 292.534386] el1_interrupt+0x20/0x58<br>
>> [ 292.534389] el1h_64_irq_handler+0x18/0x24<br>
>> [ 292.534395] el1h_64_irq+0x7c/0x80<br>
>> [ 292.534399] local_daif_inherit+0x10/0x18<br>
>> [ 292.534405] el1h_64_sync_handler+0x48/0xb4<br>
>> [ 292.534410] el1h_64_sync+0x7c/0x80<br>
>> [ 292.534414] a6xx_gmu_set_oob+0xbc/0x1fc<br>
>> [ 292.534422] a6xx_get_timestamp+0x40/0xb4<br>
>> [ 292.534426] adreno_get_param+0x12c/0x1e0<br>
>> [ 292.534433] msm_ioctl_get_param+0x64/0x70<br>
>> [ 292.534440] drm_ioctl_kernel+0xe8/0x158<br>
>> [ 292.534448] drm_ioctl+0x208/0x320<br>
>> [ 292.534453] __arm64_sys_ioctl+0x98/0xd0<br>
>> [ 292.534461] invoke_syscall+0x4c/0x118<br>
>> [ 292.534467] el0_svc_common+0x98/0x104<br>
>> [ 292.534473] do_el0_svc+0x30/0x80<br>
>> [ 292.534478] el0_svc+0x20/0x50<br>
>> [ 292.534481] el0t_64_sync_handler+0x78/0x108<br>
>> [ 292.534485] el0t_64_sync+0x1a4/0x1a8<br>
>> [ 292.534632] Kernel Offset: 0x1a5f800000 from 0xffffffc008000000<br>
>> [ 292.534635] PHYS_OFFSET: 0x80000000<br>
>> [ 292.534638] CPU features: 0x40018541,a3300e42<br>
>> [ 292.534644] Memory Limit: none<br>
>> <br>
>> Cc: Rob Clark <<a href="mailto:robdclark@chromium.org" target="_blank">robdclark@chromium.org</a>><br>
>> Cc: Steven Rostedt <<a href="mailto:rostedt@goodmis.org" target="_blank">rostedt@goodmis.org</a>><br>
>> Cc: Ricardo Ribalda <<a href="mailto:ribalda@chromium.org" target="_blank">ribalda@chromium.org</a>><br>
>> Cc: Ross Zwisler <<a href="mailto:zwisler@kernel.org" target="_blank">zwisler@kernel.org</a>><br>
>> Signed-off-by: Joel Fernandes (Google) <<a href="mailto:joel@joelfernandes.org" target="_blank">joel@joelfernandes.org</a>><br>
>> ---<br>
>> drivers/gpu/drm/msm/adreno/adreno_device.c | 1 +<br>
>> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-<br>
>> drivers/gpu/drm/msm/msm_gpu.h | 3 +++<br>
>> 3 files changed, 5 insertions(+), 1 deletion(-)<br>
>> <br>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c<br>
>> index f0cff62812c3..03d912dc0130 100644<br>
>> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c<br>
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c<br>
>> @@ -612,6 +612,7 @@ static void adreno_shutdown(struct platform_device *pdev)<br>
>> {<br>
>> struct msm_gpu *gpu = dev_to_gpu(&pdev->dev);<br>
>> + gpu->is_shutdown = true;<br>
>> WARN_ON_ONCE(adreno_system_suspend(&pdev->dev));<br>
>> }<br>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c<br>
>> index 382fb7f9e497..6903c6892469 100644<br>
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c<br>
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c<br>
>> @@ -251,7 +251,7 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx,<br>
>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);<br>
>> /* No pointer params yet */<br>
>> - if (*len != 0)<br>
>> + if (*len != 0 || gpu->is_shutdown)<br>
>> return -EINVAL;<br>
> This will race with shutdown.<br>
<br>
Could you clarify what you mean? At this point in the code, the shutdown is completed and it crashes here.<br>
</blockquote><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto">Ok so I think you meant that if the shut down happens after we sample the is_shutdown, then we run into the same issue.</div><div dir="auto"><br></div><div dir="auto">I can’t reproduce that but I’ll look into that. Another way might be to synchronize using a mutex. Though maybe the shutdown path can wait for active pm_runtime references?</div><div dir="auto"><br></div><div dir="auto">Thanks.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> Probably, propagating back the return value of pm_runtime_get() in every possible ioctl call path is the right thing to do.<br>
<br>
Ok I’ll look into that. But the patch I posted works reliably and fixes all crashes we could reproduce.<br>
<br>
> I have never thought about this scenario. Do you know why userspace is not freezed before kexec?<br>
<br>
I am not sure. It depends on how kexec is used. The userspace freeze happens only when kexec is called to switch back and forth between different kernels (persistence mode). In such scenario I believe the userspace has to be frozen and unfrozen. However for normal kexec, that does not happen.<br>
<br>
Thanks.<br>
<br>
<br>
> <br>
> -Akhil.<br>
>> switch (param) {<br>
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h<br>
>> index ff911e7305ce..f18b0a91442b 100644<br>
>> --- a/drivers/gpu/drm/msm/msm_gpu.h<br>
>> +++ b/drivers/gpu/drm/msm/msm_gpu.h<br>
>> @@ -214,6 +214,9 @@ struct msm_gpu {<br>
>> /* does gpu need hw_init? */<br>
>> bool needs_hw_init;<br>
>> + /* is the GPU shutdown? */<br>
>> + bool is_shutdown;<br>
>> +<br>
>> /**<br>
>> * global_faults: number of GPU hangs not attributed to a particular<br>
>> * address space<br>
> <br>
</blockquote></div></div>