From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933661AbdBPSme (ORCPT ); Thu, 16 Feb 2017 13:42:34 -0500 Received: from Galois.linutronix.de ([146.0.238.70]:53753 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933578AbdBPSmb (ORCPT ); Thu, 16 Feb 2017 13:42:31 -0500 Date: Thu, 16 Feb 2017 19:42:07 +0100 (CET) From: Thomas Gleixner To: Andrew Banman cc: mingo@redhat.com, akpm@linux-foundation.org, hpa@zytor.com, mike.travis@hpe.com, rja@hpe.com, sivanich@hpe.com, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier In-Reply-To: <1487123931-56809-7-git-send-email-abanman@hpe.com> Message-ID: References: <1487123931-56809-1-git-send-email-abanman@hpe.com> <1487123931-56809-7-git-send-email-abanman@hpe.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 14 Feb 2017, Andrew Banman wrote: > On UV4, the destination agent verifies each message by checking the > descriptor qualifier field of the message payload. Messages without this > field set to 0x534749 will cause a hub error to assert. Ok. What's missing here is: Seperate the message structs for uv123 and uv4. > Make this the default action for future architectures, anticipating they > will have the same requirement. That's a guarantee to cause issues when uv5 comes around. The way better solution for this is to do: enum uv_bau_version { UV_BAU_V1 = 1, UV_BAU_V2, UV_BAU_V3, UV_BAU_V4, }; Make bau->uvhub_version type uv_bau_version and use the enum constants in the switch case. That way the compiler will catch you when you add UV_BAU_V5 and forgot to update that switch case. That's probably handy to have that in a few other places which switch on the bau version. > -struct bau_msg_payload { > +struct uv1_2_3_bau_msg_payload { > unsigned long address; /* signifies a page or all > TLB's of the cpu */ > /* 64 bits */ > @@ -236,6 +238,20 @@ struct bau_msg_payload { > unsigned int reserved1:32; /* not usable */ > }; > > +struct uv4_bau_msg_payload { > + unsigned long address; /* signifies a page or all > + * TLB's of the cpu > + */ Please get rid of these tail comments. Either document the struct members with a comment above the member or even better use the KernelDoc comment format above the struct to document it. > + /* 64 bits */ And these are horrible. I had to look twice where this belongs to. I know you copied existing crap, but that does not make it any better. And really, if you want to express the size of a member here because you have to talk to hardware then use the proper types we have for this: u64, u32, u16 .... > + unsigned short sending_cpu; /* filled in by sender */ > + /* 16 bits */ > + unsigned short acknowledge_count; /* filled in by destination */ > + /* 16 bits */ > + unsigned int reserved1:8; /* not usable */ > + unsigned int qualifier:24; /* descriptor qualifier filled > + * in by sender > @@ -1200,6 +1200,7 @@ const struct cpumask *uv_flush_tlb_other > struct bau_control *bcp; > unsigned long descriptor_status; > unsigned long status; > + unsigned long address; Same types can go into a single line. No value in wasting lines. Thanks, tglx