Re: [PATCH v2 6/6] remoteproc: amd_mbv_bram: Add RPMsg support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] remoteproc: amd_mbv_bram: active mailboxes in amd_bram_rproc_stop() race
- [Medium] remoteproc: amd_mbv_bram: leak of carveouts on amd_bram_rproc_prepare() error
- [Medium] remoteproc: amd_mbv_bram: unmapped vdev0buffer added to coredump segments
--

commit 65a1b80207879c4cf9d647ab712871d29690bd39
Author: Ben Levinsky <ben.levinsky@amd.com>

remoteproc: amd_mbv_bram: Add RPMsg support

This commit adds RPMsg support to the AMD MicroBlaze/V BRAM remoteproc
driver by registering reserved-memory carveouts, exposing the loaded
resource table, and using the mailbox framework for virtqueue kicks and
notifications.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index da621f62f7df5..9c2694fd645a6 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c

[ ... ]

> +static void amd_bram_rproc_release_mboxes(void *data)
> +{
> +	struct amd_bram_rproc *priv = data;
> +
> +	/*
> +	 * Free the channels before cancelling the receive work. RX channel
> +	 * shutdown disables the controller interrupt synchronously, so no
> +	 * notification can queue the work again once it has been cancelled.
> +	 */
> +	amd_bram_rproc_free_mboxes(priv);
> +	cancel_work_sync(&priv->rx_work);
> +}
> +
> +static int amd_bram_rproc_add_reserved_mem(struct rproc *rproc)
> +{
> +	struct amd_bram_rproc *priv = rproc->priv;
> +	struct device_node *np = priv->dev->of_node;
> +	struct rproc_mem_entry *mem;
> +	int count;
> +	int ret;
> +	int i;
> +
> +	if (!of_property_present(np, "memory-region"))
> +		return 0;
> +
> +	count = of_reserved_mem_region_count(np);
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		struct resource res;
> +		const char *name;
> +
> +		ret = of_property_read_string_index(np, "memory-region-names",
> +						    i, &name);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to read memory-region name %d\n",
> +					     i);
> +
> +		ret = of_reserved_mem_region_to_resource(np, i, &res);
> +		if (ret)
> +			return dev_err_probe(priv->dev, ret,
> +					     "failed to parse memory-region %d\n", i);
> +
> +		if (!strcmp(name, "vdev0buffer"))
> +			mem = rproc_of_resm_mem_entry_init(priv->dev, i,
> +							   resource_size(&res),
> +							   res.start, name);
> +		else
> +			mem = rproc_mem_entry_init(priv->dev, NULL,
> +						   (dma_addr_t)res.start,
> +						   resource_size(&res), res.start,
> +						   rproc_mem_entry_ioremap_wc,
> +						   rproc_mem_entry_iounmap,
> +						   "%s", name);
> +		if (!mem)
> +			return -ENOMEM;
> +
> +		rproc_add_carveout(rproc, mem);
> +		rproc_coredump_add_segment(rproc, res.start, resource_size(&res));

[Severity: Medium]
Should vdev0buffer be excluded from rproc_coredump_add_segment() in
amd_bram_rproc_add_reserved_mem()?

When name is "vdev0buffer", the entry is initialized with
rproc_of_resm_mem_entry_init(), which leaves mem->alloc and mem->va as
NULL. During boot, rproc_alloc_registered_carveouts() only allocates
entries with an alloc callback, and rproc_add_virtio_dev() attaches the
reserved memory region to the child virtio device without setting mem->va.

Because amd_bram_rproc_ops does not provide a custom da_to_va callback,
rproc_coredump() -> rproc_copy_segment() -> rproc_da_to_va() skips any
carveout with a NULL va, logs an invalid copy request error, and fills the
vdev0buffer coredump segment with 0xff bytes.

> +		dev_dbg(priv->dev, "reserved memory carveout %pR\n", &res);
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_bram_rproc_prepare(struct rproc *rproc)
>  {
>  	struct amd_bram_rproc *priv = rproc->priv;
> @@ -80,7 +292,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
>  	rproc_add_carveout(rproc, mem);
>  	rproc_coredump_add_segment(rproc, da, resource_size(&res));
>  
> -	return 0;
> +	return amd_bram_rproc_add_reserved_mem(rproc);
>  }

[Severity: Medium]
As reported by Sashiko on v1 [1], if amd_bram_rproc_add_reserved_mem()
fails in amd_bram_rproc_prepare(), does this leak the struct
rproc_mem_entry and struct rproc_dump_segment entries already added to
rproc->carveouts and rproc->dump_segments?

In amd_bram_rproc_prepare(), the executable BRAM carveout and coredump
segment are queued onto rproc->carveouts and rproc->dump_segments before
calling amd_bram_rproc_add_reserved_mem(), which also queues entries in a
loop over memory-region.

In drivers/remoteproc/remoteproc_core.c, when rproc_prepare_device() fails
in rproc_fw_boot(), execution jumps directly to disable_iommu and bypasses
clean_up_resources and rproc_resource_cleanup():

drivers/remoteproc/remoteproc_core.c:rproc_fw_boot() {
    ...
	ret = rproc_prepare_device(rproc);
	if (ret) {
		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
		goto disable_iommu;
	}
    ...
}

If amd_bram_rproc_add_reserved_mem() fails at of_reserved_mem_region_count(),
of_property_read_string_index(), of_reserved_mem_region_to_resource(), or
carveout allocation, those queued entries remain on rproc->carveouts and
rproc->dump_segments, causing duplicate entries on subsequent rproc_boot()
calls and leaking them if the device is unbound while offline.

>  static int amd_bram_rproc_start(struct rproc *rproc)
> @@ -133,6 +345,9 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}
> +	cancel_work_sync(&priv->rx_work);
> +	bitmap_zero(priv->pending_notify_ids,
> +		    ARRAY_SIZE(priv->notify_msgs));

[Severity: High]
As reported by Sashiko on v1 [2], since priv->rx_chan and priv->tx_chan
stay active across amd_bram_rproc_stop(), can amd_bram_rproc_rx_callback()
still fire during or after amd_bram_rproc_stop() and re-queue priv->rx_work?

As the comment in amd_bram_rproc_release_mboxes() notes, shutting down the
mailbox channels disables the controller interrupt synchronously and resets
the hardware FIFOs. Because amd_bram_rproc_stop() keeps the channels open,
any notification words still in the hardware RX FIFO or threaded IRQ handler
(amd_pl_mbox_irq_thread() -> amd_pl_mbox_rx() -> mbox_chan_received_data()
-> amd_bram_rproc_rx_callback()) can race with the non-atomic bitmap_zero()
and call schedule_work(&priv->rx_work) after cancel_work_sync() returns.

That allows amd_bram_rproc_rx_work() -> rproc_vq_interrupt() to run
concurrently with __rproc_shutdown() -> rproc_resource_cleanup() ->
rproc_free_vring() as the virtqueues and notify IDs are torn down.

Keeping priv->tx_chan active across stop also leaves any unread kick word in
the hardware TX FIFO while the remote processor is held in reset with its
clock disabled, causing the mailbox core hrtimer to poll last_tx_done every
1 ms while offline and deliver a stale kick on the next start, and loses PL
Mailbox register state across partial reconfiguration (DFX).

[1] https://lore.kernel.org/r/20261014213426.2957480-7-ben.levinsky@amd.com
[2] https://lore.kernel.org/r/20261014213426.2957480-7-ben.levinsky@amd.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105051902.1646651-1-ben.levinsky@amd.com?part=6