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

[sashiko-bot@kernel.org]

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 <Jelly.Jia@cixtech.com>

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.

> diff --git a/drivers/dma/cix-sky1-dma350.c b/drivers/dma/cix-sky1-dma350.c
> new file mode 100644
> index 0000000000000..5904453f34818
> --- /dev/null
> +++ b/drivers/dma/cix-sky1-dma350.c

[ ... ]

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