On 2017/12/23 0:37, Jason Gunthorpe wrote:
> On Tue, Nov 14, 2017 at 05:26:15PM +0800, Yixian Liu wrote:
>> This patch-set refactor eq code for hip06 and add eq
>> support for hip08.
>>
>> Yixian Liu (2):
>> RDMA/hns: Refactor eq code for hip06
>> RDMA/hns: Add eq support of hip08
>
> I read this this whole giant patch and will apply it to for-next.
>
> I did notice some existing bothersome things:
>
> 1) Bad commenting around all memory barriers:
>
> /* Memory barrier */
> rmb();
>
> Everyone knows that rmb/wmb/mb are memory barriers.
> If you use these calls you *MUST* have a comment explaining
> what the locking situation is. Explain where the *OTHER SIDE* of
> the barrier is.
>
> 2) This construct
>
> static inline void hns_roce_write64_k(__be32 val[2], void __iomem *dest)
> {
> __raw_writeq(*(u64 *) val, dest);
> }
>
> static void set_eq_cons_index_v2(struct hns_roce_eq *eq)
> {
> u32 doorbell[2];
> [..]
> hns_roce_write64_k(doorbell, eq->doorbell);
>
> Is not OK. doorbell is technically not guarenteed to be 64 bit
> aligned, and the deref *(u64 *) requires 64 bit alignment in the
> kernel.
>
> I purged this stupid array thing out of the mellanox drivers in
> userspace some time ago, you might do the same, or minimually a
> union is needed:
>
> union doorbell {
> u64 doorbell64;
> u32 doorbell[2];
> }
>
> Plese consider sending followup patches.
>
Thanks! I will send out bugfix patches according to your suggestions.
> Jason
>
>
--
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
