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 <[email protected]> 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/[email protected]?part=3
