On 2017/12/28 23:55, Jason Gunthorpe wrote:
> On Thu, Dec 28, 2017 at 10:18:02PM +0800, Yixian Liu wrote:
>> This patch adds more detailed comments when we call the
>> memory barrier function, such as rmb, wmb and mb. Three
>> mb() callers are deleted since they are unnecessary.
>>
>> v3:
>> 1. Subsitute rmb() with dma_rmb() according to Jason's comment,
>> since using dma_rmb() is enough.
>
> This is such a big improvement, thanks
>
>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>> index 6100ace..47ef90b 100644
>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>> @@ -3963,7 +3963,10 @@ static void set_eq_cons_index_v1(struct hns_roce_eq *eq, int req_not)
>> {
>> roce_raw_write((eq->cons_index & HNS_ROCE_V1_CONS_IDX_M) |
>> (req_not << eq->log_entries), eq->doorbell);
>> - /* Memory barrier */
>> +
>> + /* Make sure we update the consumer index of EQ before
>> + * accessing it later
>> + */
>> mb();
>> }
>
> This (and the other one below like it) is the only one that puzzles
> me..
>
> What are you trying to accomplish here? It looks like cons_index is
> the tail pointer for a ring that the device DMA's into? And the
> dma_rmb seems properly placed, so no barrier should be needed for DMA
> when updating the tail pointer.
>
> Is the barrier attempting to protect eq->cons_index in system memory?
> If yes, shouldn't it be an atomic and/or locked?
>
> Jason
>
You are right, here it write to the device register, so there is no need
to put a barrier there. I will eliminate them in patch v4.
Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
