On Thu, 1 Oct 2026 at 10:51, Shah, Tanmay <[email protected]> wrote:
>
>
>
> On 9/30/2026 11:27 AM, Mathieu Poirier wrote:
> > Hi,
> >
> > On Thu, Sep 24, 2026 at 01:34:09PM -0700, Tanmay Shah wrote:
> >> On AMD-Xilinx platforms cortex-A and cortex-R can be configured as
> >> separate subsystems. In this case, both cores can boot independent of
> >> each other. This is platform management firmware configuration to manage
> >
> > I'm not sure to understand what the above sentence adds to the changelog. I
> > suggest either reworking or removing.
> >
>
> Ack, I will remove it.
>
> >> cores. In such a configuration, if Linux went through an uncontrolled
> >> reboot during active rpmsg communication, then during next boot it can
> >> find rpmsg virtio status not in the reset state. In such case it is
> >> important to reset the virtio status during attach callback and wait
> >> for the remote to handle virtio device reset. After reset, the remote
> >> is expected to generate the notification to the host or the host will
> >> eventually timeout and continue the normal boot flow.
> >>
> >> Assisted-by: LLM
> >> Signed-off-by: Tanmay Shah <[email protected]>
> >> ---
> >> drivers/remoteproc/xlnx_r5_remoteproc.c | 74 +++++++++++++++++++++++++
> >> 1 file changed, 74 insertions(+)
> >>
> >> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> index 630621288430..6e7e2a5ea83c 100644
> >> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >> @@ -6,6 +6,7 @@
> >>
> >> #include <linux/dma-mapping.h>
> >> #include <linux/firmware/xlnx-zynqmp.h>
> >> +#include <linux/jiffies.h>
> >> #include <linux/kernel.h>
> >> #include <linux/mailbox_client.h>
> >> #include <linux/mailbox/zynqmp-ipi-message.h>
> >> @@ -15,6 +16,7 @@
> >> #include <linux/of_reserved_mem.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/remoteproc.h>
> >> +#include <linux/wait.h>
> >>
> >> #include "remoteproc_internal.h"
> >>
> >> @@ -33,6 +35,8 @@
> >> #define RSC_TBL_XLNX_MAGIC ((uint32_t)'x' << 24 | (uint32_t)'a' << 16 | \
> >> (uint32_t)'m' << 8 | (uint32_t)'p')
> >>
> >> +#define RPROC_ATTACH_TIMEOUT_US (1000 * 1000)
> >> +
> >
> > Please see if you can use a kernel defined time constant instead of minting
> > your
> > own.
> >
>
> Ack.
>
> >> /*
> >> * settings for RPU cluster mode which
> >> * reflects possible values of xlnx,cluster-mode dt-property
> >> @@ -167,6 +171,9 @@ struct xlnx_rproc_crash_report {
> >> * @rsc_tbl_size: resource table size retrieved from remote
> >> * @pm_domain_id: RPU CPU power domain id
> >> * @ipi: pointer to mailbox information
> >> + * @attach_wq: wait queue for attach-time vdev reset acknowledgment
> >
> > I don't understand the explanation for @attach_wq - please rework.
> >
> >> + * @waiting_for_attach_ack: whether attach is waiting for remote interrupt
> >> + * @attach_ack: remote interrupt observed while attach wait is active
> >> */
> >> struct zynqmp_r5_core {
> >> struct xlnx_rproc_crash_report *crash_report;
> >> @@ -181,6 +188,9 @@ struct zynqmp_r5_core {
> >> u32 rsc_tbl_size;
> >> u32 pm_domain_id;
> >> struct mbox_info *ipi;
> >> + wait_queue_head_t attach_wq;
> >> + bool waiting_for_attach_ack;
> >> + bool attach_ack;
> >> };
> >>
> >> /**
> >> @@ -270,10 +280,17 @@ static void handle_event_notified(struct work_struct
> >> *work)
> >> static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
> >> {
> >> struct zynqmp_ipi_message *ipi_msg, *buf_msg;
> >> + struct zynqmp_r5_core *r5_core;
> >> struct mbox_info *ipi;
> >> size_t len;
> >>
> >> ipi = container_of(cl, struct mbox_info, mbox_cl);
> >> + r5_core = ipi->r5_core;
> >
> > Is there really a chance that ipi->r5_core be NULL?
> >
>
> Recently, INIT_WORK was moved before
> mbox_request_channel_byname(mbox_cl, "rx"), In this case, if interrupt
> occurs between requesting "rx" channel, and assigning r5_cores to ipi,
> then r5_core can be NULL.
>
> >> +
> >> + if (r5_core && READ_ONCE(r5_core->waiting_for_attach_ack)) {
> >> + WRITE_ONCE(r5_core->attach_ack, true);
> >
> > Why use READ_ONCE/WRITE_ONCE here - what does it give you?
> >
>
> I think this was added by AI agent, and I think it is to maintain atomic
> nature of variable access. But if you prefer to protect these variables
> via locks I will do that. These variables are shared between attach()
> context and IPI interrupt, so I think they should be protected somehow.
>
READ_ONCE/WRITE_ONCE don't guard against concurrency, only against
access reordering.
> >> + wake_up(&r5_core->attach_wq);
> >> + }
> >
> > If @rsc->status has been set to 0 in zynqmp_r5_attach() and an IPI is
> > received
> > before ->kick(), the core may erroneously think the remote processor is
> > acknowleging the reset.
> >
>
> Ack. Probably need to set these flags after kick(). I will do that.
>
> >>
> >> /* copy data from ipi buffer to r5_core if IPI is buffered. */
> >> ipi_msg = (struct zynqmp_ipi_message *)msg;
> >> @@ -820,6 +837,62 @@ static int zynqmp_r5_get_rsc_table_va(struct
> >> zynqmp_r5_core *r5_core)
> >>
> >> static int zynqmp_r5_attach(struct rproc *rproc)
> >> {
> >> + struct zynqmp_r5_core *r5_core = rproc->priv;
> >> + struct device *dev = &rproc->dev;
> >> + bool wait_for_remote = false;
> >> + struct fw_rsc_vdev *rsc;
> >> + struct fw_rsc_hdr *hdr;
> >> + int i, offset, avail;
> >> + long time_left;
> >> +
> >> + if (!rproc->table_ptr)
> >> + goto attach_success;
> >> +
> >> + for (i = 0; i < rproc->table_ptr->num; i++) {
> >> + offset = rproc->table_ptr->offset[i];
> >> + hdr = (void *)rproc->table_ptr + offset;
> >> + avail = rproc->table_sz - offset - sizeof(*hdr);
> >> + rsc = (void *)hdr + sizeof(*hdr);
> >> +
> >> + /* make sure table isn't truncated */
> >> + if (avail < 0) {
> >> + dev_err(dev, "rsc table is truncated\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (hdr->type != RSC_VDEV)
> >> + continue;
> >> +
> >> + /*
> >> + * reset vdev status, in case previous run didn't leave it in
> >> + * a clean state.
> >> + */
> >> + if (rsc->status) {
> >> + rsc->status = 0;
> >> + wait_for_remote = true;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (wait_for_remote) {
> >> + WRITE_ONCE(r5_core->attach_ack, false);
> >> + WRITE_ONCE(r5_core->waiting_for_attach_ack, true);
> >> + }
> >
> > Again, I would like to understand the motivation behind using WRITE_ONCE()
> > here... I just don't see what kind of re-ordering issue you need to guard
> > against.
> >
>
> Ack. I will remove and introduce locks if that plan works.
>
> >> +
> >> + /* kick remote to notify about attach */
> >> + rproc->ops->kick(rproc, 0);
> >
> > Will older FW be able to deal with this properly?
> >
This question hasn't been answered.
> >> +
> >> + if (wait_for_remote) {
> >> + time_left = wait_event_timeout(r5_core->attach_wq,
> >> + READ_ONCE(r5_core->attach_ack),
> >> +
> >> usecs_to_jiffies(RPROC_ATTACH_TIMEOUT_US));
> >
> > The condition where the driver is removed or the remoteproc shut down needs
> > also
> > needs to be handled as a break out condition.
> >
>
> So this whole feature will be helpful only if linux gets rebooted
> without proper cleanup. If driver is removed, it will call detach() via
> zynqmp_cluster_exit(). If machine goes throug 'reboot' command, then the
> driver has 'shutdown' callback registered which will call 'detach()'
> operation. In these cases the remoteproc reset is guranteed and the
> status will be 0 on next boot.
>
> But if linux didn't get chance to execute above callbacks, only then the
> status will be left non-zero on reboot.
>
What if you insmod the driver after an unexpected reboot and then
rmmod the driver while waiting for the remote processor to come back
up?
> > Thanks,
> > Mathieu
> >
> >> + WRITE_ONCE(r5_core->waiting_for_attach_ack, false);
> >> +
> >> + if (!time_left)
> >> + dev_warn(dev, "timeout waiting for remote vdev reset
> >> ack\n");
> >> + }
> >> +
> >> +attach_success:
> >> dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
> >>
> >> return 0;
> >> @@ -920,6 +993,7 @@ static struct zynqmp_r5_core
> >> *zynqmp_r5_alloc_rproc_core(struct device *cdev)
> >> r5_core = r5_rproc->priv;
> >> r5_core->dev = cdev;
> >> r5_core->np = dev_of_node(cdev);
> >> + init_waitqueue_head(&r5_core->attach_wq);
> >> if (!r5_core->np) {
> >> dev_err(cdev, "can't get device node for r5 core\n");
> >> ret = -EINVAL;
> >>
> >> base-commit: 5f639b3018c0026a5341949724b4b921cf3a3d5d
> >> --
> >> 2.43.0
> >>
>
