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

[Rob Clark]

On Mon, Aug 28, 2023 at 2:35 PM Aravind Vijayakumar
<quic_aprasann at 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 at 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 at 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


> 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
> >>