lkml.org 
[lkml]   [2017]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/6] x86/platform/uv/BAU: Implement uv4_wait_completion with read_status
    On Tue, 14 Feb 2017, Andrew Banman wrote:
    > UV4 does not employ a software-timeout as in previous generations so a new
    > wait_completion routine without this logic is required. Certain completion
    > statuses require the AUX status bit in addition to ERROR and BUSY.
    >
    > Add the read_status routine to construct the full completion status. Use
    > read_status in the uv4_wait_completion routine to handle all possible
    > completion statuses.

    Ok.

    > +/*
    > + * Returns the status of current BAU message for cpu desc as a bit field
    > + * [Error][Busy][Aux]
    > + */
    > +static unsigned long read_status(unsigned long status_mmr, int index, int desc)
    > +{
    > + unsigned long descriptor_status;
    > +
    > + descriptor_status =
    > + ((read_lmmr(status_mmr) >> index) & UV_ACT_STATUS_MASK) << 1;
    > +
    > + descriptor_status |=
    > + (read_lmmr(UVH_LB_BAU_SB_ACTIVATION_STATUS_2) >> desc) & 0x1;

    You can spare all those ugly line breaks by chosing a short variable
    name. You explain already in the comment that the returned value is the
    status for a cpu descriptor. So where is the point of making the local
    helper variable repeat than info?

    > + return descriptor_status;
    > +}
    > +
    > +static int uv4_wait_completion(struct bau_desc *bau_desc,
    > + struct bau_control *bcp, long try)
    > +{
    > + unsigned long descriptor_stat;
    > + unsigned long err_busy_mmr;
    > + int err_busy_index;
    > + int desc = bcp->uvhub_cpu;
    > + struct ptc_stats *stat = bcp->statp;

    We usually order the local variables in reverse fir tree mode:

    > + struct ptc_stats *stat = bcp->statp;
    > + unsigned long descriptor_stat;
    > + unsigned long err_busy_mmr;
    > + int desc = bcp->uvhub_cpu;
    > + int err_busy_index;

    It's simpler to parse than the random line length mode above.

    > + status_mmr_loc(&err_busy_mmr, &err_busy_index, desc);
    > + descriptor_stat = read_status(err_busy_mmr, err_busy_index, desc);
    > +
    > + /* spin on the status MMR, waiting for it to go idle */
    > + while (descriptor_stat != UV2H_DESC_IDLE) {
    > + switch (descriptor_stat) {
    > + case UV2H_DESC_SOURCE_TIMEOUT:
    > + stat->s_stimeout++;
    > + return FLUSH_GIVEUP;
    > +
    > + case UV2H_DESC_DEST_TIMEOUT:
    > + stat->s_dtimeout++;
    > + bcp->conseccompletes = 0;
    > + return FLUSH_RETRY_TIMEOUT;
    > +
    > + case UV2H_DESC_DEST_STRONG_NACK:
    > + stat->s_plugged++;
    > + bcp->conseccompletes = 0;
    > + return FLUSH_RETRY_PLUGGED;
    > +
    > + case UV2H_DESC_DEST_PUT_ERR:
    > + bcp->conseccompletes = 0;
    > + return FLUSH_GIVEUP;
    > +
    > + default:
    > + /* descriptor_stat is still BUSY */
    > + cpu_relax();
    > + }
    > + descriptor_stat =
    > + read_status(err_busy_mmr, err_busy_index, desc);

    Again, making the variable name shorter spares you this ugly line
    break. 'stat' is clear enough.

    Other than those nitpicks, that's all fine.

    Thanks,

    tglx

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