[PATCH 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc

[PATCH 0/6] Add AMD PL Mailbox and RPMsg support for the AMD BRAM remoteproc

[Ben Levinsky]

This series adds a mailbox controller for the AMD programmable-logic (PL)
Mailbox soft IP and uses it to add RPMsg support to the AMD MicroBlaze/V
BRAM remoteproc driver, so that a soft processor in the PL can communicate
with Linux over virtio-rpmsg.

Patches 1-2 fix the existing BRAM remoteproc driver:
- Pass the device name to rproc_mem_entry_init() through a constant format
  string instead of using it as the format.
- Drive the reset GPIO with gpiod_direction_output() so a start still
  produces a reset pulse after the PL has been reconfigured, which restores
  the AXI GPIO to its default direction.

Patches 3-4 add the binding and the mailbox-framework controller for the
PL Mailbox IP. The controller exposes independent TX and RX channels. Its
interface clock is enabled only while a channel is started. RX uses the
receive-threshold interrupt and delivers up to one FIFO depth of words per
message, with the depth taken from the device tree. TX completion is
reported by mailbox-core polling of the send-threshold status bit.

Patches 5-6 extend the BRAM remoteproc binding and driver with optional
mailbox channels and named reserved-memory regions for the vrings and the
shared RPMsg buffer pool. The driver registers the carveouts, exposes the
loaded resource table, requests the mailbox channels only while the remote
processor clock is enabled, sends virtqueue notification IDs through the
mailbox framework, and defers received notifications to process context.
Without mailboxes the driver keeps supporting load, start and stop only.

The series applies to v7.3-rc7 and uses the remoteproc wc-ioremap and
optional resource-table helpers merged for v7.3-rc1.


Ben Levinsky (6):
  remoteproc: amd_mbv_bram: Bound device name formatting
  remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL
    reconfig
  dt-bindings: mailbox: Add AMD PL Mailbox IP
  mailbox: Add AMD PL Mailbox soft IP controller
  dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc
  remoteproc: amd_mbv_bram: Add RPMsg support

 .../mailbox/amd,zynqmp-pl-mbox-2.1.yaml       |  74 ++++
 .../bindings/remoteproc/amd,bram-rproc.yaml   |  31 ++
 MAINTAINERS                                   |   7 +
 drivers/mailbox/Kconfig                       |  10 +
 drivers/mailbox/Makefile                      |   1 +
 drivers/mailbox/amd-pl-mailbox.c              | 369 ++++++++++++++++++
 drivers/remoteproc/Kconfig                    |   6 +-
 drivers/remoteproc/amd_mbv_bram_rproc.c       | 256 +++++++++++-
 8 files changed, 748 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml
 create mode 100644 drivers/mailbox/amd-pl-mailbox.c


base-commit: 0fcb05b72344f40806cbe810de8872f1698bf053
-- 
2.34.1

[PATCH 1/6] remoteproc: amd_mbv_bram: Bound device name formatting

[Ben Levinsky]

rproc_mem_entry_init() interprets its name argument as a printf format
string. Passing the device name directly allows percent conversions in a
DT-derived platform device name to consume nonexistent variadic arguments.

Pass the device name through a constant format so it is always handled as
data. Limit the formatted name to 31 characters to match the space
available in rproc_mem_entry::name while leaving room for its terminating
null byte.

Fixes: f87b4998994e ("remoteproc: Add AMD MicroBlaze/V BRAM-based remote processor driver")
Reviewed-by: Tanmay Shah <tanmay.shah@amd.com>
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/amd_mbv_bram_rproc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index e4a103cf8455..6cf33c7b7e64 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -72,7 +72,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
 				   resource_size(&res), da,
 				   rproc_mem_entry_ioremap_wc,
 				   rproc_mem_entry_iounmap,
-				   dev_name(priv->dev));
+				   "%.31s", dev_name(priv->dev));
 	if (!mem)
 		return -ENOMEM;
 
-- 
2.34.1

Re: [PATCH 1/6] remoteproc: amd_mbv_bram: Bound device name formatting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] remoteproc: carveout memory leak on rproc_parse_fw() failure
--

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

remoteproc: amd_mbv_bram: Bound device name formatting

Pass the device name through a constant format string to prevent percent
conversions in the device name from consuming nonexistent variadic arguments,
and bound it to 31 characters.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index e4a103cf84558..6cf33c7b7e641 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
> @@ -72,7 +72,7 @@ static int amd_bram_rproc_prepare(struct rproc *rproc)
>  				   resource_size(&res), da,
>  				   rproc_mem_entry_ioremap_wc,
>  				   rproc_mem_entry_iounmap,
> -				   dev_name(priv->dev));
> +				   "%.31s", dev_name(priv->dev));
>  	if (!mem)
>  		return -ENOMEM;
>  

[Severity: Medium]
Could this introduce a regression where the carveout memory leaks?

If rproc_parse_fw() fails during boot (for instance, due to a memory
allocation failure in rproc_elf_load_rsc_table()), the error path in
rproc_fw_boot() bypasses rproc_resource_cleanup():

drivers/remoteproc/remoteproc_core.c:rproc_fw_boot() {
    ...
	ret = rproc_parse_fw(rproc, fw);
	if (ret)
		goto unprepare_rproc;
    ...
clean_up_resources:
	rproc_resource_cleanup(rproc);
    ...
unprepare_rproc:
	rproc_unprepare_device(rproc);
    ...
}

Because the driver adds the carveout to rproc->carveouts shortly after this
allocation:

	mem = rproc_mem_entry_init(...);
	if (!mem)
		return -ENOMEM;

	rproc_add_carveout(rproc, mem);

And relies on the core's rproc_resource_cleanup() for cleanup, the carveout
would remain on the list if parsing fails.

Subsequent rproc_boot() attempts would then call prepare() again, potentially
adding duplicate carveouts and causing unbounded list growth.

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

[PATCH 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

[Ben Levinsky]

Partial reconfiguration of the programmable logic restores the AXI GPIO
that drives the soft-core reset to its hardware default direction, but
gpiolib keeps its cached output direction. gpiod_set_value_cansleep()
then only updates the data register, so the processor never sees a
reset transition: remoteproc reports it as running while the newly
loaded firmware never starts.

Use gpiod_direction_output() for every reset transition so the output
direction is re-established, and assert reset briefly before releasing
it so each start produces a clean reset pulse after the firmware has
been loaded.

Fixes: f87b4998994e ("remoteproc: Add AMD MicroBlaze/V BRAM-based remote processor driver")
Assisted-by: LLM sparse
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/amd_mbv_bram_rproc.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index 6cf33c7b7e64..da621f62f7df 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -94,8 +95,24 @@ static int amd_bram_rproc_start(struct rproc *rproc)
 		return ret;
 	}
 
