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
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 <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> Reviewed by: Hemant Kumar <[email protected]>
>
Hi Mani,
On Mon, 2 May 2022 at 12:42, Manivannan Sadhasivam
<[email protected]> 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 <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> 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
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
> <[email protected]> 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 <[email protected]>
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > 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
On Wed, May 04, 2022 at 11:25:33AM +0200, Loic Poulain wrote:
> On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
> <[email protected]> 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
> > > <[email protected]> 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
On Wed, 4 May 2022 at 10:17, Manivannan Sadhasivam
<[email protected]> 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
> > <[email protected]> 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
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
>>> <[email protected]> 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
>>>>> <[email protected]> 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
Hi Hemant,
On Fri, 6 May 2022 at 20:02, Hemant Kumar <[email protected]> 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
> >>> <[email protected]> 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
> >>>>> <[email protected]> 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