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;
Subject: Re: [PATCH 2/5] Powerpc/hw-breakpoint: Refactor
 hw_breakpoint_arch_parse()
To:     Ravi Bangoria <ravi.bangoria@linux.ibm.com>, mpe@ellerman.id.au
Cc:     benh@kernel.crashing.org, paulus@samba.org, mikey@neuling.org,
        linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org,
        npiggin@gmail.com, naveen.n.rao@linux.vnet.ibm.com
References: <20190618042732.5582-1-ravi.bangoria@linux.ibm.com>
 <20190618042732.5582-3-ravi.bangoria@linux.ibm.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <66e70f57-befa-f241-9476-8e06519bac90@c-s.fr>
Date:   Tue, 18 Jun 2019 08:21:56 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <20190618042732.5582-3-ravi.bangoria@linux.ibm.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Le 18/06/2019 à 06:27, Ravi Bangoria a écrit :
> Move feature availability check at the start of the function.
> Rearrange comment to it's associated code. Use hw->address and
> hw->len in the 512 bytes boundary check(to write if statement
> in a single line). Add spacing between code blocks.

Are those cosmetic changes in the boundary check worth it since they 
disappear in the final patch ?

Christophe

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
>   arch/powerpc/kernel/hw_breakpoint.c | 34 +++++++++++++++--------------
>   1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 1908e4fcc132..36bcf705df65 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -133,10 +133,13 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   			     const struct perf_event_attr *attr,
>   			     struct arch_hw_breakpoint *hw)
>   {
> -	int ret = -EINVAL, length_max;
> +	int length_max;
> +
> +	if (!ppc_breakpoint_available())
> +		return -ENODEV;
>   
>   	if (!bp)
> -		return ret;
> +		return -EINVAL;
>   
>   	hw->type = HW_BRK_TYPE_TRANSLATE;
>   	if (attr->bp_type & HW_BREAKPOINT_R)
> @@ -145,34 +148,33 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
>   		hw->type |= HW_BRK_TYPE_WRITE;
>   	if (hw->type == HW_BRK_TYPE_TRANSLATE)
>   		/* must set alteast read or write */
> -		return ret;
> +		return -EINVAL;
> +
>   	if (!attr->exclude_user)
>   		hw->type |= HW_BRK_TYPE_USER;
>   	if (!attr->exclude_kernel)
>   		hw->type |= HW_BRK_TYPE_KERNEL;
>   	if (!attr->exclude_hv)
>   		hw->type |= HW_BRK_TYPE_HYP;
> +
>   	hw->address = attr->bp_addr;
>   	hw->len = attr->bp_len;
>   
> -	/*
> -	 * Since breakpoint length can be a maximum of HW_BREAKPOINT_LEN(8)
> -	 * and breakpoint addresses are aligned to nearest double-word
> -	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address, the
> -	 * 'symbolsize' should satisfy the check below.
> -	 */
> -	if (!ppc_breakpoint_available())
> -		return -ENODEV;
>   	length_max = 8; /* DABR */
>   	if (dawr_enabled()) {
>   		length_max = 512 ; /* 64 doublewords */
> -		/* DAWR region can't cross 512 boundary */
> -		if ((attr->bp_addr >> 9) !=
> -		    ((attr->bp_addr + attr->bp_len - 1) >> 9))
> +		/* DAWR region can't cross 512 bytes boundary */
> +		if ((hw->address >> 9) != ((hw->address + hw->len - 1) >> 9))
>   			return -EINVAL;
>   	}
> -	if (hw->len >
> -	    (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
> +
> +	/*
> +	 * Since breakpoint length can be a maximum of length_max and
> +	 * breakpoint addresses are aligned to nearest double-word
> +	 * HW_BREAKPOINT_ALIGN by rounding off to the lower address,
> +	 * the 'symbolsize' should satisfy the check below.
> +	 */
> +	if (hw->len > (length_max - (hw->address & HW_BREAKPOINT_ALIGN)))
>   		return -EINVAL;
>   	return 0;
>   }
>