Hi Sunil,
> -----Original Message-----
> From: dev <dev-boun...@dpdk.org> On Behalf Of Sunil Kumar Kori
> Sent: Monday, December 16, 2019 6:40 PM
> To: jer...@marvell.com; Nithin Dabilpuram <ndabilpu...@marvell.com>;
> Vamsi Attunuru <vattun...@marvell.com>
> Cc: dev@dpdk.org; Sunil Kumar Kori <sk...@marvell.com>; Harman Kalra
> <hka...@marvell.com>
> Subject: [dpdk-dev] [PATCH v3 2/2] common/octeontx2: add polling based
> response mbox message
>
> Currently otx2_mbox_get_rsp_xxx get response once AF driver
> interrupts after completion. But this funciton will get into
> deadlock if called in another interrupt context.
>
> To avoid it, implemented another version of this function which polls
> on dedicated memory for a given timeout.
>
> Also after clearing interrupt, there could UP messages available for
> processing. So irq handler must check mbox messages.
>
> Signed-off-by: Sunil Kumar Kori <sk...@marvell.com>
> Signed-off-by: Harman Kalra <hka...@marvell.com>
> ---
> v3:
> - Remove experimental tag as API is defined static.
> - Merge all changes to single patch.
> v2:
> - Included Makefile and meson build changes.
> - Rebased patch on 19.11-rc4
>
> drivers/common/octeontx2/otx2_dev.c | 41 ++++++++++----------
> drivers/common/octeontx2/otx2_mbox.c | 57 +++++++++++++++++++++++---
> --
> drivers/common/octeontx2/otx2_mbox.h | 5 ++-
> 3 files changed, 72 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/common/octeontx2/otx2_dev.c
> b/drivers/common/octeontx2/otx2_dev.c
> index 0fc799e4a..d61c712fa 100644
> --- a/drivers/common/octeontx2/otx2_dev.c
> +++ b/drivers/common/octeontx2/otx2_dev.c
> @@ -577,17 +577,16 @@ otx2_pf_vf_mbox_irq(void *param)
>
> intr = otx2_read64(dev->bar2 + RVU_VF_INT);
> if (intr == 0)
> - return;
> + otx2_base_dbg("Proceeding to check mbox UP messages if
> any");
>
> otx2_write64(intr, dev->bar2 + RVU_VF_INT);
> otx2_base_dbg("Irq 0x%" PRIx64 "(pf:%d,vf:%d)", intr, dev->pf, dev-
> >vf);
> - if (intr) {
> - /* First process all configuration messages */
> - otx2_process_msgs(dev, dev->mbox);
>
> - /* Process Uplink messages */
> - otx2_process_msgs_up(dev, &dev->mbox_up);
> - }
> + /* First process all configuration messages */
> + otx2_process_msgs(dev, dev->mbox);
> +
> + /* Process Uplink messages */
> + otx2_process_msgs_up(dev, &dev->mbox_up);
> }
>
> static void
> @@ -598,18 +597,16 @@ otx2_af_pf_mbox_irq(void *param)
>
> intr = otx2_read64(dev->bar2 + RVU_PF_INT);
> if (intr == 0)
> - return;
> + otx2_base_dbg("Proceeding to check mbox UP messages if
> any");
>
> otx2_write64(intr, dev->bar2 + RVU_PF_INT);
> -
> otx2_base_dbg("Irq 0x%" PRIx64 "(pf:%d,vf:%d)", intr, dev->pf, dev-
> >vf);
> - if (intr) {
> - /* First process all configuration messages */
> - otx2_process_msgs(dev, dev->mbox);
>
> - /* Process Uplink messages */
> - otx2_process_msgs_up(dev, &dev->mbox_up);
> - }
> + /* First process all configuration messages */
> + otx2_process_msgs(dev, dev->mbox);
> +
> + /* Process Uplink messages */
> + otx2_process_msgs_up(dev, &dev->mbox_up);
> }
>
> static int
> @@ -900,6 +897,7 @@ otx2_dev_priv_init(struct rte_pci_device *pci_dev,
> void *otx2_dev)
> {
> int up_direction = MBOX_DIR_PFAF_UP;
> int rc, direction = MBOX_DIR_PFAF;
> + uint64_t intr_offset = RVU_PF_INT;
> struct otx2_dev *dev = otx2_dev;
> uintptr_t bar2, bar4;
> uint64_t bar4_addr;
> @@ -924,15 +922,18 @@ otx2_dev_priv_init(struct rte_pci_device *pci_dev,
> void *otx2_dev)
> if (otx2_dev_is_vf(dev)) {
> direction = MBOX_DIR_VFPF;
> up_direction = MBOX_DIR_VFPF_UP;
> + intr_offset = RVU_VF_INT;
> }
>
> /* Initialize the local mbox */
> - rc = otx2_mbox_init(&dev->mbox_local, bar4, bar2, direction, 1);
> + rc = otx2_mbox_init(&dev->mbox_local, bar4, bar2, direction, 1,
> + intr_offset);
> if (rc)
> goto error;
> dev->mbox = &dev->mbox_local;
>
> - rc = otx2_mbox_init(&dev->mbox_up, bar4, bar2, up_direction, 1);
> + rc = otx2_mbox_init(&dev->mbox_up, bar4, bar2, up_direction, 1,
> + intr_offset);
> if (rc)
> goto error;
>
> @@ -967,13 +968,15 @@ otx2_dev_priv_init(struct rte_pci_device *pci_dev,
> void *otx2_dev)
> }
> /* Init mbox object */
> rc = otx2_mbox_init(&dev->mbox_vfpf, (uintptr_t)hwbase,
> - bar2, MBOX_DIR_PFVF, pci_dev->max_vfs);
> + bar2, MBOX_DIR_PFVF, pci_dev->max_vfs,
> + intr_offset);
> if (rc)
> goto iounmap;
>
> /* PF -> VF UP messages */
> rc = otx2_mbox_init(&dev->mbox_vfpf_up,
> (uintptr_t)hwbase,
> - bar2, MBOX_DIR_PFVF_UP, pci_dev-
> >max_vfs);
> + bar2, MBOX_DIR_PFVF_UP, pci_dev-
> >max_vfs,
> + intr_offset);
> if (rc)
> goto mbox_fini;
> }
> diff --git a/drivers/common/octeontx2/otx2_mbox.c
> b/drivers/common/octeontx2/otx2_mbox.c
> index c359bf42f..f07f07918 100644
> --- a/drivers/common/octeontx2/otx2_mbox.c
> +++ b/drivers/common/octeontx2/otx2_mbox.c
> @@ -11,6 +11,7 @@
> #include <rte_cycles.h>
>
> #include "otx2_mbox.h"
> +#include "otx2_dev.h"
>
> #define RVU_AF_AFPF_MBOX0 (0x02000)
> #define RVU_AF_AFPF_MBOX1 (0x02008)
> @@ -59,12 +60,13 @@ otx2_mbox_reset(struct otx2_mbox *mbox, int devid)
> }
>
> int
> -otx2_mbox_init(struct otx2_mbox *mbox, uintptr_t hwbase,
> - uintptr_t reg_base, int direction, int ndevs)
> +otx2_mbox_init(struct otx2_mbox *mbox, uintptr_t hwbase, uintptr_t
> reg_base,
> + int direction, int ndevs, uint64_t intr_offset)
> {
> struct otx2_mbox_dev *mdev;
> int devid;
>
> + mbox->intr_offset = intr_offset;
> mbox->reg_base = reg_base;
> mbox->hwbase = hwbase;
>
> @@ -230,8 +232,8 @@ otx2_mbox_get_rsp(struct otx2_mbox *mbox, int
> devid, void **msg)
> int rc;
>
> rc = otx2_mbox_wait_for_rsp(mbox, devid);
> - if (rc != 1)
> - return -EIO;
> + if (rc < 0)
> + return rc;
>
> rte_rmb();
>
> @@ -244,6 +246,37 @@ otx2_mbox_get_rsp(struct otx2_mbox *mbox, int
> devid, void **msg)
> return msghdr->rc;
> }
>
> +/**
> + * Polling for given wait time to get mailbox response
> + */
> +static int
> +mbox_poll(struct otx2_mbox *mbox, uint32_t wait)
> +{
> + uint32_t timeout = 0, sleep = 1;
> + uint64_t rsp_reg = 0;
> + uintptr_t reg_addr;
> +
> + reg_addr = mbox->reg_base + mbox->intr_offset;
> + while (!rsp_reg) {
The first iteration of (!rsp_reg) always evaluate to 'true'.
Why not use do .. while to save one iteration?
> + rte_rmb();
Rte_rmb is overkill, how is reg_addr mapped(what is the memory attribute? WC?
cacheable? )
If it is a PCI mmaped region, then rte_io_barrier is okay, I am proposing to
relax the io barrier for aarch64 as a compiler barrier.
http://patches.dpdk.org/patch/61662/
> + rsp_reg = otx2_read64(reg_addr);
> +
> + if (timeout >= wait)
> + return -ETIMEDOUT;
> +
> + rte_delay_us(sleep);
> + timeout += sleep;
> + }
> +
> + /* Clear interrupt */
> + otx2_write64(rsp_reg, reg_addr);
> +
> + /* Reset mbox */
> + otx2_mbox_reset(mbox, 0);
> +
> + return 0;
> +}
> +
> /**
> * @internal
> * Wait and get mailbox response with timeout
> @@ -258,8 +291,8 @@ otx2_mbox_get_rsp_tmo(struct otx2_mbox *mbox,
> int devid, void **msg,
> int rc;
>
> rc = otx2_mbox_wait_for_rsp_tmo(mbox, devid, tmo);
> - if (rc != 1)
> - return -EIO;
> + if (rc < 0)
> + return rc;
>
> rte_rmb();
>
> @@ -321,11 +354,15 @@ otx2_mbox_wait_for_rsp_tmo(struct otx2_mbox
> *mbox, int devid, uint32_t tmo)
> }
>
> /* Wait message */
> - rc = mbox_wait(mbox, devid, tmo);
> - if (rc)
> - return rc;
> + if (rte_thread_is_intr()) {
> + rc = mbox_poll(mbox, tmo);
> + } else {
> + rc = mbox_wait(mbox, devid, tmo);
> + if (!rc)
> + rc = mdev->msgs_acked;
> + }
>
> - return mdev->msgs_acked;
> + return rc;
> }
>
> /**
> diff --git a/drivers/common/octeontx2/otx2_mbox.h
> b/drivers/common/octeontx2/otx2_mbox.h
> index e0e4e2f63..0535cec36 100644
> --- a/drivers/common/octeontx2/otx2_mbox.h
> +++ b/drivers/common/octeontx2/otx2_mbox.h
> @@ -73,6 +73,7 @@ struct otx2_mbox {
> uint16_t tx_size; /* Size of Tx region */
> uint16_t ndevs; /* The number of peers */
> struct otx2_mbox_dev *dev;
> + uint64_t intr_offset; /* Offset to interrupt register */
> };
>
> /* Header which precedes all mbox messages */
> @@ -1562,8 +1563,8 @@ struct tim_enable_rsp {
> const char *otx2_mbox_id2name(uint16_t id);
> int otx2_mbox_id2size(uint16_t id);
> void otx2_mbox_reset(struct otx2_mbox *mbox, int devid);
> -int otx2_mbox_init(struct otx2_mbox *mbox, uintptr_t hwbase,
> - uintptr_t reg_base, int direction, int ndevs);
> +int otx2_mbox_init(struct otx2_mbox *mbox, uintptr_t hwbase, uintptr_t
> reg_base,
> + int direction, int ndevsi, uint64_t intr_offset);
> void otx2_mbox_fini(struct otx2_mbox *mbox);
> void otx2_mbox_msg_send(struct otx2_mbox *mbox, int devid);
> int otx2_mbox_wait_for_rsp(struct otx2_mbox *mbox, int devid);
> --
> 2.17.1
