* Toshi Kani <[email protected]> wrote:
> MTRRs contain fixed and variable entries. mtrr_type_lookup()
> may repeatedly call __mtrr_type_lookup() to handle a request
> that overlaps with variable entries. However,
> __mtrr_type_lookup() also handles the fixed entries, which
> do not have to be repeated. Therefore, this patch creates
> separate functions, mtrr_type_lookup_fixed() and
> mtrr_type_lookup_variable(), to handle the fixed and variable
> ranges respectively.
>
> The patch also updates the function headers to clarify the
> return values and output argument. It updates comments to
> clarify that the repeating is necessary to handle overlaps
> with the default type, since overlaps with multiple entries
> alone can be handled without such repeating.
>
> There is no functional change in this patch.
Nice cleanup!
I also suggest adding a small table to the comments before the
function, that lists the fixed purpose MTRRs and their address ranges
- to make it more obvious what the magic hexadecimal constants within
the code are doing.
> +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> +{
> + int idx;
> +
> + if (start >= 0x100000)
> + return 0xFF;
Btw., as a separate cleanup patch, we should probably also change
'0xFF' (which is sometimes written as 0xff) to be some sufficiently
named constant, and explain its usage somewhere?
> + if (!(mtrr_state.have_fixed) ||
> + !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
Btw., can MTRR_STATE_MTRR_FIXED_ENABLED ever be set in
mtrr_state.enabled, without mtrr_state.have_fixed being set?
AFAICS get_mtrr_state() will only ever fill in mtrr_state with fixed
MTRRs if mtrr_state.have_fixed != 0 - but I might be mis-reading the
(rather convoluted) flow of code ...
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
