Re: [PATCH v1 4/5] dmaengine: cix-sky1-dma350: Add Sky1 integration driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Race condition allocating DMA buffers due to delayed reserved memory attachment.
- [High] Infinite probe deferral if the child 'arm,dma-350' device node is disabled.
- [Medium] Improper mixing of pulse and level reset semantics leads to ignored errors and potential power-off failures.
- [Medium] Leaking partially populated child platform devices on `of_platform_populate` error path.
--

commit a340fbf6d0923e08511995628a9b9b856250a6c8
Author: Jelly Jia [off-list ref]

dmaengine: cix-sky1-dma350: Add Sky1 integration driver

Add a CIX Sky1 integration driver for Arm DMA-350 instances. The driver
manages clocks, reset, optional interrupt routing, optional reserved
memory attachment, and populates the child Arm DMA-350 controller.

[ ... ]

+static int cix_sky1_dma350_enable_resources(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_bulk_prepare_enable(data->num_clks, data->clks);
+	if (ret)
+		return ret;
+
+	ret = reset_control_reset(data->reset);
+	if (ret)
+		goto err_disable_clks;

[ ... ]

+static void cix_sky1_dma350_disable_resources(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+
+	reset_control_assert(data->reset);

[Severity: Medium]
Is it safe to mix pulse and level reset semantics here?

The enable path uses reset_control_reset() for a pulse, but the disable path
uses reset_control_assert() for a level assertion.

If the underlying reset controller only implements the .reset callback
(pulse), reset_control_assert() will return -ENOTSUPP. Because the return
value is ignored, won't this silently fail to reset the hardware during
unbind or system suspend, leaving it active?

+	clk_bulk_disable_unprepare(data->num_clks, data->clks);
+}
+
+static int cix_sky1_dma350_attach_reserved_mem(struct device *dev)
+{
+	struct cix_sky1_dma350 *data = dev_get_drvdata(dev);
+	struct platform_device *child_pdev;
+	struct device_node *child_np;
+	int ret;
+
+	if (!of_property_present(dev->of_node, "memory-region"))
+		return 0;
+
+	child_np = of_get_compatible_child(dev->of_node, "arm,dma-350");
+	if (!child_np)
+		return -ENODEV;
+
+	child_pdev = of_find_device_by_node(child_np);
+	of_node_put(child_np);
+	if (!child_pdev)
+		return -EPROBE_DEFER;

[Severity: High]
Can this lead to an infinite probe deferral loop if the child device node is
disabled in the device tree?

of_get_compatible_child() does not check the node's availability, so it will
return the child node even if it has status = "disabled". Because
of_platform_populate() correctly skips disabled nodes, no device will be
instantiated.

As a result, of_find_device_by_node() will return NULL, causing the driver
to continually return -EPROBE_DEFER instead of ignoring the disabled device.

+
+	/*
+	 * Reserved memory is attached after the child has probed. This relies
+	 * on arm-dma350 not allocating coherent command buffers in probe;
+	 * those allocations happen per descriptor at prep time.
+	 */
+	ret = of_reserved_mem_device_init_by_idx(&child_pdev->dev,
+						 dev->of_node, 0);

[Severity: High]
Is there a race condition with allocating DMA buffers here?

of_platform_populate() in the probe function synchronously instantiates and
probes the child device, exposing it to the rest of the system.

Could a concurrent DMA client request a channel and prepare a transfer before
cix_sky1_dma350_attach_reserved_mem() executes? If that happens, allocations
might draw from the default system pool instead of the designated reserved
region, potentially causing hardware faults.

+	if (ret) {
+		put_device(&child_pdev->dev);
+		return ret;
+	}

[ ... ]

+static int cix_sky1_dma350_probe(struct platform_device *pdev)
+{

[ ... ]

+	ret = cix_sky1_dma350_enable_resources(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable resources\n");
+
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret)
+		goto err_disable_resources;

[Severity: Medium]
Does this leak partially populated child platform devices?

If of_platform_populate() encounters an error while instantiating multiple
children, it breaks and returns an error without cleaning up the ones it
already created.

Bypassing err_depopulate here prevents cleanup of platform devices created
prior to the failure, permanently leaking them.

+
+	ret = cix_sky1_dma350_attach_reserved_mem(dev);
+	if (ret)
+		goto err_depopulate;
+
+	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907033404.596839-1-Jelly.Jia@cixtech.com?part=4