+	/*
+	 * Partial reconfiguration restores the PL GPIO hardware defaults but
+	 * leaves gpiolib's cached direction unchanged. Re-establish the output
+	 * direction and generate a reset pulse after firmware has been loaded.
+	 * gpiod_set_value_cansleep() is insufficient here because it changes the
+	 * value without restoring an output direction lost during DFX.
+	 */
+	ret = gpiod_direction_output(priv->reset, 1);
+	if (ret) {
+		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+	/* Hold reset asserted long enough to provide a clean reset pulse. */
+	usleep_range(10, 20);
+
 	/* Deassert reset and let the processor run. */
-	ret = gpiod_set_value_cansleep(priv->reset, 0);
+	ret = gpiod_direction_output(priv->reset, 0);
 	if (ret) {
 		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
 		clk_disable_unprepare(priv->clk);
@@ -110,8 +127,8 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
 	struct amd_bram_rproc *priv = rproc->priv;
 	int ret;
 
-	/* Assert reset before disabling the processor clock. */
-	ret = gpiod_set_value_cansleep(priv->reset, 1);
+	/* Restore the output direction after DFX and assert reset. */
+	ret = gpiod_direction_output(priv->reset, 1);
 	if (ret) {
 		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
 		return ret;
-- 
2.34.1

Re: [PATCH 2/6] remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] remoteproc: amd_mbv_bram: returning error from stop() leaks resources during unbind
--

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

remoteproc: amd_mbv_bram: Restore reset GPIO direction after PL reconfig

Partial reconfiguration of the programmable logic restores the AXI GPIO
that drives the soft-core reset to its hardware default direction, but
gpiolib keeps its cached output direction. gpiod_set_value_cansleep()
then only updates the data register, so the processor never sees a
reset transition: remoteproc reports it as running while the newly
loaded firmware never starts.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index 6cf33c7b7e641..da621f62f7df5 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> @@ -110,8 +127,8 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  	struct amd_bram_rproc *priv = rproc->priv;
>  	int ret;
>  
> -	/* Assert reset before disabling the processor clock. */
> -	ret = gpiod_set_value_cansleep(priv->reset, 1);
> +	/* Restore the output direction after DFX and assert reset. */
> +	ret = gpiod_direction_output(priv->reset, 1);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Disable clock after asserting reset */
>  	clk_disable_unprepare(priv->clk);
>  
>  	return 0;
>  }

[Severity: High]
Does this error path bypass local clock teardown and cause the core remoteproc
to abort its cleanup?

If gpiod_direction_output() fails in amd_bram_rproc_stop() during device unbind
or module removal, it returns an error. This propagates to __rproc_shutdown():

drivers/remoteproc/remoteproc_core.c:__rproc_shutdown() {
	...
	ret = rproc_stop(rproc, crashed);
	if (ret) {
		atomic_inc(&rproc->power);
		goto out;
	}

	/* clean up all acquired resources */
	rproc_resource_cleanup(rproc);
	...
}

This causes __rproc_shutdown() to abort early and skip rproc_resource_cleanup().

Since rproc_del() proceeds unconditionally:

drivers/remoteproc/remoteproc_core.c:rproc_del() {
	...
	__rproc_shutdown(rproc, true);

	rproc_delete_debug_dir(rproc);
	...
}

the error from __rproc_shutdown() is ignored.

Can this result in the rproc object and driver memory being freed by devm
while leaving the clock enabled, core remoteproc resources (carveouts,
IOMMU mappings, trace files) leaked, and any local mailboxes/works uncancelled?

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

[PATCH 3/6] dt-bindings: mailbox: Add AMD PL Mailbox IP

[Ben Levinsky]

Document the AMD programmable-logic Mailbox IP used for bidirectional
FIFO communication between two processor-facing interfaces.

The same IP is used on ZynqMP, Versal, Versal NET and Versal Gen 2.
Name each SoC in its compatible, and use amd,zynqmp-pl-mbox-2.1 as the
fallback that drivers match.

Define the transmit and receive channel indices together with the register
aperture, interrupt, and interface clock. Describe the FIFO depth chosen
when the IP is configured, so that the driver can size received messages
to it.

Assisted-by: LLM sparse
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 .../mailbox/amd,zynqmp-pl-mbox-2.1.yaml       | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml

diff --git a/Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml b/Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml
new file mode 100644
index 000000000000..2cc0d106b888
--- /dev/null
+++ b/Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mailbox/amd,zynqmp-pl-mbox-2.1.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMD Programmable Logic Mailbox IP
+
+maintainers:
+  - Ben Levinsky <ben.levinsky@amd.com>
+
+description: |
+  The AMD Programmable Logic Mailbox IP provides bidirectional message passing
+  between two processor-facing interfaces. Each interface writes messages to
+  a send FIFO and reads messages from a receive FIFO. The register aperture,
+  interrupt, and clock described by a node belong to one processor-facing
+  interface. Channel 0 represents transmit and channel 1 represents receive
+  for that interface.
+
+properties:
+  compatible:
+    oneOf:
+      - const: amd,zynqmp-pl-mbox-2.1
+      - items:
+          - enum:
+              - amd,versal-pl-mbox-2.1
+              - amd,versal-net-pl-mbox-2.1
+              - amd,versal2-pl-mbox-2.1
+          - const: amd,zynqmp-pl-mbox-2.1
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  amd,mbox-pl-fifo-len:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 16
+    maximum: 8192
+    default: 16
+    description:
+      Depth of the send and receive FIFOs in 32-bit words, as configured by
+      the C_MAILBOX_DEPTH parameter of the IP. Must match the configured
+      depth.
+
+  '#mbox-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - '#mbox-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    mailbox@b2060000 {
+        compatible = "amd,zynqmp-pl-mbox-2.1";
+        reg = <0xb2060000 0x10000>;
+        interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&pl_clk>;
+        amd,mbox-pl-fifo-len = <16>;
+        #mbox-cells = <1>;
+    };
+...
-- 
2.34.1

[PATCH 4/6] mailbox: Add AMD PL Mailbox soft IP controller

[Ben Levinsky]

Add a mailbox-framework controller for the AMD programmable-logic
Mailbox soft IP. Expose separate transmit and receive FIFO channels.

