Re: [PATCH v7 12/19] drivers: soc: ti: k3-ringacc: handle absence of tisci

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The direct register fallback path in `k3_ringacc_ring_reconfig_qmode_sci` destroys the ring configuration by zeroing base addresses and sizes.
- [High] Uninitialized `cfg` and `intr` pointers for standalone rings lead to unconditional NULL pointer dereferences.
- [High] Driver probe fails unconditionally on platforms without TISCI firmware, completely preventing initialization.
- [High] The direct register configuration path silently truncates the upper 32 bits of the ring's DMA address to 4 bits.
- [Medium] Using `wmb()` is insufficient to flush posted MMIO writes when resetting the ring size.
- [Medium] Pointer arithmetic on a NULL pointer when assigning `tisci_ring_ops` on platforms without TISCI.
- [Medium] `k3_ringacc_ring_clear_irq` clears all interrupt status bits unconditionally, causing a TOCTOU race.
--

--- Patch [12]: [PATCH v7 12/19] drivers: soc: ti: k3-ringacc: handle absence of tisci ---
commit 8ac5aa2236fab24ddafc2ffcf45db7d49490cae1
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>

drivers: soc: ti: k3-ringacc: handle absence of tisci

Handle absence of tisci with direct register writes. This will support
platforms that do not have tisci firmware like AM62L.

