Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 5 Jul 2018 08:28:23 +0200 (CEST)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Bin Yang <bin.yang@intel.com>
cc:     mingo@redhat.com, hpa@zytor.com, x86@kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] x86/mm: fix cpu stuck issue in
 __change_page_attr_set_clr
In-Reply-To: <1530769636-26603-1-git-send-email-bin.yang@intel.com>
Message-ID: <alpine.DEB.2.21.1807050815570.1579@nanos.tec.linutronix.de>
References: <1530769636-26603-1-git-send-email-bin.yang@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 5 Jul 2018, Bin Yang wrote:
>  static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
> -				   unsigned long pfn)
> +				   unsigned long pfn, unsigned long *page_num)
>  {
>  	pgprot_t forbidden = __pgprot(0);
> +	unsigned long tmp;
> +	unsigned long num = PUD_PAGE_SIZE >> PAGE_SHIFT;

Everything is a PUD? Oh well, you clearly made a lot of effort to
understand the code you are changing.

>  	/*
>  	 * The BIOS area between 640k and 1Mb needs to be executable for
>  	 * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
>  	 */
>  #ifdef CONFIG_PCI_BIOS
> -	if (pcibios_enabled && within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
> -		pgprot_val(forbidden) |= _PAGE_NX;
> +	if (pcibios_enabled) {
> +		tmp = (BIOS_BEGIN >> PAGE_SHIFT) > pfn ?
> +			(BIOS_BEGIN >> PAGE_SHIFT) - pfn : ULONG_MAX;
> +		if (within(pfn, BIOS_BEGIN >> PAGE_SHIFT,
> +		    BIOS_END >> PAGE_SHIFT)) {
> +			pgprot_val(forbidden) |= _PAGE_NX;
> +			tmp = (BIOS_END >> PAGE_SHIFT) - pfn;
> +		}
> +		num = num > tmp ? tmp : num;

What? I really gave you a hint which used a overlaps() helper in the pseudo
code. But sure open coding the same thing in 5 places is faster, right?

You managed to make this code completely unreadable and I'm not even trying
to review that mess.

> +	for (i = 0; i < (psize >> PAGE_SHIFT);
> +	    i += pnum, addr += PAGE_SIZE * pnum, pfn += pnum) {
> +		pgprot_t chk_prot =
> +			static_protections(req_prot, addr, pfn, &pnum);

When done right, then there is no need for a loop at all. And I told you so.
But that would need more effort than creating a trainwreck, right? 

Stop sending half baken and half thought out patches. I really spent a lot
of time explaining you things in detail and giving you hints how it should
be done. Feel free to ignore me, but don't be surprised if I'm ignoring you
as well.

Thanks,

	tglx