Re: [PATCH for-next 0/2] Revise eq support for hip06 & hip08

[Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org>]

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.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html