[PATCH 0/5] MHI host cleanups

[PATCH 0/5] MHI host cleanups

[Manivannan Sadhasivam]

Hi,

This series has few cleanups to the MHI host stack. Apart from the cleanups,
patch (5/5) removes the redundant dma_wmb() from mhi_ring_chan_db() function.
I've provided the reasoning in the commit message, if my understanding is wrong
please let me know.

Thanks,
Mani

Manivannan Sadhasivam (5):
  bus: mhi: host: Rename process_db callback to ring_db
  bus: mhi: host: Rename mhi_db_brstmode() and mhi_db_brstmode_disable()
  bus: mhi: host: Use {READ/WITE}_ONCE macros for db_mode and db_val
  bus: mhi: host: Rename parse_{rsc/xfer}_event to
    process{rsc/xfer}_event
  bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

 drivers/bus/mhi/host/init.c     | 12 +++++------
 drivers/bus/mhi/host/internal.h |  6 +++---
 drivers/bus/mhi/host/main.c     | 37 ++++++++++++++++-----------------
 3 files changed, 27 insertions(+), 28 deletions(-)

-- 
2.25.1

[PATCH 1/5] bus: mhi: host: Rename process_db callback to ring_db

[Manivannan Sadhasivam]

"process_db" might be confusing at times as it also implies that the
function is used to process the doorbells from the endpoint device.
So rename it to "ring_db" to make it clear that it is only used to ring
doorbell to the device.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/host/init.c     | 8 ++++----
 drivers/bus/mhi/host/internal.h | 2 +-
 drivers/bus/mhi/host/main.c     | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index e43e2e145871..50d2a1f66e5e 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -669,9 +669,9 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
 			goto error_ev_cfg;
 
 		if (mhi_event->db_cfg.brstmode == MHI_DB_BRST_ENABLE)
-			mhi_event->db_cfg.process_db = mhi_db_brstmode;
+			mhi_event->db_cfg.ring_db = mhi_db_brstmode;
 		else
-			mhi_event->db_cfg.process_db = mhi_db_brstmode_disable;
+			mhi_event->db_cfg.ring_db = mhi_db_brstmode_disable;
 
 		mhi_event->data_type = event_cfg->data_type;
 
@@ -806,9 +806,9 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
 		}
 
 		if (mhi_chan->db_cfg.brstmode == MHI_DB_BRST_ENABLE)
-			mhi_chan->db_cfg.process_db = mhi_db_brstmode;
+			mhi_chan->db_cfg.ring_db = mhi_db_brstmode;
 		else
