Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier

[Andrew Banman]

I will resubmit this patch and [Patch 5/6] as separate patches. They are 
both independent of the wait_completion feature.

I agree with your suggestions here. The updated version will have 
KernelDoc comments and proper u** integer types. I'd also like to add a 
preparatory patch to implement the hub version enum constants. This 
patch will replace all the magic numbers used to check uvhub_version. 
Like so:

[Patch 1/2] x86/platform/uv/BAU: Add uv_bau_version enumerated constants
[PATCH 2/2] x86/platform/uv/BAU: Add payload descriptor qualifier


Thank you for the suggestions!

Andrew Banman

On 02/16/2017 12:42 PM, Thomas Gleixner wrote:
> 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