Re: [PATCH v2] bus: mhi: Ensure correct ring update ordering with memory barrier

[Jeffrey Hugo <jhugo@codeaurora.org>]

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.