Messages in this thread Patch in this message | | | Date | Thu, 16 Feb 2017 19:42:07 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH 6/6] x86/platform/uv/BAU: Add payload descriptor qualifier |
| |
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
| |