Enable the interface clock only while a channel is started, before any
register access, and disable it last when the channel shuts down. Keep
provider probe free of register accesses and leave the IRQ disabled until
a client starts the receive channel. Initialize each FIFO when its channel
starts, use receive-threshold interrupts for incoming words, and poll the
send-threshold status for transmit completion. Stop that polling from
accessing registers once the transmit channel begins to shut down.

Carry mailbox payloads in struct zynqmp_ipi_message, limited to the FIFO
depth given by the device tree. The FIFO transfers 32-bit words and the
hardware conveys neither a byte count nor the FIFO occupancy, so a
transmit length that is not a multiple of four is zero-padded to a whole
word.

On a receive-threshold interrupt, read up to one FIFO depth of words and
deliver them as one message, then acknowledge the interrupt. The event
latches only when the FIFO goes from empty to nonempty, so a word that
arrives between the drain and the acknowledge, or words left by the
bounded drain, raise no new event. If the FIFO is nonempty after the
acknowledge, toggle the threshold to latch a new event for them.

Assisted-by: LLM sparse
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 MAINTAINERS                      |   7 +
 drivers/mailbox/Kconfig          |  10 +
 drivers/mailbox/Makefile         |   1 +
 drivers/mailbox/amd-pl-mailbox.c | 369 +++++++++++++++++++++++++++++++
 4 files changed, 387 insertions(+)
 create mode 100644 drivers/mailbox/amd-pl-mailbox.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f53047aa8d3c..4e28e0c311c0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1287,6 +1287,13 @@ S:	Supported
 T:	git https://gitlab.freedesktop.org/agd5f/linux.git
 F:	drivers/gpu/drm/amd/pm/
 
+AMD PROGRAMMABLE LOGIC MAILBOX DRIVER
+M:	Ben Levinsky <ben.levinsky@amd.com>
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/mailbox/amd,zynqmp-pl-mbox-2.1.yaml
+F:	drivers/mailbox/amd-pl-mailbox.c
+
 AMD PSTATE DRIVER
 M:	Huang Rui <ray.huang@amd.com>
 M:	Mario Limonciello <mario.limonciello@amd.com>
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
index ab869769e278..3c3f262c903c 100644
--- a/drivers/mailbox/Kconfig
+++ b/drivers/mailbox/Kconfig
@@ -8,6 +8,16 @@ menuconfig MAILBOX
 
 if MAILBOX
 
+config AMD_PL_MAILBOX
+	tristate "AMD PL Mailbox soft IP controller"
+	depends on HAS_IOMEM
+	depends on OF
+	help
+	  Mailbox controller for the AMD Programmable Logic Mailbox soft IP.
+	  The controller exposes transmit and receive FIFO channels to mailbox
+	  clients. It supports interrupt-driven receive notification and
+	  polling-based transmit completion through the common mailbox framework.
+
 config ARM_MHU
 	tristate "ARM MHU Mailbox"
 	depends on ARM_AMBA
diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
index 45dc59bb3e7f..5614d7e1e2f8 100644
--- a/drivers/mailbox/Makefile
+++ b/drivers/mailbox/Makefile
@@ -4,6 +4,7 @@
 obj-$(CONFIG_MAILBOX)		+= mailbox.o
 
 obj-$(CONFIG_MAILBOX_TEST)	+= mailbox-test.o
+obj-$(CONFIG_AMD_PL_MAILBOX)	+= amd-pl-mailbox.o
 
 obj-$(CONFIG_ARM_MHU)	+= arm_mhu.o arm_mhu_db.o
 
diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
new file mode 100644
index 000000000000..bd6595332f53
--- /dev/null
+++ b/drivers/mailbox/amd-pl-mailbox.c
@@ -0,0 +1,369 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mailbox/zynqmp-ipi-message.h>
+#include <linux/mailbox_controller.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define AMD_PL_MBOX_WRDATA		0x00
+#define AMD_PL_MBOX_RDDATA		0x08
+#define AMD_PL_MBOX_STATUS		0x10
+#define AMD_PL_MBOX_ERROR		0x14
+#define AMD_PL_MBOX_SEND_THRESHOLD	0x18
+#define AMD_PL_MBOX_RECV_THRESHOLD	0x1c
+#define AMD_PL_MBOX_IRQ_STATUS		0x20
+#define AMD_PL_MBOX_IRQ_ENABLE		0x24
+#define AMD_PL_MBOX_CTRL		0x2c
+
+#define AMD_PL_MBOX_STS_RX_EMPTY	BIT(0)
+#define AMD_PL_MBOX_STS_TX_FULL		BIT(1)
+#define AMD_PL_MBOX_STS_TX_THRESHOLD	BIT(2)
+#define AMD_PL_MBOX_IRQ_RX_THRESHOLD	BIT(1)
+#define AMD_PL_MBOX_IRQ_ERROR		BIT(2)
+#define AMD_PL_MBOX_CTRL_TX_RESET	BIT(0)
+#define AMD_PL_MBOX_CTRL_RX_RESET	BIT(1)
+
+/* FIFO depth range of the IP, in 32-bit words. */
+#define AMD_PL_MBOX_FIFO_LEN_MIN	16
+#define AMD_PL_MBOX_FIFO_LEN_MAX	8192
+
+/*
+ * The send threshold is active while send FIFO occupancy is less than or
+ * equal to the programmed value. The receive threshold is active while
+ * receive FIFO occupancy is greater than the programmed value. The related
+ * interrupt status bits latch when a threshold condition becomes active,
+ * and a new event takes priority over a simultaneous status clear.
+ *
+ * A send threshold of zero makes the status bit indicate an empty TX FIFO.
+ * A receive threshold of zero raises an interrupt for the first received
+ * word after the RX FIFO becomes empty.
+ */
+
+enum {
+	AMD_PL_MBOX_CHAN_TX = 0,
+	AMD_PL_MBOX_CHAN_RX = 1,
+	AMD_PL_MBOX_NUM_CHANS = 2,
+};
+
+/**
+ * struct amd_pl_mbox - AMD PL Mailbox controller state
+ * @regs: mapped register aperture
+ * @clk: interface clock, enabled while a channel is started
+ * @irq: receive interrupt
+ * @fifo_len: FIFO depth in 32-bit words
+ * @tx_lock: serializes TX completion polling against TX shutdown
+ * @tx_active: TX channel is started and its registers may be accessed
+ * @controller: common mailbox controller
+ * @chans: transmit and receive mailbox channels
+ * @rx_msg: receive buffer passed to mailbox clients
+ */
+struct amd_pl_mbox {
+	void __iomem *regs;
+	struct clk *clk;
+	int irq;
+	u32 fifo_len;
+	spinlock_t tx_lock; /* protects tx_active */
+	bool tx_active;
+	struct mbox_controller controller;
+	struct mbox_chan chans[AMD_PL_MBOX_NUM_CHANS];
+	struct zynqmp_ipi_message *rx_msg;
+};
+
+static int amd_pl_mbox_send_data(struct mbox_chan *chan, void *data)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	struct zynqmp_ipi_message *msg = data;
+	size_t offset;
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX])
+		return -EINVAL;
+
+	/* The TX FIFO is empty here, since TX completion waits for it. */
+	if (!msg || !msg->len || msg->len > mb->fifo_len * sizeof(u32))
+		return -EINVAL;
+
+	for (offset = 0; offset < msg->len; offset += sizeof(u32)) {
+		size_t count = min(sizeof(u32), msg->len - offset);
+		u32 word = 0;
+
+		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
+		    AMD_PL_MBOX_STS_TX_FULL)
+			return -EBUSY;
+
+		memcpy(&word, &msg->data[offset], count);
+		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);
+	}
+
+	return 0;
+}
+
+static bool amd_pl_mbox_last_tx_done(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	bool done = true;
+
+	/*
+	 * The mailbox core can poll after shutdown() has started, while the
+	 * interface clock is being disabled. Do not access registers then.
+	 */
+	spin_lock_irqsave(&mb->tx_lock, flags);
+	if (mb->tx_active)
+		/* SIT is zero, so the threshold status means TX FIFO empty. */
+		done = readl(mb->regs + AMD_PL_MBOX_STATUS) &
+		       AMD_PL_MBOX_STS_TX_THRESHOLD;
+	spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+	return done;
+}
+
+static int amd_pl_mbox_startup(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	u32 irq_enable;
+	int ret;
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX] &&
+	    chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
+		return -EINVAL;
+
+	ret = clk_prepare_enable(mb->clk);
+	if (ret)
+		return ret;
+
+	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
+		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+		writel(0, mb->regs + AMD_PL_MBOX_SEND_THRESHOLD);
+
+		spin_lock_irqsave(&mb->tx_lock, flags);
+		mb->tx_active = true;
+		spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+		return 0;
+	}
+
+	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+	/* Interrupt when the RX FIFO changes from empty to nonempty. */
+	writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
+	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
+	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	irq_enable |= AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR;
+	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	enable_irq(mb->irq);
+
+	return 0;
+}
+
+static void amd_pl_mbox_shutdown(struct mbox_chan *chan)
+{
+	struct amd_pl_mbox *mb = chan->con_priv;
+	unsigned long flags;
+	u32 irq_enable;
+
+	if (chan == &mb->chans[AMD_PL_MBOX_CHAN_TX]) {
+		spin_lock_irqsave(&mb->tx_lock, flags);
+		mb->tx_active = false;
+		spin_unlock_irqrestore(&mb->tx_lock, flags);
+
+		writel(AMD_PL_MBOX_CTRL_TX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+		clk_disable_unprepare(mb->clk);
+		return;
+	}
+
+	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_RX])
+		return;
+
+	disable_irq(mb->irq);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	irq_enable &= ~(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR);
+	writel(irq_enable, mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	writel(AMD_PL_MBOX_IRQ_RX_THRESHOLD | AMD_PL_MBOX_IRQ_ERROR,
+	       mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	writel(AMD_PL_MBOX_CTRL_RX_RESET, mb->regs + AMD_PL_MBOX_CTRL);
+	clk_disable_unprepare(mb->clk);
+}
+
+static const struct mbox_chan_ops amd_pl_mbox_ops = {
+	.send_data = amd_pl_mbox_send_data,
+	.last_tx_done = amd_pl_mbox_last_tx_done,
+	.startup = amd_pl_mbox_startup,
+	.shutdown = amd_pl_mbox_shutdown,
+};
+
+static void amd_pl_mbox_rx(struct amd_pl_mbox *mb)
+{
+	/* data[] follows a size_t in a kzalloc() buffer: u32 aligned. */
+	u32 *rx_words = (u32 *)mb->rx_msg->data;
+	u32 words;
+
+	/*
+	 * The hardware reports neither the peer's message length nor the FIFO
+	 * occupancy. Read until the FIFO is empty, up to one FIFO depth, and
+	 * deliver the words as one message.
+	 */
+	for (words = 0; words < mb->fifo_len; words++) {
+		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
+		    AMD_PL_MBOX_STS_RX_EMPTY)
+			break;
+
+		rx_words[words] = readl(mb->regs + AMD_PL_MBOX_RDDATA);
+	}
+
+	if (words) {
+		mb->rx_msg->len = words * sizeof(u32);
+		mbox_chan_received_data(&mb->chans[AMD_PL_MBOX_CHAN_RX],
+					mb->rx_msg);
+	}
+}
+
+static void amd_pl_mbox_rx_rearm(struct amd_pl_mbox *mb)
+{
+	/*
+	 * The receive event latches only when the FIFO goes from empty to
+	 * nonempty. A word that arrived after the drain, but before the event
+	 * was acknowledged, had its event cleared, and words left by the
+	 * bounded drain never raised one. Raise the threshold and lower it
+	 * again to latch a new event, so that the interrupt fires for them.
+	 * This cannot latch an event while the FIFO is full, which happens only
+	 * if the peer writes a full FIFO depth during one interrupt.
+	 */
+	if (!(readl(mb->regs + AMD_PL_MBOX_STATUS) &
+	      AMD_PL_MBOX_STS_RX_EMPTY)) {
+		writel(mb->fifo_len - 1, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
+		writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
+	}
+}
+
+static irqreturn_t amd_pl_mbox_irq(int irq, void *data)
+{
+	struct amd_pl_mbox *mb = data;
+	u32 pending_intr;
+	u32 irq_enable;
+	u32 irq_status;
+
+	irq_status = readl(mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+	irq_enable = readl(mb->regs + AMD_PL_MBOX_IRQ_ENABLE);
+	pending_intr = irq_status & irq_enable;
+
+	if (!pending_intr) {
+		dev_warn_ratelimited(mb->controller.dev,
+				     "unexpected interrupt: status %#x enable %#x\n",
+				     irq_status, irq_enable);
+		return IRQ_NONE;
+	}
+
+	if (pending_intr & AMD_PL_MBOX_IRQ_RX_THRESHOLD)
+		amd_pl_mbox_rx(mb);
+
+	if (pending_intr & AMD_PL_MBOX_IRQ_ERROR)
+		dev_warn_ratelimited(mb->controller.dev,
+				     "mailbox FIFO error: %#x\n",
+				     readl(mb->regs + AMD_PL_MBOX_ERROR));
+
+	writel(pending_intr, mb->regs + AMD_PL_MBOX_IRQ_STATUS);
+
+	if (pending_intr & AMD_PL_MBOX_IRQ_RX_THRESHOLD)
+		amd_pl_mbox_rx_rearm(mb);
+
+	return IRQ_HANDLED;
+}
+
+static struct mbox_chan *amd_pl_mbox_xlate(struct mbox_controller *ctlr,
+					   const struct of_phandle_args *sp)
+{
+	if (sp->args_count != 1 || sp->args[0] >= AMD_PL_MBOX_NUM_CHANS)
+		return ERR_PTR(-EINVAL);
+
+	return &ctlr->chans[sp->args[0]];
+}
+
+static int amd_pl_mbox_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct amd_pl_mbox *mb;
+	int ret;
+	int i;
+
+	mb = devm_kzalloc(dev, sizeof(*mb), GFP_KERNEL);
+	if (!mb)
+		return -ENOMEM;
+
+	spin_lock_init(&mb->tx_lock);
+
+	mb->fifo_len = AMD_PL_MBOX_FIFO_LEN_MIN;
+	ret = of_property_read_u32(dev->of_node, "amd,mbox-pl-fifo-len",
+				   &mb->fifo_len);
+	if (ret && ret != -EINVAL)
+		return dev_err_probe(dev, ret, "failed to read FIFO length\n");
+
+	if (mb->fifo_len < AMD_PL_MBOX_FIFO_LEN_MIN ||
+	    mb->fifo_len > AMD_PL_MBOX_FIFO_LEN_MAX)
+		return dev_err_probe(dev, -EINVAL, "invalid FIFO length %u\n",
+				     mb->fifo_len);
+
+	mb->rx_msg = devm_kzalloc(dev, struct_size(mb->rx_msg, data,
+						   mb->fifo_len * sizeof(u32)),
+				  GFP_KERNEL);
+	if (!mb->rx_msg)
+		return -ENOMEM;
+
+	mb->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mb->regs))
+		return PTR_ERR(mb->regs);
+
+	mb->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(mb->clk))
+		return dev_err_probe(dev, PTR_ERR(mb->clk), "failed to get clock\n");
+
+	mb->irq = platform_get_irq(pdev, 0);
+	if (mb->irq < 0)
+		return mb->irq;
+
+	for (i = 0; i < AMD_PL_MBOX_NUM_CHANS; i++)
+		mb->chans[i].con_priv = mb;
+
+	mb->controller.dev = dev;
+	mb->controller.ops = &amd_pl_mbox_ops;
+	mb->controller.chans = mb->chans;
+	mb->controller.num_chans = AMD_PL_MBOX_NUM_CHANS;
+	mb->controller.txdone_poll = true;
+	mb->controller.txpoll_period = 1;
+	mb->controller.of_xlate = amd_pl_mbox_xlate;
+
+	ret = devm_request_irq(dev, mb->irq, amd_pl_mbox_irq,
+			       IRQF_NO_AUTOEN,
+			       dev_name(dev), mb);
+	if (ret)
+		return ret;
+
+	return devm_mbox_controller_register(dev, &mb->controller);
+}
+
+static const struct of_device_id amd_pl_mbox_of_match[] = {
+	{ .compatible = "amd,zynqmp-pl-mbox-2.1" },
+	{ .compatible = "amd,versal-pl-mbox-2.1" },
+	{ .compatible = "amd,versal-net-pl-mbox-2.1" },
+	{ .compatible = "amd,versal2-pl-mbox-2.1" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, amd_pl_mbox_of_match);
+
+static struct platform_driver amd_pl_mbox_driver = {
+	.probe = amd_pl_mbox_probe,
+	.driver = {
+		.name = "amd-pl-mailbox",
+		.of_match_table = amd_pl_mbox_of_match,
+	},
+};
+module_platform_driver(amd_pl_mbox_driver);
+
+MODULE_DESCRIPTION("AMD PL Mailbox soft IP controller");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH 4/6] mailbox: Add AMD PL Mailbox soft IP controller

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] io: endianness bug when reading/writing FIFO data with readl/writel
- [High] mailbox: missed RX interrupt edge transition when FIFO is completely full
--

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

