lkml.org 
[lkml]   [2017]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2017-02-16 19:44    [W:3.167 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site