| Message ID | 1606403201-5656-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Accepted |
| Commit | f49b6aeb5c45dea3a1b6ee6a842599147dfd5929 |
| Headers | show |
| Series | [v2] bus: mhi: Ensure correct ring update ordering with memory barrier | expand |
On 11/26/2020 8:06 AM, Loic Poulain wrote: > The ring element data, though being part of coherent memory, still need > to be performed before updating the ring context to point to this new > element. That can be guaranteed with a memory barrier (dma_wmb). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > v2: fix comment style > > drivers/bus/mhi/core/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 67188ea..ea39df0 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, > dma_addr_t db; > > 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 = db; > + > mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, > ring->db_addr, db); > } > Do we care about the ordering between updating ctxt_wp and the doorbell? As far as I am aware common device implementations only use the value from the doorbell, but I wonder if that changes, could we run into issues? -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: > The ring element data, though being part of coherent memory, still need > to be performed before updating the ring context to point to this new > element. That can be guaranteed with a memory barrier (dma_wmb). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > v2: fix comment style > > drivers/bus/mhi/core/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 67188ea..ea39df0 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, > dma_addr_t db; > > 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 = db; As Jeff pointed out, the barrier should come after updating ctxt_wp. Thanks, Mani > + > mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, > ring->db_addr, db); > } > -- > 2.7.4 >
On Sat, 28 Nov 2020 at 07:03, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: > > The ring element data, though being part of coherent memory, still need > > to be performed before updating the ring context to point to this new > > element. That can be guaranteed with a memory barrier (dma_wmb). > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > --- > > v2: fix comment style > > > > drivers/bus/mhi/core/main.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > > index 67188ea..ea39df0 100644 > > --- a/drivers/bus/mhi/core/main.c > > +++ b/drivers/bus/mhi/core/main.c > > @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, > > dma_addr_t db; > > > > 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 = db; > > As Jeff pointed out, the barrier should come after updating ctxt_wp. Actually, device can poll for the write pointer (e.g. in burst mode), so we need to be sure the element data are written before writing this write pointer (since it can be accessed at any time on device side, not only after doorbell). I think that what jeff pointed is that we also need to ensure that write pointer is also updated before we ring the doorbell (doorbell mode), but this is implicitly done by the MMIO writing (using writeX()) of the doorbell register.(cf https://www.kernel.org/doc/Documentation/memory-barriers.txt). Regards, Loic > > Thanks, > Mani > > > + > > mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, > > ring->db_addr, db); > > } > > -- > > 2.7.4 > >
On 11/27/2020 11:03 PM, Manivannan Sadhasivam wrote: > On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: >> The ring element data, though being part of coherent memory, still need >> to be performed before updating the ring context to point to this new >> element. That can be guaranteed with a memory barrier (dma_wmb). >> >> Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> --- >> v2: fix comment style >> >> drivers/bus/mhi/core/main.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> index 67188ea..ea39df0 100644 >> --- a/drivers/bus/mhi/core/main.c >> +++ b/drivers/bus/mhi/core/main.c >> @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, >> dma_addr_t db; >> >> 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 = db; > > As Jeff pointed out, the barrier should come after updating ctxt_wp. Actually, you potentially need both. The write to the ring element needs to hit the memory before the content write pointer is updated, since the context write pointer is making the ring element "visible" to the device. Then the context write pointer needs to hit memory before the doorbell is updated since the doorbell makes the pointer "visible" to the device. -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On 11/30/2020 1:18 AM, Loic Poulain wrote: > On Sat, 28 Nov 2020 at 07:03, Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> wrote: >> >> On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: >>> The ring element data, though being part of coherent memory, still need >>> to be performed before updating the ring context to point to this new >>> element. That can be guaranteed with a memory barrier (dma_wmb). >>> >>> Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >>> --- >>> v2: fix comment style >>> >>> drivers/bus/mhi/core/main.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >>> index 67188ea..ea39df0 100644 >>> --- a/drivers/bus/mhi/core/main.c >>> +++ b/drivers/bus/mhi/core/main.c >>> @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, >>> dma_addr_t db; >>> >>> 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 = db; >> >> As Jeff pointed out, the barrier should come after updating ctxt_wp. > > Actually, device can poll for the write pointer (e.g. in burst mode), > so we need to be sure the element data are written before writing this > write pointer (since it can be accessed at any time on device side, > not only after doorbell). > > I think that what jeff pointed is that we also need to ensure that > write pointer is also updated before we ring the doorbell (doorbell > mode), but this is implicitly done by the MMIO writing (using > writeX()) of the doorbell register.(cf > https://www.kernel.org/doc/Documentation/memory-barriers.txt). If we are using the version of the writeX API that includes a barrier, then that would be sufficient. -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: > The ring element data, though being part of coherent memory, still need > to be performed before updating the ring context to point to this new > element. That can be guaranteed with a memory barrier (dma_wmb). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Thanks, Mani > --- > v2: fix comment style > > drivers/bus/mhi/core/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 67188ea..ea39df0 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, > dma_addr_t db; > > 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 = db; > + > mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, > ring->db_addr, db); > } > -- > 2.7.4 >
On Thu, Nov 26, 2020 at 04:06:41PM +0100, Loic Poulain wrote: > The ring element data, though being part of coherent memory, still need > to be performed before updating the ring context to point to this new > element. That can be guaranteed with a memory barrier (dma_wmb). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Applied to mhi-next! Thanks, Mani > --- > v2: fix comment style > > drivers/bus/mhi/core/main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 67188ea..ea39df0 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, > dma_addr_t db; > > 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 = db; > + > mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, > ring->db_addr, db); > } > -- > 2.7.4 >
Hello: This patch was applied to qcom/linux.git (refs/heads/for-next): On Thu, 26 Nov 2020 16:06:41 +0100 you wrote: > The ring element data, though being part of coherent memory, still need > to be performed before updating the ring context to point to this new > element. That can be guaranteed with a memory barrier (dma_wmb). > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > v2: fix comment style > > [...] Here is the summary with links: - [v2] bus: mhi: Ensure correct ring update ordering with memory barrier https://git.kernel.org/qcom/c/f49b6aeb5c45 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 67188ea..ea39df0 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -111,7 +111,14 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl, dma_addr_t db; 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 = db; + mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg, ring->db_addr, db); }
The ring element data, though being part of coherent memory, still need to be performed before updating the ring context to point to this new element. That can be guaranteed with a memory barrier (dma_wmb). Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- v2: fix comment style drivers/bus/mhi/core/main.c | 7 +++++++ 1 file changed, 7 insertions(+) -- 2.7.4