On 2023-09-15 01:20, Aravind Vijayakumar wrote:
On 9/8/2023 5:21 AM, Robin Murphy wrote:
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@xxxxxxxxxxx> wrote:
On 8/16/2023 6:01 PM, Rob Clark wrote:
On Wed, Aug 16, 2023 at 3:55 PM Aravind Vijayakumar
<quic_aprasann@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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.
Hi Rob and Robin,
With the NULL check and return -ENODATA, we are getting the dma-api
managed iommu domain, and reattach the device to iommu domain by calling
"of_dma_configure" from the driver probe, but without the NULL check it
will be a kernel crash.
Um, what? Drivers should absolutely not be calling of_dma_configure() on
their own devices, that is a massively egregious abuse of the API.
Also, with the NULL check we don't have to update the
qcom_smmu_client_of_match and we can use the existing function -
"qcom_adreno_smmu_init_context" itself.
So making a bunch of conceptually-wrong code changes "saves" making the
correct 1-line data change for the mechanism to work as designed? That
hardly sounds like a good justification to me :/
Thanks,
Robin.
