lkml.org 
[lkml]   [2022]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 05/10] x86/mtrr: split generic_set_all()
On Thu, Sep 08, 2022 at 10:49:09AM +0200, Juergen Gross wrote:
> Split generic_set_all() into multiple parts, while moving the main
> function body into cacheinfo.c.
>
> This prepares the support of PAT without needing MTRR support by

"Prepare PAT support without ... "

> moving the main function body of generic_set_all() into cacheinfo.c
> while renaming it to cache_cpu_init(). The MTRR specific parts are
> moved into a dedicated small function called by cache_cpu_init().
> The PAT and MTRR specific functions are called conditionally based
> on the cache_generic bit settings.
>
> The setting of smp_changes_mask is merged into the (new) function

... and so on. So the commit message should not say what you're doing -
that should be visible from the diff itself. It should talk more about
the *why* you're doing it.

...

> diff --git a/arch/x86/kernel/cpu/cacheinfo.c b/arch/x86/kernel/cpu/cacheinfo.c
> index 47e2c72fa8a4..36378604ec61 100644
> --- a/arch/x86/kernel/cpu/cacheinfo.c
> +++ b/arch/x86/kernel/cpu/cacheinfo.c
> @@ -1120,3 +1120,22 @@ void cache_enable(void) __releases(cache_disable_lock)
>
> raw_spin_unlock(&cache_disable_lock);
> }
> +
> +void cache_cpu_init(void)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + cache_disable();
> +
> + /* Set MTRR state. */

Yeah, and when you name the functions properly as you've done, you don't
really need those comments anymore either.

> + if (cache_generic & CACHE_GENERIC_MTRR)
> + mtrr_generic_set_state();
> +
> + /* Set PAT. */

And this one.

> + if (cache_generic & CACHE_GENERIC_PAT)
> + pat_init();
> +
> + cache_enable();
> + local_irq_restore(flags);
> +}
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 5ed397f03a87..fc7b2d952737 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -731,30 +731,19 @@ void mtrr_enable(void)
> mtrr_wrmsr(MSR_MTRRdefType, deftype_lo, deftype_hi);
> }
>
> -static void generic_set_all(void)
> +void mtrr_generic_set_state(void)
> {
> unsigned long mask, count;
> - unsigned long flags;
> -
> - local_irq_save(flags);
> - cache_disable();
>
> /* Actually set the state */
> mask = set_mtrr_state();
>
> - /* also set PAT */
> - pat_init();
> -
> - cache_enable();
> - local_irq_restore(flags);
> -
> /* Use the atomic bitops to update the global mask */
> for (count = 0; count < sizeof(mask) * 8; ++count) {
> if (mask & 0x01)
> set_bit(count, &smp_changes_mask);
> mask >>= 1;
> }
> -
> }
>
> /**
> @@ -854,7 +843,7 @@ int positive_have_wrcomb(void)
> * Generic structure...
> */
> const struct mtrr_ops generic_mtrr_ops = {
> - .set_all = generic_set_all,
> + .set_all = cache_cpu_init,

I was gonna say that this looks weird - a set_all function pointer
assigned to a init function but that changes in the next patch.

But yeah, I like where this is going.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

\
 
 \ /
  Last update: 2022-09-19 21:26    [W:0.322 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site