DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
From: "Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>
To: Sunil Kumar Kori <skori@marvell.com>,
	"jerinj@marvell.com" <jerinj@marvell.com>,
	Nithin Dabilpuram <ndabilpuram@marvell.com>,
	Vamsi Attunuru <vattunuru@marvell.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Harman Kalra <hkalra@marvell.com>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	nd <nd@arm.com>
Subject: Re: [dpdk-dev] [PATCH v3 2/2] common/octeontx2: add polling based	response mbox message
Date: Tue, 17 Dec 2019 08:02:39 +0000
Message-ID: <VI1PR08MB537696E37E73B088EFD987988F500@VI1PR08MB5376.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <20191216103948.22976-2-skori@marvell.com>

Hi Sunil,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Sunil Kumar Kori
> Sent: Monday, December 16, 2019 6:40 PM
> To: jerinj@marvell.com; Nithin Dabilpuram <ndabilpuram@marvell.com>;
> Vamsi Attunuru <vattunuru@marvell.com>
> Cc: dev@dpdk.org; Sunil Kumar Kori <skori@marvell.com>; Harman Kalra
> <hkalra@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 <skori@marvell.com>
> Signed-off-by: Harman Kalra <hkalra@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


  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-25 11:35 [dpdk-dev] [PATCH 1/5] drivers/octeontx2: allow experimental symbols Sunil Kumar Kori
2019-11-25 11:35 ` [dpdk-dev] [PATCH v3 2/5] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-11-25 11:35 ` [dpdk-dev] [PATCH 3/5] common/octeontx2: add interrupt offset to mbox structure Sunil Kumar Kori
2019-11-25 11:35 ` [dpdk-dev] [PATCH 4/5] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-11-25 11:35 ` [dpdk-dev] [PATCH 5/5] common/octeontx2: enhancing mbox APIs to get response Sunil Kumar Kori
2019-11-26  6:15 ` [dpdk-dev] [PATCH v2 1/5] drivers/octeontx2: allow experimental symbols Sunil Kumar Kori
2019-11-26 16:21   ` Thomas Monjalon
2019-11-27  8:34     ` [dpdk-dev] [EXT] " Sunil Kumar Kori
2019-11-27  9:42       ` Thomas Monjalon
2019-11-27 10:22   ` [dpdk-dev] [PATCH v2 1/4] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-11-27 10:22     ` [dpdk-dev] [PATCH v2 2/4] common/octeontx2: add interrupt offset to mbox structure Sunil Kumar Kori
2019-11-27 10:22     ` [dpdk-dev] [PATCH v2 3/4] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-11-27 10:22     ` [dpdk-dev] [PATCH v2 4/4] drivers/octeontx2: enhancing mbox APIs to get response Sunil Kumar Kori
2019-11-28  0:03     ` [dpdk-dev] [PATCH v2 1/4] eal: add API to check if its interrupt context Stephen Hemminger
2019-11-28  8:10       ` [dpdk-dev] [EXT] " Harman Kalra
2019-11-28 16:45         ` Stephen Hemminger
2019-12-04 16:23           ` Harman Kalra
2019-12-16 10:39     ` [dpdk-dev] [PATCH v3 1/2] " Sunil Kumar Kori
2019-12-16 10:39       ` [dpdk-dev] [PATCH v3 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-12-17  8:02         ` Gavin Hu (Arm Technology China) [this message]
2019-12-17 11:14           ` Jerin Jacob
2019-12-18  2:45             ` Gavin Hu
2019-12-17 10:42         ` [dpdk-dev] [PATCH v4 1/2] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-12-17 10:42           ` [dpdk-dev] [PATCH v4 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-12-17 16:53         ` [dpdk-dev] [PATCH v4 1/2] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-12-17 16:53           ` [dpdk-dev] [PATCH v4 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-12-18  2:54             ` Gavin Hu
2019-12-18  7:07           ` [dpdk-dev] [PATCH v5 1/2] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-12-18  7:07             ` [dpdk-dev] [PATCH v5 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2019-12-20  6:56               ` [dpdk-dev] [PATCH v6 1/2] eal: add API to check if its interrupt context Sunil Kumar Kori
2019-12-20  6:56                 ` [dpdk-dev] [PATCH v6 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2020-01-14  8:41                   ` Jerin Jacob
2020-01-14 10:17                     ` Thomas Monjalon
2020-01-14  9:04                   ` [dpdk-dev] [PATCH v7 1/2] eal: add API to check if its interrupt context Sunil Kumar Kori
2020-01-14  9:04                     ` [dpdk-dev] [PATCH v7 2/2] common/octeontx2: add polling based response mbox message Sunil Kumar Kori
2020-01-21  8:37                       ` Sunil Kumar Kori
2020-02-06 15:35                     ` [dpdk-dev] [PATCH v7 1/2] eal: add API to check if its interrupt context David Marchand
2020-01-14  8:37                 ` [dpdk-dev] [PATCH v6 " Jerin Jacob

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VI1PR08MB537696E37E73B088EFD987988F500@VI1PR08MB5376.eurprd08.prod.outlook.com \
    --to=gavin.hu@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=dev@dpdk.org \
    --cc=hkalra@marvell.com \
    --cc=jerinj@marvell.com \
    --cc=nd@arm.com \
    --cc=ndabilpuram@marvell.com \
    --cc=skori@marvell.com \
    --cc=vattunuru@marvell.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git