Re: [PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:35PM -0600, Toshi Kani wrote:
> Document the return values of KVA mapping functions,

KVA?

Please write it out.

> pud_set_huge(), pmd_set_huge, pud_clear_huge() and
> pmd_clear_huge().
> 
> Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
> in the Kconfig, since X86_PAE depends on X86_32.
> 
> There is no functional change in this patch.
> 
> Signed-off-by: Toshi Kani <toshi.kani@xxxxxx>
> ---
>  arch/x86/Kconfig      |    2 +-
>  arch/x86/mm/pgtable.c |   36 ++++++++++++++++++++++++++++--------
>  2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cb23206..2ea27da 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -99,7 +99,7 @@ config X86
>  	select IRQ_FORCED_THREADING
>  	select HAVE_BPF_JIT if X86_64
>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> -	select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
> +	select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
>  	select ARCH_HAS_SG_CHAIN
>  	select CLKEVT_I8253
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG

This is an unrelated change, please carve it out in a separate patch.

> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 0b97d2c..4891fa1 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
>  }
>  
>  #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> +/**
> + * pud_set_huge - setup kernel PUD mapping
> + *
> + * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> + * it does not set up a huge page when the range is covered by a non-WB

"it" is what exactly?

> + * type of MTRR.  0xFF indicates that MTRR are disabled.

So this shows that this patch shouldn't be the first one in the series.

IMO you want to start with cleaning up mtrr_type_lookup(), add the
defines for its retval and *then* document its users. This way you won't
have to touch the same place twice, the net-size of your patchset will
go down and it will be easier for reviewiers.

> + *
> + * Return 1 on success, and 0 when no PUD was set.

"Returns 1 on success and 0 on failure."

> + */
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>  {
>  	u8 mtrr;
>  
> -	/*
> -	 * Do not use a huge page when the range is covered by non-WB type
> -	 * of MTRRs.
> -	 */
>  	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
>  	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
>  		return 0;

Ditto for the rest.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/