On 8/17/26 17:41, Konrad Dybcio wrote:
> On 8/8/26 5:47 AM, Paul Hollinsky wrote:
>> Two commits from the "firmware: qcom: Add OP-TEE PAS service support"
>> series break the GPU and the modem on SC7180 Chromebooks (trogdor):
>>
>> f3b1357673dd ("remoteproc: qcom_q6v5_mss: Switch to generic PAS TZ APIs")
>> 0be72be03ca7 ("drm/msm: Switch to generic PAS TZ APIs")
>>
>> Both are in linux-next as of next-20260805 and neither is in a released
>> kernel yet, so there is still time to fix this before v7.3.
>
> First of all, thank you for the amazingly thorough write-up. I've been on
> holiday so I couldn't look into it quicker.
>
> [...]
>
>> Suggested fix, drm/msm
>> ======================
>>
>> Given the above, I think the right fix is to move the availability check
>> behind the DT discovery rather than restore the SCM check. Boards with no
>> zap-shader node then never consult PAS at all, and boards that do have one
>> keep the qcom_pas_is_available() gate the original patch was going for:
>>
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -52,6 +52,12 @@
>> return -ENODEV;
>> }
>>
>> + /* We need PAS to be able to load the firmware */
>> + if (!qcom_pas_is_available()) {
>> + DRM_DEV_ERROR(dev, "PAS is not available\n");
>> + return -EPROBE_DEFER;
>> + }
>> +
>> ret = of_reserved_mem_region_to_resource(np, 0, &r);
>> if (ret) {
>> zap_available = false;
>> @@ -170,18 +176,11 @@
>> int adreno_zap_shader_load(struct msm_gpu *gpu, u32 pasid)
>> {
>> struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> - struct platform_device *pdev = gpu->pdev;
>>
>> /* Short cut if we determine the zap shader isn't available/needed */
>> if (!zap_available)
>> return -ENODEV;
>>
>> - /* We need PAS to be able to load the firmware */
>> - if (!qcom_pas_is_available()) {
>> - DRM_DEV_ERROR(&pdev->dev, "PAS is not available\n");
>> - return -EPROBE_DEFER;
>> - }
>> -
>> return zap_shader_load_mdt(gpu, adreno_gpu->info->zapfw, pasid);
>> }
>
> I believe this is the right fix
Just wondering: who will submit the proper fix to ideally fix this
before the culprit hits mainline (as Linus afaik does not like it much
if changes that are know to cause regressions in -next enter his tree)
-- or at least before -rc1 is out?
The culprit afaics was authored by Sumit Garg, whose last msg on Lore is
from 260731 -- which indicates a well deserved vacation or something
like that, so waiting for Sumit to act might take too long.
Ciao, Thorsten
>> I have this booting here: the GPU initialises and logs "Zap shader not
>> enabled - using SECVID_TRUST_CNTL instead", as it did before the
>> regression.
>>
>>
>> Suggested fix, remoteproc
>> =========================
>>
>> Keep an SCM gate for need_mem_protection and add a separate PAS gate for
>> need_pas_mem_setup. This is what I did to get it working:
>>
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -2079,7 +2079,16 @@ static int q6v5_probe(struct platform_device *pdev)
>> if (!desc)
>> return -EINVAL;
>>
>> - if (desc->need_mem_protection && !qcom_pas_is_available())
>> + /*
>> + * Memory protection is done through qcom_scm_assign_mem(), which needs
>> + * SCM but not PAS. Only the memory setup path issues PAS calls, so
>> + * requiring PAS for every need_mem_protection platform prevents the
>> + * modem from probing at all on TZ firmware that offers no PAS.
>> + */
>> + if (desc->need_mem_protection && !qcom_scm_is_available())
>> + return -EPROBE_DEFER;
>> +
>> + if (desc->need_pas_mem_setup && !qcom_pas_is_available())
>> return -EPROBE_DEFER;
>
> And likewise this looks like the correct fix too, feel free to submit
> both as patches
>
> [...]
>
>> On the API contract
>> ===================
>>
>> For what it is worth, the kernel-doc for qcom_pas_is_available()
>> (qcom_pas.c:256) reads:
>>
>> Note that it is mandatory for any PAS client to invoke this API.
>> If it returns true then only any other PAS API can be invoked.
>>
>> That is a guard to call before invoking a PAS API, not a statement that a
>> driver touching TZ at all should refuse to probe without PAS.
>> ipa_main.c:764 consults it only on the loader path that genuinely needs
>> PAS rather than as a blanket probe precondition, which is the shape both
>> fixes above are aiming for.
>
> This is a sloppiness that got through the transition patchset.. previously
> as you mentioned, qcom_scm_is_available() was a blanket "can we talk to TZ
> yet"?, and as the conversion happened, it was largely just find-and-replaced..
>
> Konrad
>
