Re: [PATCH] iommu/arm-smmu-qcom: NULL pointer check for driver data

[Robin Murphy <robin.murphy@arm.com>]

On 2023-09-08 06:17, Aravind Vijayakumar wrote:
> 
> On 8/29/2023 7:30 AM, Rob Clark wrote:
>> On Mon, Aug 28, 2023 at 2:35 PM Aravind Vijayakumar
>> <quic_aprasann@quicinc.com> wrote:
>>>
>>> On 8/16/2023 6:01 PM, Rob Clark wrote:
>>>> On Wed, Aug 16, 2023 at 3:55 PM Aravind Vijayakumar
>>>> <quic_aprasann@quicinc.com> wrote:
>>>>> The driver_data is NULL when qcom_adreno_smmu_init_context()
>>>>> is called before the dev_set_drvdata() from the client driver
>>>>> and is resulting in kernel crash.
>>>>>
>>>>> So add a null pointer check to handle the scenario
>>>>> where the client driver for the GPU SMMU device would
>>>>> be setting the driver data after the smmu client device
>>>>> probe is done and not necessarily before that. The function
>>>>> qcom_adreno_smmu_init_context() assumes that the client
>>>>> driver always set the driver data using dev_set_drvdata()
>>>>> before the smmu client device probe, but this assumption
>>>>> is not always true.
>>>>>
>>>>> Signed-off-by: Aravind Vijayakumar <quic_aprasann@quicinc.com>
>>>>> ---
>>>>>    drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 3 +++
>>>>>    1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> index c71afda79d64..5323f82264ca 100644
>>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>>> @@ -231,6 +231,9 @@ static int qcom_adreno_smmu_init_context(struct 
>>>>> arm_smmu_domain *smmu_domain,
>>>>>            */
>>>>>
>>>>>           priv = dev_get_drvdata(dev);
>>>>> +       if (!priv)
>>>>> +               return 0;
>>>> could this -EPROBE_DEFER instead, or something like that?  I think you
>>>> patch as proposed would result in per-process gpu pgtables silently
>>>> failing
>>>>
>>>> BR,
>>>> -R
>>> Thanks for the review comments. Returning -EPROBE_DEFER wont work
>>> because the probe of the client driver (which sets the driver data) will
>>> never get triggered. However, the probe of the client driver succeeds if
>>> we return -ENODATA. would that be acceptable?
>> I _think_ so.. I need to page back in the sequence of how this works,
>> but I do have some warn_on's in drm/msm to complain loudly if we don't
>> get per-process pgtables.  I'd be interested to see the callstack
>> where you hit this issue.  From what I remember the sequence should
>> be:
>>
>> 1) before the client dev probes, arm-smmu probes and attaches the
>> dma-api managed iommu_domain (which IIRC should be an identity domain,
>> and is otherwise unused).. at this point drvdata is NULL
>> 2) the drm/msm can probe
>> 3) at some point later when GPU fw is avail the GPU is loaded, drvdata
>> is set, and we start creating and attaching the iommu_domain's that
>> are actually used (one for kernel context and one each for userspace
>> processes using the GPU
>>
>> I guess maybe if you are hitting this case of NULL drvdata, then you
>> aren't getting an identity context for the dma-api managed
>> iommu_domain?
>>
>> BR,
>> -R
>>
> Yes, there are some warn_ons in io-pgtable.c, which have helped a lot 
> during debugging. The following is the call stack when we are hitting 
> the issue:
> 
>    qcom_adreno_smmu_init_context+0x28/0x100
>    arm_smmu_init_domain_context+0x1fc/0x4cc
>    arm_smmu_attach_dev+0x7c/0x410
>    __iommu_attach_device+0x28/0x110
>    iommu_probe_device+0x98/0x144
>    of_iommu_configure+0x1f0/0x278
>    of_dma_configure_id+0x15c/0x320
>    platform_dma_configure+0x24/0x90
>    really_probe+0x138/0x39c
>    __driver_probe_device+0x114/0x190
>    device_driver_attach+0x4c/0xac
>    bind_store+0xb8/0x110

OK, so it looks like you are indeed getting a non-identity default 
domain as Rob suspected. I guess that means qcom_smmu_client_of_match 
needs updating for this platform? (In which case, maybe a WARN() here to 
point in that direction might be handy as well?)

Thanks,
Robin.

> 
> This is the call stack during platform_driver_register() , if there is 
> no NULL check then the initial probe crashes, if there is NULL check, 
> instead of crashing, the really_probe returns and we can call 
> of_dma_configure again from the driver probe after setting the driver 
> data. Please let me know if there is any concerns?
> 
> Regards,
> 
> Aravind
> 
>>> Regards,
>>>
>>> Aravind
>>>
>>>>> +
>>>>>           priv->cookie = smmu_domain;
>>>>>           priv->get_ttbr1_cfg = qcom_adreno_smmu_get_ttbr1_cfg;
>>>>>           priv->set_ttbr0_cfg = qcom_adreno_smmu_set_ttbr0_cfg;
>>>>> -- 
>>>>> 2.40.1
>>>>>