mailbox: Add AMD PL Mailbox soft IP controller

Adds a mailbox framework controller for the AMD programmable-logic Mailbox soft
IP. The controller exposes separate transmit and receive FIFO channels,
supporting interrupt-driven receive notifications and polling-based transmit
completion.

> diff --git a/drivers/mailbox/amd-pl-mailbox.c b/drivers/mailbox/amd-pl-mailbox.c
> new file mode 100644
> index 0000000000000..bd6595332f533
> --- /dev/null
> +++ b/drivers/mailbox/amd-pl-mailbox.c

[ ... ]

> +static int amd_pl_mbox_send_data(struct mbox_chan *chan, void *data)
> +{
> +	struct amd_pl_mbox *mb = chan->con_priv;
> +	struct zynqmp_ipi_message *msg = data;
> +	size_t offset;
> +
> +	if (chan != &mb->chans[AMD_PL_MBOX_CHAN_TX])
> +		return -EINVAL;
> +
> +	/* The TX FIFO is empty here, since TX completion waits for it. */
> +	if (!msg || !msg->len || msg->len > mb->fifo_len * sizeof(u32))
> +		return -EINVAL;
> +
> +	for (offset = 0; offset < msg->len; offset += sizeof(u32)) {
> +		size_t count = min(sizeof(u32), msg->len - offset);
> +		u32 word = 0;
> +
> +		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +		    AMD_PL_MBOX_STS_TX_FULL)
> +			return -EBUSY;
> +
> +		memcpy(&word, &msg->data[offset], count);
> +		writel(word, mb->regs + AMD_PL_MBOX_WRDATA);

[Severity: High]
Will this corrupt the byte order of the stream payload on big-endian
architectures? writel performs implicit CPU-to-device byteswapping.
For bulk FIFO stream data, should this use writesl to preserve the byte
layout of the zynqmp_ipi_message stream when crossing endianness boundaries?

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void amd_pl_mbox_rx(struct amd_pl_mbox *mb)
> +{
> +	/* data[] follows a size_t in a kzalloc() buffer: u32 aligned. */
> +	u32 *rx_words = (u32 *)mb->rx_msg->data;
> +	u32 words;
> +
> +	/*
> +	 * The hardware reports neither the peer's message length nor the FIFO
> +	 * occupancy. Read until the FIFO is empty, up to one FIFO depth, and
> +	 * deliver the words as one message.
> +	 */
> +	for (words = 0; words < mb->fifo_len; words++) {
> +		if (readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +		    AMD_PL_MBOX_STS_RX_EMPTY)
> +			break;
> +
> +		rx_words[words] = readl(mb->regs + AMD_PL_MBOX_RDDATA);

[Severity: High]
Similarly, does using readl for stream data cause byte swapping issues on
big-endian systems? Should this use readsl to read the FIFO without unwanted
endianness conversion?

> +	}
> +
> +	if (words) {
> +		mb->rx_msg->len = words * sizeof(u32);
> +		mbox_chan_received_data(&mb->chans[AMD_PL_MBOX_CHAN_RX],
> +					mb->rx_msg);
> +	}
> +}
> +
> +static void amd_pl_mbox_rx_rearm(struct amd_pl_mbox *mb)
> +{
> +	/*
> +	 * The receive event latches only when the FIFO goes from empty to
> +	 * nonempty. A word that arrived after the drain, but before the event
> +	 * was acknowledged, had its event cleared, and words left by the
> +	 * bounded drain never raised one. Raise the threshold and lower it
> +	 * again to latch a new event, so that the interrupt fires for them.
> +	 * This cannot latch an event while the FIFO is full, which happens only
> +	 * if the peer writes a full FIFO depth during one interrupt.
> +	 */
> +	if (!(readl(mb->regs + AMD_PL_MBOX_STATUS) &
> +	      AMD_PL_MBOX_STS_RX_EMPTY)) {
> +		writel(mb->fifo_len - 1, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);
> +		writel(0, mb->regs + AMD_PL_MBOX_RECV_THRESHOLD);

[Severity: High]
Can this permanently lose the RX interrupt if the FIFO becomes completely full?

If the FIFO has exactly fifo_len words, both occupancy > fifo_len - 1 and
occupancy > 0 evaluate to true. Without a false-to-true edge transition, the
hardware might not latch the new event, leaving data stuck in the FIFO.

> +	}
> +}

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

[PATCH 5/6] dt-bindings: remoteproc: Add RPMsg resources to AMD BRAM rproc

[Ben Levinsky]

Describe the optional mailbox channels and named reserved-memory
regions used for RPMsg communication with a BRAM-based MicroBlaze
remote processor.

The three named memory regions provide the two virtio vrings and the
shared RPMsg buffer pool. Firmware and its loaded resource table remain
in the executable BRAM described by reg.

Assisted-by: LLM sparse
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 .../bindings/remoteproc/amd,bram-rproc.yaml   | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
index c0359f447ea8..daff8e615ab6 100644
--- a/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/amd,bram-rproc.yaml
@@ -84,6 +84,33 @@ properties:
     description:
       GPIO specifier controlling the soft-core reset input.
 
+  mboxes:
+    maxItems: 2
+
+  mbox-names:
+    items:
+      - const: tx
+      - const: rx
+
+  memory-region:
+    maxItems: 3
+
+  memory-region-names:
+    items:
+      - const: vdev0vring0
+      - const: vdev0vring1
+      - const: vdev0buffer
+
+dependentRequired:
+  mboxes:
+    - mbox-names
+  mbox-names:
+    - mboxes
+  memory-region:
+    - memory-region-names
+  memory-region-names:
+    - memory-region
+
 required:
   - compatible
   - reg
@@ -101,5 +128,9 @@ examples:
       clocks = <&pl_clk>;
       firmware-name = "firmware.elf";
       reset-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
+      mboxes = <&mbv_mbox0 0>, <&mbv_mbox0 1>;
+      mbox-names = "tx", "rx";
+      memory-region = <&mbv_vring0>, <&mbv_vring1>, <&mbv_vdev0buffer>;
+      memory-region-names = "vdev0vring0", "vdev0vring1", "vdev0buffer";
     };
 ...
