| Message ID | 20220502104144.91806-6-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] bus: mhi: host: Rename process_db callback to ring_db | expand |
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
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
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,
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(-)