-			mhi_chan->db_cfg.process_db = mhi_db_brstmode_disable;
+			mhi_chan->db_cfg.ring_db = mhi_db_brstmode_disable;
 
 		mhi_chan->configured = true;
 
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index b10a99f8200c..3305f4d93580 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -142,7 +142,7 @@ struct db_cfg {
 	u32 pollcfg;
 	enum mhi_db_brst_mode brstmode;
 	dma_addr_t db_val;
-	void (*process_db)(struct mhi_controller *mhi_cntrl,
+	void (*ring_db)(struct mhi_controller *mhi_cntrl,
 			   struct db_cfg *db_cfg, void __iomem *io_addr,
 			   dma_addr_t db_val);
 };
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 7da5a16d721b..c46bd2dd546b 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -112,7 +112,7 @@ void mhi_ring_er_db(struct mhi_event *mhi_event)
 {
 	struct mhi_ring *ring = &mhi_event->ring;
 
-	mhi_event->db_cfg.process_db(mhi_event->mhi_cntrl, &mhi_event->db_cfg,
+	mhi_event->db_cfg.ring_db(mhi_event->mhi_cntrl, &mhi_event->db_cfg,
 				     ring->db_addr, le64_to_cpu(*ring->ctxt_wp));
 }
 
@@ -141,7 +141,7 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
 	dma_wmb();
 	*ring->ctxt_wp = cpu_to_le64(db);
 
-	mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg,
+	mhi_chan->db_cfg.ring_db(mhi_cntrl, &mhi_chan->db_cfg,
 				    ring->db_addr, db);
 }
 
-- 
2.25.1

[PATCH 2/5] bus: mhi: host: Rename mhi_db_brstmode() and mhi_db_brstmode_disable()

[Manivannan Sadhasivam]

These functions are ringing the channel and event ring doorbells based on
the doorbell mode configured. So in order to better reflect their
functionality, let's rename them to mhi_ring_db_brstmode() and
mhi_ring_db_no_brstmode().

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/host/init.c     | 8 ++++----
 drivers/bus/mhi/host/internal.h | 4 ++--
 drivers/bus/mhi/host/main.c     | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 50d2a1f66e5e..e095d2999c06 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -669,9 +669,9 @@ static int parse_ev_cfg(struct mhi_controller *mhi_cntrl,
 			goto error_ev_cfg;
 
 		if (mhi_event->db_cfg.brstmode == MHI_DB_BRST_ENABLE)
-			mhi_event->db_cfg.ring_db = mhi_db_brstmode;
+			mhi_event->db_cfg.ring_db = mhi_ring_db_brstmode;
 		else
-			mhi_event->db_cfg.ring_db = mhi_db_brstmode_disable;
+			mhi_event->db_cfg.ring_db = mhi_ring_db_no_brstmode;
 
 		mhi_event->data_type = event_cfg->data_type;
 
@@ -806,9 +806,9 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl,
 		}
 
 		if (mhi_chan->db_cfg.brstmode == MHI_DB_BRST_ENABLE)
-			mhi_chan->db_cfg.ring_db = mhi_db_brstmode;
+			mhi_chan->db_cfg.ring_db = mhi_ring_db_brstmode;
 		else
-			mhi_chan->db_cfg.ring_db = mhi_db_brstmode_disable;
+			mhi_chan->db_cfg.ring_db = mhi_ring_db_no_brstmode;
 
 		mhi_chan->configured = true;
 
diff --git a/drivers/bus/mhi/host/internal.h b/drivers/bus/mhi/host/internal.h
index 3305f4d93580..007c7554439f 100644
--- a/drivers/bus/mhi/host/internal.h
+++ b/drivers/bus/mhi/host/internal.h
@@ -309,9 +309,9 @@ static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl)
 }
 
 /* Register access methods */
-void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
+void mhi_ring_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
 		     void __iomem *db_addr, dma_addr_t db_val);