-- 
2.34.1

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

[Ben Levinsky]

Register named reserved-memory carveouts and expose the firmware
resource table after it has been loaded into BRAM. This lets remoteproc
create virtqueues backed by reserved shared DDR without deriving
carveout roles from DT node names. Map the shared-memory carveouts with
the common rproc_mem_entry_ioremap_wc() and rproc_mem_entry_iounmap()
callbacks.

Use the mailbox framework to send virtqueue notification IDs. Defer
received notifications to process context before dispatching them to
the remoteproc virtqueues. Keep mailbox payload storage valid until
transmit completion and order write-combined vring updates before each
kick.

For designs where the mailbox and remote processor share a clock, request
the mailbox channels only after enabling that clock, while the processor
is still held in reset. Release the channels before disabling the clock
during stop.

The mailboxes are optional. Without them the driver keeps supporting
load, start, and stop only.

Select the mailbox framework, the AMD PL Mailbox controller and the
virtio RPMsg bus, which the driver uses for notifications and RPMsg
channels. Selecting MAILBOX while depending on GPIOLIB creates a
Kconfig recursive dependency, because GPIOLIB is selected by drivers
that are reachable from MAILBOX, so select GPIOLIB as well instead of
depending on it. The driver always needs GPIO for the reset line.
Depend on HAS_IOMEM, which the selected mailbox controller and the
write-combined carveout mappings require.

Assisted-by: LLM sparse
Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
---
 drivers/remoteproc/Kconfig              |   6 +-
 drivers/remoteproc/amd_mbv_bram_rproc.c | 231 +++++++++++++++++++++++-
 2 files changed, 235 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index 5b56b2dcc725..9ebbbf76289f 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -25,7 +25,11 @@ config REMOTEPROC_CDEV
 
 config AMD_MBV_BRAM_REMOTEPROC
 	tristate "AMD MicroBlaze/V BRAM-based remoteproc support"
-	depends on OF && COMMON_CLK && (GPIOLIB || COMPILE_TEST)
+	depends on OF && COMMON_CLK && HAS_IOMEM
+	select GPIOLIB
+	select MAILBOX
+	select AMD_PL_MAILBOX
+	select RPMSG_VIRTIO
 	help
 	  Say y or m here to support a MicroBlaze/V BRAM-based remote
 	  processor managed through the remoteproc framework.
diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
index da621f62f7df..65d2c3946325 100644
--- a/drivers/remoteproc/amd_mbv_bram_rproc.c
+++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
@@ -15,15 +15,20 @@
  * Reset is controlled via GPIO connected to Processor System Reset IP.
  */
 
+#include <linux/bitmap.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/gpio/consumer.h>
+#include <linux/mailbox/zynqmp-ipi-message.h>
+#include <linux/mailbox_client.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/remoteproc.h>
+#include <linux/workqueue.h>
 
 #include "remoteproc_internal.h"
 
@@ -32,13 +37,207 @@
  * @dev: device pointer
  * @reset: GPIO descriptor for reset control (active-low)
  * @clk: processor clock
+ * @rproc: remote processor instance
+ * @tx_client: mailbox client used for virtqueue kicks
+ * @rx_client: mailbox client used for remote notifications
+ * @tx_chan: transmit mailbox channel
+ * @rx_chan: receive mailbox channel
+ * @notify_msgs: persistent mailbox payload storage indexed by notify ID
+ * @pending_notify_ids: notifications awaiting process-context dispatch
+ * @rx_work: work item that dispatches received virtqueue notifications
  */
 struct amd_bram_rproc {
 	struct device *dev;
 	struct gpio_desc *reset;
 	struct clk *clk;
+	struct rproc *rproc;
+	struct mbox_client tx_client;
+	struct mbox_client rx_client;
+	struct mbox_chan *tx_chan;
+	struct mbox_chan *rx_chan;
+	struct zynqmp_ipi_message *notify_msgs[256];
+	DECLARE_BITMAP(pending_notify_ids, 256);
+	struct work_struct rx_work;
 };
 
+static void amd_bram_rproc_rx_work(struct work_struct *work)
+{
+	struct amd_bram_rproc *priv =
+		container_of(work, struct amd_bram_rproc, rx_work);
+	unsigned long notifyid;
+
+	for_each_set_bit(notifyid, priv->pending_notify_ids,
+			 ARRAY_SIZE(priv->notify_msgs)) {
+		clear_bit(notifyid, priv->pending_notify_ids);
+		if (rproc_vq_interrupt(priv->rproc, notifyid) == IRQ_NONE)
+			dev_dbg(priv->dev, "no virtqueue for notifyid %lu\n",
+				notifyid);
+	}
+}
+
+static void amd_bram_rproc_rx_callback(struct mbox_client *client, void *msg)
+{
+	struct amd_bram_rproc *priv =
+		container_of(client, struct amd_bram_rproc, rx_client);
+	struct zynqmp_ipi_message *mb_msg = msg;
+	size_t offset;
+	u32 notifyid;
+
+	if (!msg) {
+		dev_warn_ratelimited(priv->dev,
+				     "received mailbox notification without data\n");
+		return;
+	}
+
+	/* Each notification is one 32-bit notify ID. */
+	if (!mb_msg->len || mb_msg->len % sizeof(u32)) {
+		dev_warn_ratelimited(priv->dev,
+				     "malformed mailbox notification: %zu bytes\n",
+				     mb_msg->len);
+		return;
+	}
+
+	for (offset = 0; offset < mb_msg->len; offset += sizeof(u32)) {
+		memcpy(&notifyid, &mb_msg->data[offset], sizeof(notifyid));
+		if (notifyid >= ARRAY_SIZE(priv->notify_msgs)) {
+			dev_warn_ratelimited(priv->dev, "invalid notify id %u\n",
+					     notifyid);
+			continue;
+		}
+
+		set_bit(notifyid, priv->pending_notify_ids);
+	}
+
+	schedule_work(&priv->rx_work);
+}
+
+static void amd_bram_rproc_kick(struct rproc *rproc, int vqid)
+{
+	struct amd_bram_rproc *priv = rproc->priv;
+	struct zynqmp_ipi_message *msg;
+	int ret;
+
+	if (!priv->tx_chan) {
+		dev_warn_ratelimited(priv->dev,
+				     "cannot kick vq%d: no TX mailbox\n", vqid);
+		return;
+	}
+
+	if (vqid < 0 || vqid >= ARRAY_SIZE(priv->notify_msgs)) {
+		dev_err_ratelimited(priv->dev, "invalid notify id %d\n", vqid);
+		return;
+	}
+
+	/*
+	 * The mailbox core queues message pointers. Keep each notification word
+	 * in persistent storage until the controller reports TX completion.
+	 */
+	msg = priv->notify_msgs[vqid];
+	msg->len = sizeof(u32);
+	memcpy(msg->data, &vqid, sizeof(u32));
+	/* Order writes to WC vring mappings before notifying the remote. */
+	wmb();
+	ret = mbox_send_message(priv->tx_chan, msg);
+	if (ret < 0)
+		dev_err_ratelimited(priv->dev, "failed to kick vq%d: %d\n",
+				    vqid, ret);
+}
+
+static void amd_bram_rproc_free_mboxes(struct amd_bram_rproc *priv)
+{
+	if (priv->rx_chan) {
+		mbox_free_channel(priv->rx_chan);
+		priv->rx_chan = NULL;
+	}
+	if (priv->tx_chan) {
+		mbox_free_channel(priv->tx_chan);
+		priv->tx_chan = NULL;
+	}
+}
+
+static int amd_bram_rproc_request_mboxes(struct amd_bram_rproc *priv)
+{
+	int ret;
+
+	/* Without mailboxes the driver supports load, start, and stop only. */
+	if (!of_property_present(priv->dev->of_node, "mboxes"))
+		return 0;
+
+	priv->tx_chan = mbox_request_channel_byname(&priv->tx_client, "tx");
+	if (IS_ERR(priv->tx_chan)) {
+		ret = PTR_ERR(priv->tx_chan);
+		priv->tx_chan = NULL;
+		dev_err(priv->dev, "failed to request tx mailbox: %d\n", ret);
+		return ret;
+	}
+
+	priv->rx_chan = mbox_request_channel_byname(&priv->rx_client, "rx");
+	if (IS_ERR(priv->rx_chan)) {
+		ret = PTR_ERR(priv->rx_chan);
+		priv->rx_chan = NULL;
+		mbox_free_channel(priv->tx_chan);
+		priv->tx_chan = NULL;
+		dev_err(priv->dev, "failed to request rx mailbox: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+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));
+		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 +279,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);
 }
 
 static int amd_bram_rproc_start(struct rproc *rproc)
@@ -111,10 +310,17 @@ static int amd_bram_rproc_start(struct rproc *rproc)
 	/* Hold reset asserted long enough to provide a clean reset pulse. */
 	usleep_range(10, 20);
 
+	ret = amd_bram_rproc_request_mboxes(priv);
+	if (ret) {
+		clk_disable_unprepare(priv->clk);
+		return ret;
+	}
+
 	/* Deassert reset and let the processor run. */
 	ret = gpiod_direction_output(priv->reset, 0);
 	if (ret) {
 		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
+		amd_bram_rproc_free_mboxes(priv);
 		clk_disable_unprepare(priv->clk);
 		return ret;
 	}
@@ -133,6 +339,10 @@ 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));
+	amd_bram_rproc_free_mboxes(priv);
 
 	/* Disable clock after asserting reset */
 	clk_disable_unprepare(priv->clk);
