Messages in this thread | | | Date | Tue, 29 Apr 2008 15:07:04 +0200 | From | Ingo Molnar <> | Subject | Re: [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
| |