-void mhi_db_brstmode_disable(struct mhi_controller *mhi_cntrl,
+void mhi_ring_db_no_brstmode(struct mhi_controller *mhi_cntrl,
 			     struct db_cfg *db_mode, void __iomem *db_addr,
 			     dma_addr_t db_val);
 int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index c46bd2dd546b..28b41621e004 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -87,7 +87,7 @@ void mhi_write_db(struct mhi_controller *mhi_cntrl, void __iomem *db_addr,
 	mhi_write_reg(mhi_cntrl, db_addr, 0, lower_32_bits(db_val));
 }
 
-void mhi_db_brstmode(struct mhi_controller *mhi_cntrl,
+void mhi_ring_db_brstmode(struct mhi_controller *mhi_cntrl,
 		     struct db_cfg *db_cfg,
 		     void __iomem *db_addr,
 		     dma_addr_t db_val)
@@ -99,7 +99,7 @@ void mhi_db_brstmode(struct mhi_controller *mhi_cntrl,
 	}
 }
 
-void mhi_db_brstmode_disable(struct mhi_controller *mhi_cntrl,
+void mhi_ring_db_no_brstmode(struct mhi_controller *mhi_cntrl,
 			     struct db_cfg *db_cfg,
 			     void __iomem *db_addr,
 			     dma_addr_t db_val)
-- 
2.25.1

[PATCH 3/5] bus: mhi: host: Use {READ/WITE}_ONCE macros for db_mode and db_val

[Manivannan Sadhasivam]

In order to prevent the compiler from optimizing these fields that could
be used parallely, let's use the {READ/WRITE}_ONCE macros for reading and
updating. Since these fields are defined as bool, let's use the true/false
instead of 0/1.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/host/init.c |  4 ++--
 drivers/bus/mhi/host/main.c | 14 +++++++++-----
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index e095d2999c06..906bdf584860 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -325,7 +325,7 @@ int mhi_init_dev_ctxt(struct mhi_controller *mhi_cntrl)
 
 		er_ctxt->ertype = cpu_to_le32(MHI_ER_TYPE_VALID);
 		er_ctxt->msivec = cpu_to_le32(mhi_event->irq);
-		mhi_event->db_cfg.db_mode = true;
+		WRITE_ONCE(mhi_event->db_cfg.db_mode, true);
 
 		ring->el_size = sizeof(struct mhi_ring_element);
 		ring->len = ring->el_size * ring->elements;
@@ -615,7 +615,7 @@ int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl,
 
 	tre_ring->rp = tre_ring->wp = tre_ring->base;
 	buf_ring->rp = buf_ring->wp = buf_ring->base;
-	mhi_chan->db_cfg.db_mode = 1;
+	WRITE_ONCE(mhi_chan->db_cfg.db_mode, true);
 
 	/* Update to all cores */
 	smp_wmb();
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 28b41621e004..bb3b20207c4e 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -92,10 +92,14 @@ void mhi_ring_db_brstmode(struct mhi_controller *mhi_cntrl,
 		     void __iomem *db_addr,
 		     dma_addr_t db_val)
 {
-	if (db_cfg->db_mode) {
-		db_cfg->db_val = db_val;
+	if (READ_ONCE(db_cfg->db_mode)) {
+		/*
+		 * There is no barrier required here, since both compiler and
+		 * CPU will honor the load/store control dependency.
+		 */
+		WRITE_ONCE(db_cfg->db_val, db_val);
 		mhi_write_db(mhi_cntrl, db_addr, db_val);
-		db_cfg->db_mode = 0;
+		WRITE_ONCE(db_cfg->db_mode, false);
 	}
 }
 
@@ -104,7 +108,7 @@ void mhi_ring_db_no_brstmode(struct mhi_controller *mhi_cntrl,
 			     void __iomem *db_addr,
 			     dma_addr_t db_val)
 {
-	db_cfg->db_val = db_val;
+	WRITE_ONCE(db_cfg->db_val, db_val);
 	mhi_write_db(mhi_cntrl, db_addr, db_val);
 }
 
@@ -665,7 +669,7 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
 	{
 		unsigned long pm_lock_flags;
 
-		mhi_chan->db_cfg.db_mode = 1;
+		WRITE_ONCE(mhi_chan->db_cfg.db_mode, true);
 		read_lock_irqsave(&mhi_cntrl->pm_lock, pm_lock_flags);
 		if (tre_ring->wp != tre_ring->rp &&
 		    MHI_DB_ACCESS_VALID(mhi_cntrl)) {
-- 
2.25.1

[PATCH 4/5] bus: mhi: host: Rename parse_{rsc/xfer}_event to process{rsc/xfer}_event

[Manivannan Sadhasivam]

We are not parsing the event but rather processing it as we received it
from the endpoint device. So rename the functions accordingly.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/host/main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index bb3b20207c4e..966ffc2458b9 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -558,7 +558,7 @@ static void mhi_recycle_ev_ring_element(struct mhi_controller *mhi_cntrl,
 	smp_wmb();
 }
 
-static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
+static int process_xfer_event(struct mhi_controller *mhi_cntrl,
 			    struct mhi_ring_element *event,
 			    struct mhi_chan *mhi_chan)
 {
@@ -693,7 +693,7 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl,
 	return 0;
 }
 
-static int parse_rsc_event(struct mhi_controller *mhi_cntrl,
+static int process_rsc_event(struct mhi_controller *mhi_cntrl,
 			   struct mhi_ring_element *event,
 			   struct mhi_chan *mhi_chan)
 {
@@ -931,7 +931,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
 				mhi_chan = &mhi_cntrl->mhi_chan[chan];
 				if (!mhi_chan->configured)
 					break;
-				parse_xfer_event(mhi_cntrl, local_rp, mhi_chan);
+				process_xfer_event(mhi_cntrl, local_rp, mhi_chan);
 				event_quota--;
 			}
 			break;
@@ -1003,10 +1003,10 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
 			mhi_chan = &mhi_cntrl->mhi_chan[chan];
 
 			if (likely(type == MHI_PKT_TYPE_TX_EVENT)) {
-				parse_xfer_event(mhi_cntrl, local_rp, mhi_chan);
+				process_xfer_event(mhi_cntrl, local_rp, mhi_chan);
 				event_quota--;
 			} else if (type == MHI_PKT_TYPE_RSC_TX_EVENT) {
-				parse_rsc_event(mhi_cntrl, local_rp, mhi_chan);
+				process_rsc_event(mhi_cntrl, local_rp, mhi_chan);
 				event_quota--;
 			}
 		}
-- 
2.25.1

[PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Manivannan Sadhasivam]

The endpoint device will only read the context wp when the host rings
the doorbell. And moreover the doorbell write is using writel(). This
guarantess that the prior writes will be completed before ringing
doorbell.

So there is no need of an additional dma_wmb() to order the coherent
memory writes w.r.t each other. Even if the writes gets reordered, it
won't affect the endpoint device.

Cc: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/bus/mhi/host/main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 966ffc2458b9..6706a82d3aa8 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -138,11 +138,6 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
 
 	db = ring->iommu_base + (ring->wp - ring->base);
 
-	/*
-	 * Writes to the new ring element must be visible to the hardware
-	 * before letting h/w know there is new element to fetch.
-	 */
-	dma_wmb();
 	*ring->ctxt_wp = cpu_to_le64(db);
 
 	mhi_chan->db_cfg.ring_db(mhi_cntrl, &mhi_chan->db_cfg,
-- 
2.25.1

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Hemant Kumar]

On 5/2/2022 3:41 AM, Manivannan Sadhasivam wrote:
> The endpoint device will only read the context wp when the host rings
> the doorbell. And moreover the doorbell write is using writel(). This
> guarantess that the prior writes will be completed before ringing
> doorbell.
>
> So there is no need of an additional dma_wmb() to order the coherent
> memory writes w.r.t each other. Even if the writes gets reordered, it
> won't affect the endpoint device.
>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed by: Hemant Kumar <quic_hemantk@quicinc.com>
>

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Loic Poulain]

Hi Mani,

On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> The endpoint device will only read the context wp when the host rings
> the doorbell.

Are we sure about this statement? what if we update ctxt_wp while the
device is still processing the previous ring? is it going to continue
processing the new ctxt_wp or wait for a new doorbell interrupt? what
about burst mode in which we don't ring at all (ring_db is no-op)?

> And moreover the doorbell write is using writel(). This
> guarantess that the prior writes will be completed before ringing
> doorbell.

Yes but the barrier is to ensure that descriptor/ring content is
updated before we actually pass it to device ownership, it's not about
ordering with the doorbell write, but the memory coherent ones.

>
> So there is no need of an additional dma_wmb() to order the coherent
> memory writes w.r.t each other. Even if the writes gets reordered, it
> won't affect the endpoint device.
>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/bus/mhi/host/main.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 966ffc2458b9..6706a82d3aa8 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -138,11 +138,6 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
>
>         db = ring->iommu_base + (ring->wp - ring->base);
>
> -       /*
> -        * Writes to the new ring element must be visible to the hardware
> -        * before letting h/w know there is new element to fetch.
> -        */
> -       dma_wmb();
>         *ring->ctxt_wp = cpu_to_le64(db);
>
>         mhi_chan->db_cfg.ring_db(mhi_cntrl, &mhi_chan->db_cfg,
> --
> 2.25.1
>

Regards,
Loic

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Manivannan Sadhasivam]

Hi Loic,

On Wed, May 04, 2022 at 09:21:20AM +0200, Loic Poulain wrote:
> Hi Mani,
> 
> On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > The endpoint device will only read the context wp when the host rings
> > the doorbell.
> 
> Are we sure about this statement? what if we update ctxt_wp while the
> device is still processing the previous ring? is it going to continue
> processing the new ctxt_wp or wait for a new doorbell interrupt? what
> about burst mode in which we don't ring at all (ring_db is no-op)?
> 

Good point. I think my statement was misleading. But still this scenario won't
happen as per my undestanding. Please see below.

> > And moreover the doorbell write is using writel(). This
> > guarantess that the prior writes will be completed before ringing
> > doorbell.
> 
> Yes but the barrier is to ensure that descriptor/ring content is
> updated before we actually pass it to device ownership, it's not about
> ordering with the doorbell write, but the memory coherent ones.
> 

I see a clear data dependency between writing the ring element and updating the
context pointer. For instance,

```
struct mhi_ring_element *mhi_tre;

mhi_tre = ring->wp;
/* Populate mhi_tre */
...

/* Increment wp */
ring->wp += el_size;

/* Update ctx wp */
ring->ctx_wp = ring->iommu_base + (ring->wp - ring->base);
```

This is analogous to:

```
Read PTR A;
Update PTR A;
Increment PTR A;
Write PTR A to PTR B;
```

Here, because of the data dependency due to "ring->wp", the CPU or compiler
won't be ordering the instructions. I think that's one of the reason we never
hit any issue due to this.

Thanks,
Mani

> >
> > So there is no need of an additional dma_wmb() to order the coherent
> > memory writes w.r.t each other. Even if the writes gets reordered, it
> > won't affect the endpoint device.
> >
> > Cc: Loic Poulain <loic.poulain@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/bus/mhi/host/main.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> > index 966ffc2458b9..6706a82d3aa8 100644
> > --- a/drivers/bus/mhi/host/main.c
> > +++ b/drivers/bus/mhi/host/main.c
> > @@ -138,11 +138,6 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
> >
> >         db = ring->iommu_base + (ring->wp - ring->base);
> >
> > -       /*
> > -        * Writes to the new ring element must be visible to the hardware
> > -        * before letting h/w know there is new element to fetch.
> > -        */
> > -       dma_wmb();
> >         *ring->ctxt_wp = cpu_to_le64(db);
> >
> >         mhi_chan->db_cfg.ring_db(mhi_cntrl, &mhi_chan->db_cfg,
> > --
> > 2.25.1
> >
> 
> Regards,
> Loic

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Loic Poulain]

On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Hi Loic,
>
> On Wed, May 04, 2022 at 09:21:20AM +0200, Loic Poulain wrote:
> > Hi Mani,
> >
> > On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> wrote:
> > >
> > > The endpoint device will only read the context wp when the host rings
> > > the doorbell.
> >
> > Are we sure about this statement? what if we update ctxt_wp while the
> > device is still processing the previous ring? is it going to continue
> > processing the new ctxt_wp or wait for a new doorbell interrupt? what
> > about burst mode in which we don't ring at all (ring_db is no-op)?
> >
>
> Good point. I think my statement was misleading. But still this scenario won't
> happen as per my undestanding. Please see below.
>
> > > And moreover the doorbell write is using writel(). This
> > > guarantess that the prior writes will be completed before ringing
> > > doorbell.
> >
> > Yes but the barrier is to ensure that descriptor/ring content is
> > updated before we actually pass it to device ownership, it's not about
> > ordering with the doorbell write, but the memory coherent ones.
> >
>
> I see a clear data dependency between writing the ring element and updating the
> context pointer. For instance,
>
> ```
> struct mhi_ring_element *mhi_tre;
>
> mhi_tre = ring->wp;
> /* Populate mhi_tre */
> ...
>
> /* Increment wp */
> ring->wp += el_size;
>
> /* Update ctx wp */
> ring->ctx_wp = ring->iommu_base + (ring->wp - ring->base);
> ```
>
> This is analogous to:
>
> ```
> Read PTR A;
> Update PTR A;
> Increment PTR A;
> Write PTR A to PTR B;
> ```

Interesting point, but shouldn't it be more correct to translate it as:

1. Write PTR A to PTR B (mhi_tre);
2. Update PTR B DATA;
3. Increment PTR A;
4. Write PTR A to PTR C;

In that case, it looks like line 2. has no ordering constraint with 3.
& 4? whereas the following guarantee it:

1. Write PTR A to PTR B (mhi_tre);
2. Update PTR B DATA;
3. Increment PTR A;
dma_wmb()
4. Write PTR A to PTR C;

To be honest, compiler optimization is beyond my knowledge, so I don't
know if a specific compiler arch/version could be able to mess up the
sequence or not. But this pattern is really close to what is described
for dma_wmb() usage in Documentation/memory-barriers.txt. That's why I
challenged this change and would be conservative, keeping the explicit
barrier.

>
> Here, because of the data dependency due to "ring->wp", the CPU or compiler
> won't be ordering the instructions. I think that's one of the reason we never
> hit any issue due to this.

You may be right here about the implicit ordering guarantee... So if
you're sure, I think it would deserve an inline comment to explain why
we don't need a memory barrier as in the 'usual' dma descriptor update
sequences.

Loic

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Manivannan Sadhasivam]

On Wed, May 04, 2022 at 11:25:33AM +0200, Loic Poulain wrote:
> On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> wrote:
> >
> > Hi Loic,
> >
> > On Wed, May 04, 2022 at 09:21:20AM +0200, Loic Poulain wrote:
> > > Hi Mani,
> > >
> > > On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
> > > <manivannan.sadhasivam@linaro.org> wrote:
> > > >
> > > > The endpoint device will only read the context wp when the host rings
> > > > the doorbell.
> > >
> > > Are we sure about this statement? what if we update ctxt_wp while the
> > > device is still processing the previous ring? is it going to continue
> > > processing the new ctxt_wp or wait for a new doorbell interrupt? what
> > > about burst mode in which we don't ring at all (ring_db is no-op)?
> > >
> >
> > Good point. I think my statement was misleading. But still this scenario won't
> > happen as per my undestanding. Please see below.
> >
> > > > And moreover the doorbell write is using writel(). This
> > > > guarantess that the prior writes will be completed before ringing
> > > > doorbell.
> > >
> > > Yes but the barrier is to ensure that descriptor/ring content is
> > > updated before we actually pass it to device ownership, it's not about
> > > ordering with the doorbell write, but the memory coherent ones.
> > >
> >
> > I see a clear data dependency between writing the ring element and updating the
> > context pointer. For instance,
> >
> > ```
> > struct mhi_ring_element *mhi_tre;
> >
> > mhi_tre = ring->wp;
> > /* Populate mhi_tre */
> > ...
> >
> > /* Increment wp */
> > ring->wp += el_size;
> >
> > /* Update ctx wp */
> > ring->ctx_wp = ring->iommu_base + (ring->wp - ring->base);
> > ```
> >
> > This is analogous to:
> >
> > ```
> > Read PTR A;
> > Update PTR A;
> > Increment PTR A;
> > Write PTR A to PTR B;
> > ```
> 
> Interesting point, but shouldn't it be more correct to translate it as:
> 
> 1. Write PTR A to PTR B (mhi_tre);
> 2. Update PTR B DATA;
> 3. Increment PTR A;
> 4. Write PTR A to PTR C;
> 
> In that case, it looks like line 2. has no ordering constraint with 3.
> & 4? whereas the following guarantee it:
> 
> 1. Write PTR A to PTR B (mhi_tre);
> 2. Update PTR B DATA;
> 3. Increment PTR A;
> dma_wmb()
> 4. Write PTR A to PTR C;
> 
> To be honest, compiler optimization is beyond my knowledge, so I don't
> know if a specific compiler arch/version could be able to mess up the
> sequence or not. But this pattern is really close to what is described
> for dma_wmb() usage in Documentation/memory-barriers.txt. That's why I
> challenged this change and would be conservative, keeping the explicit
> barrier.
> 

Hmm. Since I was reading the memory model and going through the MHI code, I
_thought_ that this dma_wmb() is redundant. But I missed the fact that the
updating to memory pointed by "wp" happens implicitly via a pointer. So that
won't qualify as a direct dependency.

> >
> > Here, because of the data dependency due to "ring->wp", the CPU or compiler
> > won't be ordering the instructions. I think that's one of the reason we never
> > hit any issue due to this.
> 
> You may be right here about the implicit ordering guarantee... So if
> you're sure, I think it would deserve an inline comment to explain why
> we don't need a memory barrier as in the 'usual' dma descriptor update
> sequences.
> 

I think the barrier makes sense now. Sorry for the confusion and thanks for the
explanations.

Thanks,
Mani

> Loic

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Hemant Kumar]

Hi Loic,

On 5/6/2022 10:41 AM, Hemant Kumar wrote:
> Hi Loic,
> 
> On 5/4/2022 8:58 AM, Manivannan Sadhasivam wrote:
>> On Wed, May 04, 2022 at 11:25:33AM +0200, Loic Poulain wrote:
>>> On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
>>> <manivannan.sadhasivam@linaro.org>  wrote:
>>>> Hi Loic,
>>>>
>>>> On Wed, May 04, 2022 at 09:21:20AM +0200, Loic Poulain wrote:
>>>>> Hi Mani,
>>>>>
>>>>> On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
>>>>> <manivannan.sadhasivam@linaro.org>  wrote:
>>>>>> The endpoint device will only read the context wp when the host rings
>>>>>> the doorbell.
>>>>> Are we sure about this statement? what if we update ctxt_wp while the
>>>>> device is still processing the previous ring? is it going to continue
>>>>> processing the new ctxt_wp or wait for a new doorbell interrupt? what
>>>>> about burst mode in which we don't ring at all (ring_db is no-op)?
>>>>>
>>>> Good point. I think my statement was misleading. But still this scenario won't
>>>> happen as per my undestanding. Please see below.
>>>>
>>>>>> And moreover the doorbell write is using writel(). This
>>>>>> guarantess that the prior writes will be completed before ringing
>>>>>> doorbell.
>>>>> Yes but the barrier is to ensure that descriptor/ring content is
>>>>> updated before we actually pass it to device ownership, it's not about
>>>>> ordering with the doorbell write, but the memory coherent ones.
>>>>>
>>>> I see a clear data dependency between writing the ring element and updating the
>>>> context pointer. For instance,
>>>>
>>>> ```
>>>> struct mhi_ring_element *mhi_tre;
>>>>
>>>> mhi_tre = ring->wp;
>>>> /* Populate mhi_tre */
>>>> ...
>>>>
>>>> /* Increment wp */
>>>> ring->wp += el_size;
>>>>
>>>> /* Update ctx wp */
>>>> ring->ctx_wp = ring->iommu_base + (ring->wp - ring->base);
>>>> ```
>>>>
>>>> This is analogous to:
>>>>
>>>> ```
>>>> Read PTR A;
>>>> Update PTR A;
>>>> Increment PTR A;
>>>> Write PTR A to PTR B;
>>>> ```
>>> Interesting point, but shouldn't it be more correct to translate it as:
>>>
>>> 1. Write PTR A to PTR B (mhi_tre);
>>> 2. Update PTR B DATA;
>>> 3. Increment PTR A;
>>> 4. Write PTR A to PTR C;
>>>
>>> In that case, it looks like line 2. has no ordering constraint with 3.
>>> & 4? whereas the following guarantee it:
>>>
>>> 1. Write PTR A to PTR B (mhi_tre);
>>> 2. Update PTR B DATA;
>>> 3. Increment PTR A;
>>> dma_wmb()
>>> 4. Write PTR A to PTR C;
>>>
>>> To be honest, compiler optimization is beyond my knowledge, so I don't
>>> know if a specific compiler arch/version could be able to mess up the
>>> sequence or not. But this pattern is really close to what is described
>>> for dma_wmb() usage in Documentation/memory-barriers.txt. That's why I
>>> challenged this change and would be conservative, keeping the explicit
>>> barrier.
>>>
>> Hmm. Since I was reading the memory model and going through the MHI code, I
>> _thought_ that this dma_wmb() is redundant. But I missed the fact that the
>> updating to memory pointed by "wp" happens implicitly via a pointer. So that
>> won't qualify as a direct dependency.
>>
>>>> Here, because of the data dependency due to "ring->wp", the CPU or compiler
>>>> won't be ordering the instructions. I think that's one of the reason we never
>>>> hit any issue due to this.
>>> You may be right here about the implicit ordering guarantee... So if
>>> you're sure, I think it would deserve an inline comment to explain why
>>> we don't need a memory barrier as in the 'usual' dma descriptor update
>>> sequences.
>>>
>> I think the barrier makes sense now. Sorry for the confusion and thanks for the
>> explanations.
>>
>> Thanks,
>> Mani
>>
>>> Loic
> 
> You made a good point. After following your conversation, in case of 
> burst mode is enabled and currently
> 
> we are in polling mode, does it make sense to move dma_wmb after 
> updating channel WP context ?
> 
> DB ring is going to get skipped when we are in pilling mode.
> 
> instead of dma_wmb();
> *ring->ctxt_wp  =  cpu_to_le64(db);
> 
> *ring->ctxt_wp  =  cpu_to_le64(db); dma_wmb();
> 
> Thanks,
> Hemant
> 
i think i spoke too fast. I think we dont need to worry about the 
polling mode as the context_wp update would happen at some point of time 
and that does not require dma_wmb after update context wp.

Thanks,
Hemant

Re: [PATCH 5/5] bus: mhi: host: Remove redundant dma_wmb() before ctx wp update

[Loic Poulain]

Hi Hemant,

On Fri, 6 May 2022 at 20:02, Hemant Kumar <quic_hemantk@quicinc.com> wrote:
>
> Hi Loic,
>
> On 5/6/2022 10:41 AM, Hemant Kumar wrote:
> > Hi Loic,
> >
> > On 5/4/2022 8:58 AM, Manivannan Sadhasivam wrote:
> >> On Wed, May 04, 2022 at 11:25:33AM +0200, Loic Poulain wrote:
> >>> On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
> >>> <manivannan.sadhasivam@linaro.org>  wrote:
> >>>> Hi Loic,
> >>>>
> >>>> On Wed, May 04, 2022 at 09:21:20AM +0200, Loic Poulain wrote:
> >>>>> Hi Mani,
> >>>>>
> >>>>> On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
> >>>>> <manivannan.sadhasivam@linaro.org>  wrote:
> >>>>>> The endpoint device will only read the context wp when the host rings
> >>>>>> the doorbell.
> >>>>> Are we sure about this statement? what if we update ctxt_wp while the
> >>>>> device is still processing the previous ring? is it going to continue
> >>>>> processing the new ctxt_wp or wait for a new doorbell interrupt? what
> >>>>> about burst mode in which we don't ring at all (ring_db is no-op)?
> >>>>>
> >>>> Good point. I think my statement was misleading. But still this scenario won't
> >>>> happen as per my undestanding. Please see below.
> >>>>
> >>>>>> And moreover the doorbell write is using writel(). This
> >>>>>> guarantess that the prior writes will be completed before ringing
> >>>>>> doorbell.
> >>>>> Yes but the barrier is to ensure that descriptor/ring content is
> >>>>> updated before we actually pass it to device ownership, it's not about
> >>>>> ordering with the doorbell write, but the memory coherent ones.
> >>>>>
> >>>> I see a clear data dependency between writing the ring element and updating the
> >>>> context pointer. For instance,
> >>>>
> >>>> ```
> >>>> struct mhi_ring_element *mhi_tre;
> >>>>
> >>>> mhi_tre = ring->wp;
> >>>> /* Populate mhi_tre */
> >>>> ...
> >>>>
> >>>> /* Increment wp */
> >>>> ring->wp += el_size;
> >>>>
> >>>> /* Update ctx wp */
> >>>> ring->ctx_wp = ring->iommu_base + (ring->wp - ring->base);
> >>>> ```
> >>>>
> >>>> This is analogous to:
> >>>>
> >>>> ```
> >>>> Read PTR A;
> >>>> Update PTR A;
> >>>> Increment PTR A;
> >>>> Write PTR A to PTR B;
> >>>> ```
> >>> Interesting point, but shouldn't it be more correct to translate it as:
> >>>
> >>> 1. Write PTR A to PTR B (mhi_tre);
> >>> 2. Update PTR B DATA;
> >>> 3. Increment PTR A;
> >>> 4. Write PTR A to PTR C;
> >>>
> >>> In that case, it looks like line 2. has no ordering constraint with 3.
> >>> & 4? whereas the following guarantee it:
> >>>
> >>> 1. Write PTR A to PTR B (mhi_tre);
> >>> 2. Update PTR B DATA;
> >>> 3. Increment PTR A;
> >>> dma_wmb()
> >>> 4. Write PTR A to PTR C;
> >>>
> >>> To be honest, compiler optimization is beyond my knowledge, so I don't
> >>> know if a specific compiler arch/version could be able to mess up the
> >>> sequence or not. But this pattern is really close to what is described
> >>> for dma_wmb() usage in Documentation/memory-barriers.txt. That's why I
> >>> challenged this change and would be conservative, keeping the explicit
> >>> barrier.
> >>>
> >> Hmm. Since I was reading the memory model and going through the MHI code, I
> >> _thought_ that this dma_wmb() is redundant. But I missed the fact that the
> >> updating to memory pointed by "wp" happens implicitly via a pointer. So that
> >> won't qualify as a direct dependency.
> >>
> >>>> Here, because of the data dependency due to "ring->wp", the CPU or compiler
> >>>> won't be ordering the instructions. I think that's one of the reason we never
> >>>> hit any issue due to this.
> >>> You may be right here about the implicit ordering guarantee... So if
> >>> you're sure, I think it would deserve an inline comment to explain why
> >>> we don't need a memory barrier as in the 'usual' dma descriptor update
> >>> sequences.
> >>>
> >> I think the barrier makes sense now. Sorry for the confusion and thanks for the
> >> explanations.
> >>
> >> Thanks,
> >> Mani
> >>
> >>> Loic
> >
> > You made a good point. After following your conversation, in case of
> > burst mode is enabled and currently
> >
> > we are in polling mode, does it make sense to move dma_wmb after
> > updating channel WP context ?
> >
> > DB ring is going to get skipped when we are in pilling mode.
> >
> > instead of dma_wmb();
> > *ring->ctxt_wp  =  cpu_to_le64(db);
> >
> > *ring->ctxt_wp  =  cpu_to_le64(db); dma_wmb();
> >
> > Thanks,
> > Hemant
> >
> i think i spoke too fast. I think we dont need to worry about the
> polling mode as the context_wp update would happen at some point of time
> and that does not require dma_wmb after update context wp.

Exactly. It's also important to remember that a barrier only ensures
operations ordering and not committing.

Regards,
Loic