All of lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel@lists.infradead.org, jslaby@suse.com
Subject: Re: [PATCH 2/3] arm64: lib: Use modern annotations for assembly functions
Date: Tue, 7 Jan 2020 14:44:46 +0000	[thread overview]
Message-ID: <20200107144445.GC29001@willie-the-truck> (raw)
In-Reply-To: <20200106195818.56351-3-broonie@kernel.org>

Hi Mark,

[+Jiri -- couple of questions below]

On Mon, Jan 06, 2020 at 07:58:17PM +0000, Mark Brown wrote:
> In an effort to clarify and simplify the annotation of assembly functions
> in the kernel new macros have been introduced. These replace ENTRY and
> ENDPROC and also add a new annotation for static functions which previously
> had no ENTRY equivalent. Update the annotations in the library code to the
> new macros.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/lib/clear_page.S     |  4 ++--
>  arch/arm64/lib/clear_user.S     |  4 ++--
>  arch/arm64/lib/copy_from_user.S |  4 ++--
>  arch/arm64/lib/copy_in_user.S   |  4 ++--
>  arch/arm64/lib/copy_page.S      |  4 ++--
>  arch/arm64/lib/copy_to_user.S   |  4 ++--
>  arch/arm64/lib/crc32.S          |  8 ++++----
>  arch/arm64/lib/memchr.S         |  4 ++--
>  arch/arm64/lib/memcmp.S         |  4 ++--
>  arch/arm64/lib/memcpy.S         |  8 ++++----
>  arch/arm64/lib/memmove.S        |  8 ++++----
>  arch/arm64/lib/memset.S         |  8 ++++----
>  arch/arm64/lib/strchr.S         |  4 ++--
>  arch/arm64/lib/strcmp.S         |  4 ++--
>  arch/arm64/lib/strlen.S         |  4 ++--
>  arch/arm64/lib/strncmp.S        |  4 ++--
>  arch/arm64/lib/strnlen.S        |  4 ++--
>  arch/arm64/lib/strrchr.S        |  4 ++--
>  arch/arm64/lib/tishift.S        | 12 ++++++------
>  19 files changed, 50 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/lib/clear_page.S b/arch/arm64/lib/clear_page.S
> index 78a9ef66288a..073acbf02a7c 100644
> --- a/arch/arm64/lib/clear_page.S
> +++ b/arch/arm64/lib/clear_page.S
> @@ -14,7 +14,7 @@
>   * Parameters:
>   *	x0 - dest
>   */
> -ENTRY(clear_page)
> +SYM_FUNC_START(clear_page)
>  	mrs	x1, dczid_el0
>  	and	w1, w1, #0xf
>  	mov	x2, #4

Since this doesn't change behaviour, I think the patch is fine, however
on reading Documentation/asm-annotations.rst it's not completely clear to
me when SYM_FUNC_START() should be used. In this case, for example, we are
*not* pushing a stackframe and that would appear to be at odds with the
documentation.

Jiri -- is it ok to omit the stack frame for leaf functions annotated with
SYM_FUNC_START? I'm guessing it should be, since the link register isn't
going to be clobbered. Could we update the documentation to reflect that?

> diff --git a/arch/arm64/lib/crc32.S b/arch/arm64/lib/crc32.S
> index e6135f16649b..243e107e9896 100644
> --- a/arch/arm64/lib/crc32.S
> +++ b/arch/arm64/lib/crc32.S
> @@ -85,17 +85,17 @@ CPU_BE(	rev16		w3, w3		)
>  	.endm
>  
>  	.align		5
> -ENTRY(crc32_le)
> +SYM_FUNC_START(crc32_le)
>  alternative_if_not ARM64_HAS_CRC32
>  	b		crc32_le_base

Similar thing here -- I'm assuming we are ok to tail-call crc32_le_base()
from a function marked with SYM_FUNC_START like this?

