Re: [PATCH v4 03/25] accel/qda: Add compute context bank devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] accel/qda: missing IOMMU probe and domain ownership in qda_create_cb_device()
- [Medium] accel/qda: qda_create_cb_device() leaves coherent_dma_mask at 32 bits
--

commit 3c742ed6e8932c7c7fe69686db2d02383880697a
Author: Ekansh Gupta <ekansh.gupta@oss.qualcomm.com>

accel/qda: Add compute context bank devices

This commit creates a platform device for each qcom,fastrpc-compute-cb
child node during rpmsg probe and configures its DMA address mask based on
per-SoC data.

> diff --git a/drivers/accel/qda/qda_cb.c b/drivers/accel/qda/qda_cb.c
> new file mode 100644
> index 0000000000000..afb0bb71a1f44
> --- /dev/null
> +++ b/drivers/accel/qda/qda_cb.c
> @@ -0,0 +1,76 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> +#include <linux/dma-mapping.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +#include <drm/drm_print.h>
> +#include "qda_cb.h"
> +#include "qda_drv.h"
> +#include "qda_soc.h"
> +
> +static int qda_create_cb_device(struct qda_dev *qdev, struct device_node *cb_node)
> +{
> +	struct qda_cb_dev *entry;
> +	struct platform_device *pdev;
> +	u32 dma_bits;
> +	int ret;
> +
> +	pdev = of_platform_device_create(cb_node, NULL, qdev->dev);
> +	if (!pdev) {
> +		drm_err(&qdev->drm_dev, "Failed to create CB device for %pOFn\n", cb_node);
> +		return -ENODEV;
> +	}

[Severity: High]
Can qda_create_cb_device() succeed here even when the context bank device has
not been attached to an IOMMU domain or claimed for kernel DMA API use?

Because the iommus property lives on the qcom,fastrpc-compute-cb child nodes
rather than the parent fastrpc rpmsg node, fw_devlink only creates a
DL_FLAG_SYNC_STATE_ONLY proxy link on rpdev->dev, which
device_links_check_suppliers() ignores when qda_rpmsg_probe() runs.

When of_platform_device_create() calls of_device_add() -> device_add(),
bus_notify(dev, BUS_NOTIFY_ADD_DEVICE) ignores the return value of
iommu_bus_notifier(), and iommu_init_device() ignores the return value of
dev->bus->dma_configure(dev):

drivers/iommu/iommu.c:iommu_init_device() {
	if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
		mutex_unlock(&iommu_probe_device_lock);
		dev->bus->dma_configure(dev);
		mutex_lock(&iommu_probe_device_lock);
		/* If another instance finished the job for us, skip it */
		if (!dev->iommu || dev->iommu_group)
			return -ENODEV;
	}
}

If the SMMU supplier has not probed yet, of_iommu_configure() returns
-EPROBE_DEFER, causing of_dma_configure_id() to return early before
arch_setup_dma_ops() sets dev->dma_coherent:

drivers/of/device.c:of_dma_configure_id() {
	ret = of_iommu_configure(dev, np, id);
	if (ret == -EPROBE_DEFER) {
		/* Don't touch range map if it wasn't set from a valid dma-ranges */
		if (set_map)
			dev->dma_range_map = NULL;
		kfree(map);
		return -EPROBE_DEFER;
	}
}

In the platform bus model, DMA and IOMMU configuration errors (such as
-EPROBE_DEFER or SMMU stream mapping failures) are only propagated when a
driver binds in really_probe() -> platform_dma_configure(). Since no platform
driver binds to pdev, of_platform_device_create() still returns a valid pdev
with pdev->dev.iommu_group == NULL, allowing the DRM device to be registered
while subsequent session DMA allocations fall back to unisolated dma-direct
physical mappings.

Additionally, when platform_dma_configure() is invoked from
iommu_init_device() with drv == NULL, it returns before calling
iommu_device_use_default_domain():

drivers/base/platform.c:platform_dma_configure() {
	/* @dev->driver may not be valid when we're called from the IOMMU layer */
	if (ret || !drv || to_platform_driver(drv)->driver_managed_dma)
		return ret;

	ret = iommu_device_use_default_domain(dev);
}

This leaves group->owner_cnt at 0, so kernel DMA API ownership of the context
bank's IOMMU group is never claimed (unlike drivers/misc/fastrpc.c, which
binds a platform driver to qcom,fastrpc-compute-cb devices). Should a
component/sub-driver bind to these context bank platform devices, or should
qda_create_cb_device() explicitly configure DMA and claim default domain
ownership while propagating errors such as -EPROBE_DEFER?

> +
> +	dma_bits = qda_soc_dma_addr_bits(qdev);
> +	ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));

[Severity: Medium]
Should qda_create_cb_device() call dma_set_mask_and_coherent() instead of
dma_set_mask() here?

When of_platform_device_create() allocates the device through
of_platform_device_create_pdata() -> of_device_alloc() ->
platform_device_alloc() -> setup_pdev_dma_masks(), pdev->dev.dma_mask is
pointed at pdev->platform_dma_mask while pdev->dev.coherent_dma_mask is
initialized separately to DMA_BIT_MASK(32):

drivers/base/platform.c:setup_pdev_dma_masks() {
	if (!pdev->dev.coherent_dma_mask)
		pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
	if (!pdev->dev.dma_mask) {
		pdev->platform_dma_mask = DMA_BIT_MASK(32);
		pdev->dev.dma_mask = &pdev->platform_dma_mask;
	}
}

Because dev->dev.dma_mask is already non-NULL when
of_platform_device_create_pdata() checks if (!dev->dev.dma_mask),
dev->dev.dma_mask and dev->dev.coherent_dma_mask remain separate fields.

Calling dma_set_mask() only updates *pdev->dev.dma_mask
(pdev->platform_dma_mask) to 34 bits on Kaanapali CDSP and leaves
pdev->dev.coherent_dma_mask at DMA_BIT_MASK(32). When later patches allocate
context bank buffers via dma_alloc_coherent() (in commit "accel/qda: Add
buffer allocation for the DSP"), the IOMMU DMA allocator passes
dev->coherent_dma_mask as the upper IOVA limit to iommu_dma_alloc_iova(),
restricting coherent allocations on 34-bit CDSP context banks to 32-bit
IOVAs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-v4-v4-0-88f2f24a5f0b@qti.qualcomm.com?part=3