lkml.org 
[lkml]   [2008]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] x86: mtrr cleanup for converting continuous to discrete layout v8

    a few minor cleanliness observations:

    > +#ifdef CONFIG_MTRR_SANITIZER
    > +
    > +#ifdef CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT
    > +static int enable_mtrr_cleanup __initdata = 1;
    > +#else
    > +static int enable_mtrr_cleanup __initdata;
    > +#endif
    > +
    > +#else
    > +
    > +static int enable_mtrr_cleanup __initdata = -1;
    > +
    > +#endif

    this should be a single:

    #ifdef CONFIG_MTRR_SANITIZER
    static int mtrr_cleanup_enabled = CONFIG_MTRR_SANITIZER_DEFAULT;
    #endif

    block.

    > +#define RANGE_NUM 256

    small explaination (comment) about what the limit means.

    > +static int __init add_range(struct res_range *range, int nr_range, unsigned long start,
    > + unsigned long end, int merge)

    looks cleaner this way:

    static int __init
    add_range(struct res_range *range, int nr_range, unsigned long start,
    unsigned long end, int merge)

    > +{
    > + int i;
    > +
    > + if (!merge)
    > + goto addit;
    > +
    > + /* try to merge it with old one */
    > + for (i = 0; i < nr_range; i++) {
    > + unsigned long final_start, final_end;
    > + unsigned long common_start, common_end;
    > +
    > + if (!range[i].end)
    > + continue;
    > +
    > + common_start = max(range[i].start, start);
    > + common_end = min(range[i].end, end);
    > + if (common_start > common_end + 1)
    > + continue;
    > +
    > + final_start = min(range[i].start, start);
    > + final_end = max(range[i].end, end);
    > +
    > + range[i].start = final_start;
    > + range[i].end = final_end;
    > + return nr_range;
    > + }
    > +
    > +addit:

    perhaps factor out the loop into a separate function and avoid the goto.

    > +static void __init subtract_range(struct res_range *range, unsigned long start,
    > + unsigned long end)

    should be:

    static void __init
    subtract_range(struct res_range *range, unsigned long start,
    unsigned long end)

    > + int i;
    > + int j;

    can be:

    int i, j;

    > + }
    > +
    > +

    stale newline.

    > + if (start > range[j].start && end >= range[j].end && range[j].end > start - 1) {

    should be some sort of more readable in_range() check?

    > + range[j].end = start - 1;
    > + continue;
    > + }
    > +
    > + if (start > range[j].start && end < range[j].end) {
    > + /* find the new spare */
    > + for (i = 0; i < RANGE_NUM; i++) {
    > + if (range[i].end == 0)
    > + break;
    > + }
    > + if (i < RANGE_NUM) {
    > + range[i].end = range[j].end;
    > + range[i].start = end + 1;
    > + } else {
    > + printk(KERN_ERR "run of slot in ranges\n");
    > + }
    > + range[j].end = start - 1;
    > + continue;
    > + }
    > + }
    > +}

    > +struct var_mtrr_state {
    > + unsigned long range_startk, range_sizek;
    > + unsigned long chunk_sizek;
    > + unsigned long gran_sizek;
    > + unsigned int reg;
    > + unsigned address_bits;
    > +};

    s/unsigned address_bits/unsigned int address_bits/

    also move range_sizek on a separate line.

    plus we tend to align structures this way:

    > +struct var_mtrr_state {
    > + unsigned long range_startk;
    > + unsigned long range_sizek;
    > + unsigned long chunk_sizek;
    > + unsigned long gran_sizek;
    > + unsigned int reg;
    > + unsigned int address_bits;
    > +};

    (to put the types and field names into a visually more consistent form)

    > +static void __init set_var_mtrr(
    > + unsigned int reg, unsigned long basek, unsigned long sizek,
    > + unsigned char type, unsigned address_bits)

    should be:

    static void __init
    set_var_mtrr(unsigned int reg, unsigned long basek, unsigned long sizek,
    unsigned char type, unsigned address_bits)

    > + u32 base_lo, base_hi, mask_lo, mask_hi;
    > + unsigned address_mask_high;

    s/unsigned/unsigned int

    hm, will this work on 64-bit? Above-4G is controlled via separate
    mechanisms though so i guess it does.

    > + address_mask_high = ((1u << (address_bits - 32u)) - 1u);

    use alignment macros instead.

    > + unsigned long sizek;
    > + /* Compute the maximum size I can make a range */
    > + if (range_startk)

    put extra newline between variable definition and code.

    > + var_state.range_startk = 0;
    > + var_state.range_sizek = 0;
    > + var_state.reg = 0;
    > + var_state.address_bits = address_bits;
    > + var_state.chunk_sizek = mtrr_chunk_size >> 10;
    > + var_state.gran_sizek = mtrr_gran_size >> 10;

    initialization looks nicer with vertical alignment, i.e.:

    > + var_state.range_startk = 0;
    > + var_state.range_sizek = 0;
    > + var_state.reg = 0;
    > + var_state.address_bits = address_bits;
    > + var_state.chunk_sizek = mtrr_chunk_size >> 10;
    > + var_state.gran_sizek = mtrr_gran_size >> 10;

    > + /* Clear out the extra MTRR's */
    > + while (var_state.reg < num_var_ranges)
    > + set_var_mtrr(var_state.reg++, 0, 0, 0, var_state.address_bits);

    the ++ is a hard to notice side-effect of the loop. It's cleaner to
    separate it out or to have a for() loop for it.

    > +static int __init mtrr_cleanup(unsigned address_bits)
    > +{
    > + unsigned long i, base, size, def, dummy;
    > + mtrr_type type;
    > + struct res_range range[RANGE_NUM];
    > + int nr_range;
    > + unsigned long extra_remove_base, extra_remove_size;

    try to use a 'christmas tree' ordering of variables, i.e.:

    > + unsigned long extra_remove_base, extra_remove_size;
    > + unsigned long i, base, size, def, dummy;
    > + struct res_range range[RANGE_NUM];
    > + mtrr_type type;
    > + int nr_range;

    > + return 1;
    > +
    > +}

    superfluous newline.

    all in one, this is a very useful and nice feature.

    Ingo


    \
     
     \ /
      Last update: 2008-04-29 15:11    [W:4.255 / U:0.212 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site