>  alternative_else_nop_endif
>  	__crc32
> -ENDPROC(crc32_le)
> +SYM_FUNC_END(crc32_le)
>  
>  	.align		5
> -ENTRY(__crc32c_le)
> +SYM_FUNC_START(__crc32c_le)
>  alternative_if_not ARM64_HAS_CRC32
>  	b		__crc32c_le_base
>  alternative_else_nop_endif
>  	__crc32		c
> -ENDPROC(__crc32c_le)
> +SYM_FUNC_END(__crc32c_le)
> diff --git a/arch/arm64/lib/memchr.S b/arch/arm64/lib/memchr.S
> index 48a3ab636e4f..99910c5d5db7 100644
> --- a/arch/arm64/lib/memchr.S
> +++ b/arch/arm64/lib/memchr.S
> @@ -19,7 +19,7 @@
>   * Returns:
>   *	x0 - address of first occurrence of 'c' or 0
>   */
> -WEAK(memchr)
> +SYM_FUNC_START_PI_WEAK(memchr)
>  	and	w1, w1, #0xff
>  1:	subs	x2, x2, #1
>  	b.mi	2f
> @@ -30,5 +30,5 @@ WEAK(memchr)
>  	ret
>  2:	mov	x0, #0
>  	ret
> -ENDPIPROC(memchr)
> +SYM_FUNC_END_PI(memchr)
>  EXPORT_SYMBOL_NOKASAN(memchr)
> diff --git a/arch/arm64/lib/memcmp.S b/arch/arm64/lib/memcmp.S
> index b297bdaaf549..b889f312bdb3 100644
> --- a/arch/arm64/lib/memcmp.S
> +++ b/arch/arm64/lib/memcmp.S
> @@ -46,7 +46,7 @@ pos		.req	x11
>  limit_wd	.req	x12
>  mask		.req	x13
>  
> -WEAK(memcmp)
> +SYM_FUNC_START_PI_WEAK(memcmp)
>  	cbz	limit, .Lret0
>  	eor	tmp1, src1, src2
>  	tst	tmp1, #7
> @@ -243,5 +243,5 @@ CPU_LE( rev	data2, data2 )
>  .Lret0:
>  	mov	result, #0
>  	ret
> -ENDPIPROC(memcmp)
> +SYM_FUNC_END_PI(memcmp)
>  EXPORT_SYMBOL_NOKASAN(memcmp)
> diff --git a/arch/arm64/lib/memcpy.S b/arch/arm64/lib/memcpy.S
> index d79f48994dbb..9f382adfa88a 100644
> --- a/arch/arm64/lib/memcpy.S
> +++ b/arch/arm64/lib/memcpy.S
> @@ -57,11 +57,11 @@
>  	.endm
>  
>  	.weak memcpy

Hmm, any idea why we use .weak explicitly here? Maybe better off using
the proper macros now? (same applies to many of the other lib/ functions
you're touching)

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-01-07 14:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-06 19:58 [PATCH 0/3] arm64: Conversions to modern assembly annotations Mark Brown
2020-01-06 19:58 ` [PATCH 1/3] arm64: asm: Add new-style position independent function annotations Mark Brown
2020-01-07 14:44   ` Will Deacon
2020-01-07 16:45     ` Mark Brown
2020-01-06 19:58 ` [PATCH 2/3] arm64: lib: Use modern annotations for assembly functions Mark Brown
2020-01-07 14:44   ` Will Deacon [this message]
2020-01-07 17:47     ` Mark Brown
2020-01-08 12:29       ` Will Deacon
2020-01-10 16:56         ` Jiri Slaby
2020-01-15 18:43           ` Will Deacon
2020-01-06 19:58 ` [PATCH 3/3] arm64: mm: " Mark Brown
2020-01-07 14:43   ` Will Deacon
2020-01-07 16:42     ` Mark Brown
2020-01-08 12:17       ` Will Deacon
2020-01-08 13:50         ` Mark Brown
2020-01-08 14:56           ` Will Deacon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200107144445.GC29001@willie-the-truck \
    --to=will@kernel.org \
    --cc=broonie@kernel.org \
    --cc=catalin.marinas@arm.com \
    --cc=jslaby@suse.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.