Re: [PATCH] drivers/net/tile/: on-chip network drivers for the tile architecture

[Chris Metcalf <cmetcalf@tilera.com>]

Stephen, thanks for your feedback!

On 11/3/2010 12:59 PM, Stephen Hemminger wrote:
> 1. MUST not use volatile, see volatile-considered-harmful.txt

The "harmful" use of volatile is in trying to fake out SMP.  Believe me,
with a 64-core architecture, we know our SMP guidelines. :-)  Our use here
is simply to force the compiler to issue a load, for the side-effect of
populating the TLB, for example.

However, your response does suggest that simply the syntactic use of
"volatile" will cause a red flag for readers.  I'll move this to an inline
function in a header with a comment explaining what it's for, and use the
function instead.

> 2. SHOULD use __u32 rather than uint32_t in kernel structures

Thanks, I've made this change in drivers/net/tile/tilepro.c.  In fact, I
used "u32" since this code is not shared with userspace.  However, see
below for the <hv/xxx.h> header files.

> 3. MUST not introduce typedef's; use structures
> 4. SHOULD use proper kernel implementation

(I think you mean proper kernel indentation.)  This is already true for the
kernel sources in driver/net/tile/, but false for the tile-specific
hypervisor headers in arch/tile/include/hv/.  These are "upstream" headers
that are being added to the kernel on an as-needed basis so the kernel can
talk with the hypervisor.

Including a copy of the hypervisor headers with the kernel makes it easier
to build the kernel (since there are no external dependencies that need to
be tracked down to do the build) and is consistent with the fact that we
can in principle modify the hypervisor and its headers out of sync with the
kernel, but still expect the old headers to work correctly with a new
hypervisor since Linux always initializes itself to the hypervisor with the
compiled-in header version number.

So the upshot is that these headers are imported into the kernel and
generally can't be stylistically modified.  But they are "ghettoized" to
arch/tile/include/hv/ and should not cause any trouble. :-)

> If you use scripts/checkpatch.pl it will tell you about these.

Yes, the "volatile" issue is mentioned, but even with "--strict", there's
no mention of uint32_t, so I missed that.  I don't see "uint32" mentioned
anywhere in scripts/checkpatch.pl, in fact.

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com