Fwd: Re: [PATCH v8 05/22] IB/hns: Add initial profile resource

Fwd: Re: [PATCH v8 05/22] IB/hns: Add initial profile resource

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2015 bytes --]




-------- Forwarded Message --------
Subject: Re: [PATCH v8 05/22] IB/hns: Add initial profile resource
Date: Wed, 25 May 2016 15:00:13 -0400
From: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Organization: Red Hat, Inc.
To: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

On 05/25/2016 11:05 AM, Lijun Ou wrote:
> This patch mainly configured some profile resoure. For example,
> vendor_id, hardware version, and some data structure sizes so on.

> +/* Address shift 32bit with the special hardware address operation of RoCEE */
> +#define ADDR_SHIFT_32				32
> +

When I said you need to document the address shifts, this is not what I
had in mind.  Anyone can tell that this is a 32 bit address shift,
that's obvious.  The question is *why* is it a 32bit shift?


> +				((u64)le32_to_cpu(roce_readl(hr_dev->reg_base +
> +					     ROCEE_SYS_IMAGE_GUID_H_REG)) <<
> +					     ADDR_SHIFT_32);

And here's a usage of it that really doesn't need to be a #define.  If
you have a u64 and you are reading two u32s, then it is OK to shift by
the literal 32.  In fact, when you need an explanation in this case is
if you *don't* shift it by 32.  So this one is OK to use a literal 32
and you don't need a define.

However, in a later patch, you add a shift by 12.  And you add the same
silly comment in front of it.  That does nothing to tell the reader that
the *real* reason you have a fixed ADDR_SHIFT_12 is that you have a
register on this hardware (maybe more than one, I only saw the one) that
assumes the address written to it is shifted as though the system uses a
4K page size even if it uses some other page size.  *That's* the sort of
explanation you need for a hard shift of 12 in your address shift comments.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD




-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]