@@ -156,6 +366,8 @@ static const struct rproc_ops amd_bram_rproc_ops = {
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
 	.parse_fw	= amd_bram_rproc_parse_fw,
+	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.kick		= amd_bram_rproc_kick,
 };
 
 static int amd_bram_rproc_probe(struct platform_device *pdev)
@@ -165,6 +377,7 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 	const char *fw_name = NULL;
 	struct rproc *rproc;
 	int ret;
+	int i;
 
 	ret = rproc_of_parse_firmware(dev, 0, &fw_name);
 	if (ret < 0 && ret != -EINVAL)
@@ -178,6 +391,8 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 
 	priv = rproc->priv;
 	priv->dev = dev;
+	priv->rproc = rproc;
+	INIT_WORK(&priv->rx_work, amd_bram_rproc_rx_work);
 
 	/* Get the processor clock */
 	priv->clk = devm_clk_get(dev, NULL);
@@ -195,6 +410,20 @@ static int amd_bram_rproc_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(priv->reset),
 				     "failed to get reset gpio\n");
 
+	priv->tx_client.dev = dev;
+	priv->tx_client.tx_block = false;
+	priv->tx_client.knows_txdone = false;
+	priv->rx_client.dev = dev;
+	priv->rx_client.rx_callback = amd_bram_rproc_rx_callback;
+	for (i = 0; i < ARRAY_SIZE(priv->notify_msgs); i++) {
+		priv->notify_msgs[i] =
+			devm_kzalloc(dev, struct_size(priv->notify_msgs[i],
+						      data, sizeof(u32)),
+				     GFP_KERNEL);
+		if (!priv->notify_msgs[i])
+			return -ENOMEM;
+	}
+
 	rproc->auto_boot = false;
 
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] remoteproc: UAF due to flawed rx_work teardown and early returns
- [Medium] remoteproc: memory leak of carveouts on prepare() failure
--

commit ef18a056105c5574bbf373609a1d2b4aa260576f
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. It registers named reserved-memory carveouts and maps them for
virtqueues. It also introduces mailbox support to send and receive
virtqueue notification IDs.

> diff --git a/drivers/remoteproc/amd_mbv_bram_rproc.c b/drivers/remoteproc/amd_mbv_bram_rproc.c
> index da621f62f7df5..65d2c3946325e 100644
> --- a/drivers/remoteproc/amd_mbv_bram_rproc.c
> +++ b/drivers/remoteproc/amd_mbv_bram_rproc.c
[ ... ]
> +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);

[Severity: Medium]
In amd_bram_rproc_prepare(), resources are added to rproc->carveouts and
rproc->dump_segments before returning a call to this function.

If an error occurs in this loop, the function returns the error early.

The remoteproc core's rproc_fw_boot() handles a failure from ops->prepare
by explicitly bypassing the clean_up_resources label, which means
rproc_resource_cleanup() is never called.

Does this permanently leak any rproc_mem_entry and rproc_dump_segment
objects added before the failure occurs? Retrying the boot might also append
duplicate entries to the lists. Shouldn't the driver clean up its own
carveouts on error, or rely on rproc_resource_cleanup() before returning?

[ ... ]
> @@ -80,7 +279,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);
>  }
>  
>  static int amd_bram_rproc_start(struct rproc *rproc)
> @@ -111,10 +310,17 @@ static int amd_bram_rproc_start(struct rproc *rproc)
>  	/* Hold reset asserted long enough to provide a clean reset pulse. */
>  	usleep_range(10, 20);
>  
> +	ret = amd_bram_rproc_request_mboxes(priv);
> +	if (ret) {
> +		clk_disable_unprepare(priv->clk);
> +		return ret;
> +	}
> +
>  	/* Deassert reset and let the processor run. */
>  	ret = gpiod_direction_output(priv->reset, 0);
>  	if (ret) {
>  		dev_err(priv->dev, "failed to deassert reset: %d\n", ret);
> +		amd_bram_rproc_free_mboxes(priv);
>  		clk_disable_unprepare(priv->clk);
>  		return ret;
>  	}

[Severity: High]
If gpiod_direction_output() fails here, amd_bram_rproc_free_mboxes() is
called but cancel_work_sync(&priv->rx_work) is missing.

Could any work scheduled by a mailbox interrupt between request and free
remain queued? If it executes later, could it access the freed priv pointer
and cause a use-after-free?

[ ... ]
> @@ -133,6 +339,10 @@ static int amd_bram_rproc_stop(struct rproc *rproc)
>  		dev_err(priv->dev, "failed to assert reset: %d\n", ret);
>  		return ret;
>  	}

[Severity: High]
If gpiod_direction_output() fails, this returns early without cancelling the
workqueue or freeing the mailbox channels.

Since the core rproc_del() ignores the stop error and frees the rproc and
priv structs, wouldn't this leave active mailboxes and workqueues pointing
to freed memory?

> +	cancel_work_sync(&priv->rx_work);
> +	bitmap_zero(priv->pending_notify_ids,
> +		    ARRAY_SIZE(priv->notify_msgs));
> +	amd_bram_rproc_free_mboxes(priv);
>  
>  	/* Disable clock after asserting reset */
>  	clk_disable_unprepare(priv->clk);

[Severity: High]
Since cancel_work_sync() is called before freeing the mailboxes, the
mailbox interrupt is still active.

Could an incoming notification trigger amd_bram_rproc_rx_callback() and
reschedule the work right after it was cancelled, leading to a
use-after-free on driver unbind?

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