> diff --git a/drivers/soc/ti/k3-ringacc.c b/drivers/soc/ti/k3-ringacc.c
> index 5966db4327b13..e81d19a9ed324 100644
> --- a/drivers/soc/ti/k3-ringacc.c
> +++ b/drivers/soc/ti/k3-ringacc.c
[ ... ]
> @@ -467,15 +518,31 @@ static void k3_ringacc_ring_reset_sci(struct k3_ring *ring)
>  	struct k3_ringacc *ringacc = ring->parent;
>  	int ret;
>  
> -	ring_cfg.nav_id = ringacc->tisci_dev_id;
> -	ring_cfg.index = ring->ring_id;
> -	ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_RING_COUNT_VALID;
> -	ring_cfg.count = ring->size;
> +	if (!ringacc->tisci) {
> +		u32 reg;
>  
> -	ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
> -	if (ret)
> -		dev_err(ringacc->dev, "TISCI reset ring fail (%d) ring_idx %d\n",
> -			ret, ring->ring_id);
> +		if (!ring->cfg)
> +			return;
> +
> +		reg = readl(&ring->cfg->size);
> +		reg &= ~K3_DMARING_CFG_SIZE_MASK;
> +		writel(reg, &ring->cfg->size);
> +
> +		/* Ensure the register clear operation completes before writing new value */
> +		wmb();
> +		reg |= ring->size;
> +		writel(reg, &ring->cfg->size);

[Severity: Medium]
Is wmb() sufficient to flush posted MMIO writes when resetting the ring
size?

While wmb() flushes CPU store buffers, it does not guarantee that the
interconnect has delivered the first write to the hardware peripheral before
the second is issued.

Would a dummy read-back of the register (e.g., readl(&ring->cfg->size)) be
necessary here to flush posted MMIO writes and ensure the hardware processes
the clear operation first?

[ ... ]
> @@ -501,10 +568,25 @@ static void k3_ringacc_ring_reconfig_qmode_sci(struct k3_ring *ring,
>  	ring_cfg.valid_params = TI_SCI_MSG_VALUE_RM_RING_MODE_VALID;
>  	ring_cfg.mode = mode;
>  
> -	ret = ringacc->tisci_ring_ops->set_cfg(ringacc->tisci, &ring_cfg);
> -	if (ret)
> -		dev_err(ringacc->dev, "TISCI reconf qmode fail (%d) ring_idx %d\n",
> -			ret, ring->ring_id);
> +	if (!ringacc->tisci) {
> +		u32 reg;
> +
> +		writel(ring_cfg.addr_lo, &ring->cfg->ba_lo);
> +		writel((ring_cfg.addr_hi & K3_DMARING_CFG_ADDR_HI_MASK) +
> +				(ring_cfg.asel << K3_DMARING_CFG_ASEL_SHIFT),
> +				&ring->cfg->ba_hi);
> +
> +		reg = readl(&ring->cfg->size);
> +		reg &= ~K3_DMARING_CFG_SIZE_MASK;
> +		reg |= ring_cfg.count & K3_DMARING_CFG_SIZE_MASK;
> +
> +		writel(reg, &ring->cfg->size);

[Severity: High]
Does this fallback path in k3_ringacc_ring_reconfig_qmode_sci() overwrite
the ring configuration with zeros?

Since ring_cfg is initialized to zero at the start of the function and only
the mode field is updated, writing ring_cfg.addr_lo, ring_cfg.addr_hi, and
ring_cfg.count to the hardware registers will write zeros.

Additionally, this direct register configuration branch does not seem to
write the intended mode parameter to the hardware.

[ ... ]
> @@ -670,15 +767,30 @@ int k3_ringacc_get_ring_irq_num(struct k3_ring *ring)
>  }
>  EXPORT_SYMBOL_GPL(k3_ringacc_get_ring_irq_num);
>  
> +u32 k3_ringacc_ring_get_irq_status(struct k3_ring *ring)
> +{
> +	struct k3_ringacc *ringacc = ring->parent;
> +	struct k3_ring *ring2 = &ringacc->rings[ring->ring_id];
> +
> +	return readl(&ring2->intr->status);
> +}
> +EXPORT_SYMBOL_GPL(k3_ringacc_ring_get_irq_status);
> +
> +void k3_ringacc_ring_clear_irq(struct k3_ring *ring)
> +{
> +	struct k3_ringacc *ringacc = ring->parent;
> +	struct k3_ring *ring2 = &ringacc->rings[ring->ring_id];
> +
> +	writel(0xFF, &ring2->intr->status);

[Severity: High]
Are the cfg and intr pointers initialized for standalone rings?

It appears that ring->cfg and ring->intr are only initialized for DMA rings
in k3_ringacc_dmarings_init(). For generic or standalone rings initialized in
k3_ringacc_init(), these remain NULL.

If invoked for a standalone ring on a non-TISCI platform, will these
unconditional dereferences in k3_ringacc_ring_get_irq_status() and
k3_ringacc_ring_clear_irq() lead to a kernel panic from a NULL pointer
dereference?

[Severity: Medium]
Will this unconditionally clear all interrupt status bits in
k3_ringacc_ring_clear_irq()?

Writing 0xFF to a Write-1-to-Clear (W1C) register blindly clears all active
interrupt bits.

Could this risk dropping unhandled interrupts if multiple interrupts assert
simultaneously between the read and clear phases?

[ ... ]
> @@ -689,6 +801,24 @@ static int k3_ringacc_ring_cfg_sci(struct k3_ring *ring)
>  	ring_cfg.size = ring->elm_size;
>  	ring_cfg.asel = ring->asel;
>  
> +	if (!ringacc->tisci) {
> +		u32 reg;
> +
> +		writel(ring_cfg.addr_lo, &ring->cfg->ba_lo);
> +		writel((ring_cfg.addr_hi & K3_DMARING_CFG_ADDR_HI_MASK) +
> +				(ring_cfg.asel << K3_DMARING_CFG_ASEL_SHIFT),
> +				&ring->cfg->ba_hi);

[Severity: High]
Does this mask out address bits beyond the 36th bit in
k3_ringacc_ring_cfg_sci()?

K3_DMARING_CFG_ADDR_HI_MASK is defined as GENMASK(3, 0), so it only
preserves the lowest 4 bits of ring_cfg.addr_hi.

If the DMA subsystem allocates ring memory above the 36-bit boundary, will
this silently truncate the address and lead to memory corruption or IOMMU
faults?

[ ... ]
> @@ -1347,8 +1477,11 @@ static int k3_ringacc_probe_dt(struct k3_ringacc *ringacc)
>  		return PTR_ERR(ringacc->rm_gp_range);
>  	}
>  
> -	return ti_sci_inta_msi_domain_alloc_irqs(ringacc->dev,
> -						 ringacc->rm_gp_range);
> +	if (IS_ENABLED(CONFIG_TI_K3_UDMA))
> +		return ti_sci_inta_msi_domain_alloc_irqs(ringacc->dev,
> +			ringacc->rm_gp_range);
> +	else
> +		return 0;
>  }

[Severity: High]
Will the driver probe fail unconditionally on platforms without TISCI
firmware?

Earlier in k3_ringacc_probe_dt(), if ti_sci_get_by_phandle() fails because
the ti,sci node is absent, it logs an error and returns the error code
immediately:

    ringacc->tisci = ti_sci_get_by_phandle(node, "ti,sci");
    if (IS_ERR(ringacc->tisci)) {
        ret = PTR_ERR(ringacc->tisci);
        ...
        return ret;
    }

Does this abort the entire probe process, preventing initialization on
platforms like AM62L and rendering the !ringacc->tisci fallback logic
added by this patch unreachable?

[ ... ]
> @@ -1499,6 +1635,10 @@ struct k3_ringacc *k3_ringacc_dmarings_init(struct platform_device *pdev,
>  		struct k3_ring *ring = &ringacc->rings[i];
>  
>  		ring->rt = base_rt + K3_DMARING_RT_REGS_STEP * i;
> +		ring->cfg = base_rt + K3_RINGACC_RT_CFG_REGS_OFS +
> +			    K3_DMARING_RT_REGS_STEP * i;
> +		ring->intr = base_rt + K3_RINGACC_RT_INT_REGS_OFS +
> +			     K3_DMARING_RT_REGS_STEP * i;
>  		ring->parent = ringacc;
>  		ring->ring_id = i;
>  		ring->proxy_id = K3_RINGACC_PROXY_NOT_USED;

[Severity: Medium]
Will this result in undefined behavior due to pointer arithmetic on a NULL
pointer in k3_ringacc_dmarings_init()?

On platforms without TISCI firmware, ringacc->tisci is NULL. Later in
k3_ringacc_dmarings_init(), there is an unconditional assignment:

    ringacc->tisci_ring_ops = &ringacc->tisci->ops.rm_ring_ops;

Evaluating the address of a member of a NULL pointer results in undefined
behavior. Should this be updated to only assign tisci_ring_ops if
ringacc->tisci is valid?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724102115.2854-1-s-adivi@ti.com?part=12