[PATCH v3 0/4] Generalize fncpy availability

[PATCH v3 0/4] Generalize fncpy availability

[Florian Fainelli]

Hi all,

This patch series makes ARM's fncpy() implementation more generic (dropping the
Thumb-specifics) and available in an asm-generic header file.

Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.

Changes in v3 (thanks Doug!):
- correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
- utilize Kbuild to provide the fncpy.h header on ARM64

Changes in v2:
- leave the ARM implementation where it is
- make the generic truly generic (no)

This is helpful in making SoC-specific power management code become true drivers
that can be shared between different architectures.

Thanks!

Florian Fainelli (4):
  ARM: fncpy: Rename include guards
  asm-generic: Provide a fncpy() implementation
  arm64: Provide a fncpy implementation
  misc: sram: Allow ARM64 to select SRAM_EXEC

 arch/arm/include/asm/fncpy.h  |  6 +--
 arch/arm64/include/asm/Kbuild |  1 +
 drivers/misc/Kconfig          |  2 +-
 include/asm-generic/fncpy.h   | 93 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 4 deletions(-)
 create mode 100644 include/asm-generic/fncpy.h

-- 
2.9.3

[PATCH v3 1/4] ARM: fncpy: Rename include guards

[Florian Fainelli]

In preparation for allowing a generic fncpy() implementation to live
under include/asm-generic/fncpy.h, rename the current include guards to
be __ASM_ARM_FNCPY_H, this also makes the header file more consistent
with other headers in the same directory.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/include/asm/fncpy.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/fncpy.h b/arch/arm/include/asm/fncpy.h
index de5354746924..86a8fc14cde9 100644
--- a/arch/arm/include/asm/fncpy.h
+++ b/arch/arm/include/asm/fncpy.h
@@ -53,8 +53,8 @@
  * in the file where f is defined.
  */
 
-#ifndef __ASM_FNCPY_H
-#define __ASM_FNCPY_H
+#ifndef __ASM_ARM_FNCPY_H
+#define __ASM_ARM_FNCPY_H
 
 #include <linux/types.h>
 #include <linux/string.h>
@@ -91,4 +91,4 @@
 	__result;							\
 })
 
-#endif /* !__ASM_FNCPY_H */
+#endif /* !__ASM_ARM_FNCPY_H */
-- 
2.9.3

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Florian Fainelli]

Define a generic fncpy() implementation largely based on the ARM version
that requires an 8 bytes alignment for the destination address where to
copy this function as well as the function's own address.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)
 create mode 100644 include/asm-generic/fncpy.h

diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
new file mode 100644
index 000000000000..ec03b83b8535
--- /dev/null
+++ b/include/asm-generic/fncpy.h
@@ -0,0 +1,93 @@
+/*
+ * include/asm-generic/fncpy.h - helper macros for function body copying
+ *
+ * Copyright (C) 2011 Linaro Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+/*
+ * These macros are intended for use when there is a need to copy a low-level
+ * function body into special memory.
+ *
+ * For example, when reconfiguring the SDRAM controller, the code doing the
+ * reconfiguration may need to run from SRAM.
+ *
+ * NOTE: that the copied function body must be entirely self-contained and
+ * position-independent in order for this to work properly.
+ *
+ * NOTE: in order for embedded literals and data to get referenced correctly,
+ * the alignment of functions must be preserved when copying.  To ensure this,
+ * the source and destination addresses for fncpy() must be aligned to a
+ * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
+ * You will typically need a ".align 3" directive in the assembler where the
+ * function to be copied is defined, and ensure that your allocator for the
+ * destination buffer returns 8-byte-aligned pointers.
+ *
+ * Typical usage example:
+ *
+ * extern int f(args);
+ * extern uint32_t size_of_f;
+ * int (*copied_f)(args);
+ * void *sram_buffer;
+ *
+ * copied_f = fncpy(sram_buffer, &f, size_of_f);
+ *
+ * ... later, call the function: ...
+ *
+ * copied_f(args);
+ *
+ * The size of the function to be copied can't be determined from C:
+ * this must be determined by other means, such as adding assmbler directives
+ * in the file where f is defined.
+ */
+
+#ifndef __ASM_FNCPY_H
+#define __ASM_FNCPY_H
+
+#include <linux/types.h>
+#include <linux/string.h>
+
+#include <asm/bug.h>
+#include <asm/cacheflush.h>
+
+/*
+ * Minimum alignment requirement for the source and destination addresses
+ * for function copying.
+ */
+#define FNCPY_ALIGN 8
+
+#define fncpy(dest_buf, funcp, size) ({					\
+	uintptr_t __funcp_address;					\
+	typeof(funcp) __result;						\
+									\
+	asm("" : "=r" (__funcp_address) : "0" (funcp));			\
+									\
+	/*								\
+	 * Ensure alignment of source and destination addresses.	\
+	 */								\
+	BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) ||		\
+		(__funcp_address & (FNCPY_ALIGN - 1)));			\
+									\
+	memcpy(dest_buf, (void const *)__funcp_address, size);		\
+	flush_icache_range((unsigned long)(dest_buf),			\
+		(unsigned long)(dest_buf) + (size));			\
+									\
+	asm("" : "=r" (__result)					\
+		: "0" ((uintptr_t)(dest_buf)));				\
+									\
+	__result;							\
+})
+
+#endif /* !__ASM_FNCPY_H */
-- 
2.9.3

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Yury Norov]

Hi Florian,

Some questions and thoughts inline.

Yury

On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
> Define a generic fncpy() implementation largely based on the ARM version
> that requires an 8 bytes alignment for the destination address where to
> copy this function as well as the function's own address.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
>  create mode 100644 include/asm-generic/fncpy.h
> 
> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
> new file mode 100644
> index 000000000000..ec03b83b8535
> --- /dev/null
> +++ b/include/asm-generic/fncpy.h
> @@ -0,0 +1,93 @@
> +/*
> + * include/asm-generic/fncpy.h - helper macros for function body copying
> + *
> + * Copyright (C) 2011 Linaro Limited
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +/*
> + * These macros are intended for use when there is a need to copy a low-level
> + * function body into special memory.
> + *
> + * For example, when reconfiguring the SDRAM controller, the code doing the
> + * reconfiguration may need to run from SRAM.
> + *
> + * NOTE: that the copied function body must be entirely self-contained and
> + * position-independent in order for this to work properly.
> + *
> + * NOTE: in order for embedded literals and data to get referenced correctly,
> + * the alignment of functions must be preserved when copying.  To ensure this,
> + * the source and destination addresses for fncpy() must be aligned to a
> + * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
> + * You will typically need a ".align 3" directive in the assembler where the
> + * function to be copied is defined, and ensure that your allocator for the
> + * destination buffer returns 8-byte-aligned pointers.
> + *
> + * Typical usage example:
> + *
> + * extern int f(args);
> + * extern uint32_t size_of_f;
> + * int (*copied_f)(args);
> + * void *sram_buffer;
> + *
> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
> + *
> + * ... later, call the function: ...
> + *
> + * copied_f(args);
> + *
> + * The size of the function to be copied can't be determined from C:
> + * this must be determined by other means, such as adding assmbler directives
> + * in the file where f is defined.
> + */
> +
> +#ifndef __ASM_FNCPY_H
> +#define __ASM_FNCPY_H
> +
> +#include <linux/types.h>
> +#include <linux/string.h>
> +
> +#include <asm/bug.h>
> +#include <asm/cacheflush.h>
> +
> +/*
> + * Minimum alignment requirement for the source and destination addresses
> + * for function copying.
> + */
> +#define FNCPY_ALIGN 8

>From now this is not arm-only, and it's possible that some architectures
might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
guards.

By the way, compiler already has an information on the proper alignment.
Maybe it's better to use it as the default value here instead of the
hardcoded value?

#ifndef FNCPY_ALIGN
#define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
#endif

> +
> +#define fncpy(dest_buf, funcp, size) ({					\

Do you really need to check types inside the macro? If not, you can
declare it as function, which is better in general, with the memcpy-like
prototype.

> +	uintptr_t __funcp_address;					\
> +	typeof(funcp) __result;						\
> +									\
> +	asm("" : "=r" (__funcp_address) : "0" (funcp));			\
> +									\
> +	/*								\
> +	 * Ensure alignment of source and destination addresses.	\
> +	 */								\
> +	BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) ||		\

People don't like new BUG_ONs. Maybe it's better to use BUILD_BUG_ON()
at compile time and WARN_ON() at runtime?

> +		(__funcp_address & (FNCPY_ALIGN - 1)));			\

There is IS_ALIGNED() macro for things like this.

And I frankly don't understand the 2nd check. One can imagine the
situation when someone wants copy the function from the packed blob or
some intermediate location were the function is unaligned, and it's
impossible with the current implementation.

> +									\
> +	memcpy(dest_buf, (void const *)__funcp_address, size);		\
> +	flush_icache_range((unsigned long)(dest_buf),			\
> +		(unsigned long)(dest_buf) + (size));			\
> +									\
> +	asm("" : "=r" (__result)					\
> +		: "0" ((uintptr_t)(dest_buf)));				\
> +									\
> +	__result;							\
> +})
> +
> +#endif /* !__ASM_FNCPY_H */
> -- 
> 2.9.3

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Yury Norov]

> > +/*
> > + * Minimum alignment requirement for the source and destination addresses
> > + * for function copying.
> > + */
> > +#define FNCPY_ALIGN 8
> 
> >From now this is not arm-only, and it's possible that some architectures
> might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> guards.
> 
> By the way, compiler already has an information on the proper alignment.
> Maybe it's better to use it as the default value here instead of the
> hardcoded value?
> 
> #ifndef FNCPY_ALIGN
> #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> #endif

Ah sorry, at first it should be like this:
#define FNCPY_ALIGN ({void foo(); __alignof__(foo);})

And at second, the correct version returns 1 always.

Even if I pass falign-functions=4096 to gcc, and I see that functions
are aligned accordingly in elf file, the macro returns 1 anyway. So if 
it doesn't work, the hardcoded '8' is the only option.

Yury

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Yury Norov]

On Mon, Jun 19, 2017 at 02:51:08AM +0300, Yury Norov wrote:
> Hi Florian,
> 
> Some questions and thoughts inline.
> 
> Yury
> 
> On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
> > Define a generic fncpy() implementation largely based on the ARM version
> > that requires an 8 bytes alignment for the destination address where to
> > copy this function as well as the function's own address.
> > 
> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > ---
> >  include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 93 insertions(+)
> >  create mode 100644 include/asm-generic/fncpy.h

One else thing I forgot to ask - now you have the generic
implementation for fncpy(), so do you really need to save arm
version of it?

Yury

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Florian Fainelli]

On 06/19/2017 08:18 AM, Yury Norov wrote:
> On Mon, Jun 19, 2017 at 02:51:08AM +0300, Yury Norov wrote:
>> Hi Florian,
>>
>> Some questions and thoughts inline.
>>
>> Yury
>>
>> On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
>>> Define a generic fncpy() implementation largely based on the ARM version
>>> that requires an 8 bytes alignment for the destination address where to
>>> copy this function as well as the function's own address.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>>  include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 93 insertions(+)
>>>  create mode 100644 include/asm-generic/fncpy.h
> 
> One else thing I forgot to ask - now you have the generic
> implementation for fncpy(), so do you really need to save arm
> version of it?

Yes, it needs to deal with the Thumb bit, and there is no point in
making the generic implementation extremely flexible to support that
special case.

Thanks
-- 
Florian

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Russell King - ARM Linux]

On Mon, Jun 19, 2017 at 06:18:18PM +0300, Yury Norov wrote:
> One else thing I forgot to ask - now you have the generic
> implementation for fncpy(), so do you really need to save arm
> version of it?

This was covered in the review of v1, which took the ARM version
and incorrectly used it as an asm-generic implementation.

I explicitly asked Florian _not_ to copy the ARM fncpy() version
to asm-generic because it has (surprise surprise) ARM specific
behaviours that do not belong in a cross-architecture generic
version.

Namely, the ARM specific behaviour that bit 0 of a code address is
used to signal whether the code should be executed as ARM code or
as Thumb code.

This behaviour has no meaning on other architectures (eg, x86)
where code addresses are not 32-bit aligned.

So, suggesting that the ARM fncpy() should be used as an asm-generic
version is completely absurd, and just because we have an asm-generic
version also does not mean ARM should use it.

Florian's approach to providing an asm-generic version, leaving the
ARM specific version is entirely correct and appropriate.

So, in answer to your question, yes, we need _both_ an ARM specific
version and an asm-generic version, where the ARM specific version is
different from the asm-generic version.  Purely because it needs
architecture specific details.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Yury Norov]

On Mon, Jun 19, 2017 at 06:43:48PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 19, 2017 at 06:18:18PM +0300, Yury Norov wrote:
> > One else thing I forgot to ask - now you have the generic
> > implementation for fncpy(), so do you really need to save arm
> > version of it?
> 
> This was covered in the review of v1, which took the ARM version
> and incorrectly used it as an asm-generic implementation.
> 
> I explicitly asked Florian _not_ to copy the ARM fncpy() version
> to asm-generic because it has (surprise surprise) ARM specific
> behaviours that do not belong in a cross-architecture generic
> version.
> 
> Namely, the ARM specific behaviour that bit 0 of a code address is
> used to signal whether the code should be executed as ARM code or
> as Thumb code.
> 
> This behaviour has no meaning on other architectures (eg, x86)
> where code addresses are not 32-bit aligned.
> 
> So, suggesting that the ARM fncpy() should be used as an asm-generic
> version is completely absurd, and just because we have an asm-generic
> version also does not mean ARM should use it.
> 
> Florian's approach to providing an asm-generic version, leaving the
> ARM specific version is entirely correct and appropriate.
> 
> So, in answer to your question, yes, we need _both_ an ARM specific
> version and an asm-generic version, where the ARM specific version is
> different from the asm-generic version.  Purely because it needs
> architecture specific details.

Hi Russell, Florian,

Thanks for clarifications. Thumb bit is a good reason to save arm
version, and I completely agree with you in this. Sorry that missed it
in the v1 discussion.

> I explicitly asked Florian _not_ to copy the ARM fncpy() version
> to asm-generic because it has (surprise surprise) ARM specific
> behaviours that do not belong in a cross-architecture generic
> version.

But it seems that v3 does exactly that - copies arm with very small
changes. :) Maybe there are good reasons to have arm version exactly
how it looks now, but in general case, for me, some things that
it does are not needed. I mean checking the alignment of the source and
the type of destination. And after some headscratching I became even
more convinced that for the general case it would be much preferable
to write the fncpy() as regular function in .c file, not a macro, at
least to have the corresponding symbol in binary and let the assembler
code to call it, which is very probable.

Yury

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Florian Fainelli]

On 06/18/2017 04:51 PM, Yury Norov wrote:
> Hi Florian,
> 
> Some questions and thoughts inline.
> 
> Yury
> 
> On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
>> Define a generic fncpy() implementation largely based on the ARM version
>> that requires an 8 bytes alignment for the destination address where to
>> copy this function as well as the function's own address.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 93 insertions(+)
>>  create mode 100644 include/asm-generic/fncpy.h
>>
>> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
>> new file mode 100644
>> index 000000000000..ec03b83b8535
>> --- /dev/null
>> +++ b/include/asm-generic/fncpy.h
>> @@ -0,0 +1,93 @@
>> +/*
>> + * include/asm-generic/fncpy.h - helper macros for function body copying
>> + *
>> + * Copyright (C) 2011 Linaro Limited
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +/*
>> + * These macros are intended for use when there is a need to copy a low-level
>> + * function body into special memory.
>> + *
>> + * For example, when reconfiguring the SDRAM controller, the code doing the
>> + * reconfiguration may need to run from SRAM.
>> + *
>> + * NOTE: that the copied function body must be entirely self-contained and
>> + * position-independent in order for this to work properly.
>> + *
>> + * NOTE: in order for embedded literals and data to get referenced correctly,
>> + * the alignment of functions must be preserved when copying.  To ensure this,
>> + * the source and destination addresses for fncpy() must be aligned to a
>> + * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
>> + * You will typically need a ".align 3" directive in the assembler where the
>> + * function to be copied is defined, and ensure that your allocator for the
>> + * destination buffer returns 8-byte-aligned pointers.
>> + *
>> + * Typical usage example:
>> + *
>> + * extern int f(args);
>> + * extern uint32_t size_of_f;
>> + * int (*copied_f)(args);
>> + * void *sram_buffer;
>> + *
>> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
>> + *
>> + * ... later, call the function: ...
>> + *
>> + * copied_f(args);
>> + *
>> + * The size of the function to be copied can't be determined from C:
>> + * this must be determined by other means, such as adding assmbler directives
>> + * in the file where f is defined.
>> + */
>> +
>> +#ifndef __ASM_FNCPY_H
>> +#define __ASM_FNCPY_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +
>> +#include <asm/bug.h>
>> +#include <asm/cacheflush.h>
>> +
>> +/*
>> + * Minimum alignment requirement for the source and destination addresses
>> + * for function copying.
>> + */
>> +#define FNCPY_ALIGN 8
> 
> From now this is not arm-only, and it's possible that some architectures
> might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> guards.
> 
> By the way, compiler already has an information on the proper alignment.
> Maybe it's better to use it as the default value here instead of the
> hardcoded value?
> 
> #ifndef FNCPY_ALIGN
> #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> #endif
> 
>> +
>> +#define fncpy(dest_buf, funcp, size) ({					\
> 
> Do you really need to check types inside the macro? If not, you can
> declare it as function, which is better in general, with the memcpy-like
> prototype.
> 
>> +	uintptr_t __funcp_address;					\
>> +	typeof(funcp) __result;						\
>> +									\
>> +	asm("" : "=r" (__funcp_address) : "0" (funcp));			\
>> +									\
>> +	/*								\
>> +	 * Ensure alignment of source and destination addresses.	\
>> +	 */								\
>> +	BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) ||		\
> 
> People don't like new BUG_ONs. Maybe it's better to use BUILD_BUG_ON()
> at compile time and WARN_ON() at runtime?

If you have a BUILD_BUG_ON() what's the point of the WARN_ON()?

> 
>> +		(__funcp_address & (FNCPY_ALIGN - 1)));			\
> 
> There is IS_ALIGNED() macro for things like this.

Sure, makes sense.

> 
> And I frankly don't understand the 2nd check. One can imagine the
> situation when someone wants copy the function from the packed blob or
> some intermediate location were the function is unaligned, and it's
> impossible with the current implementation.

That's a good point, I am not sure if this is historical, or if there is
a reason for that from the ARM/Linux implementation. It sounds unlikely
that the function would be unaligned though considering that you'd have
to refer to the function being copied by its symbolic name, which
assumes it's in the kernel image or a module, and highly probable that
it is also aligned.

> 
>> +									\
>> +	memcpy(dest_buf, (void const *)__funcp_address, size);		\
>> +	flush_icache_range((unsigned long)(dest_buf),			\
>> +		(unsigned long)(dest_buf) + (size));			\
>> +									\
>> +	asm("" : "=r" (__result)					\
>> +		: "0" ((uintptr_t)(dest_buf)));				\
>> +									\
>> +	__result;							\
>> +})
>> +
>> +#endif /* !__ASM_FNCPY_H */
>> -- 
>> 2.9.3


-- 
Florian

[PATCH v3 2/4] asm-generic: Provide a fncpy() implementation

[Yury Norov]

On Mon, Jun 19, 2017 at 01:58:53PM -0700, Florian Fainelli wrote:
> On 06/18/2017 04:51 PM, Yury Norov wrote:
> > Hi Florian,
> > 
> > Some questions and thoughts inline.
> > 
> > Yury
> > 
> > On Fri, Jun 16, 2017 at 05:07:42PM -0700, Florian Fainelli wrote:
> >> Define a generic fncpy() implementation largely based on the ARM version
> >> that requires an 8 bytes alignment for the destination address where to
> >> copy this function as well as the function's own address.
> >>
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  include/asm-generic/fncpy.h | 93 +++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 93 insertions(+)
> >>  create mode 100644 include/asm-generic/fncpy.h
> >>
> >> diff --git a/include/asm-generic/fncpy.h b/include/asm-generic/fncpy.h
> >> new file mode 100644
> >> index 000000000000..ec03b83b8535
> >> --- /dev/null
> >> +++ b/include/asm-generic/fncpy.h
> >> @@ -0,0 +1,93 @@
> >> +/*
> >> + * include/asm-generic/fncpy.h - helper macros for function body copying
> >> + *
> >> + * Copyright (C) 2011 Linaro Limited
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License
> >> + * along with this program; if not, write to the Free Software
> >> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> >> + */
> >> +
> >> +/*
> >> + * These macros are intended for use when there is a need to copy a low-level
> >> + * function body into special memory.
> >> + *
> >> + * For example, when reconfiguring the SDRAM controller, the code doing the
> >> + * reconfiguration may need to run from SRAM.
> >> + *
> >> + * NOTE: that the copied function body must be entirely self-contained and
> >> + * position-independent in order for this to work properly.
> >> + *
> >> + * NOTE: in order for embedded literals and data to get referenced correctly,
> >> + * the alignment of functions must be preserved when copying.  To ensure this,
> >> + * the source and destination addresses for fncpy() must be aligned to a
> >> + * multiple of 8 bytes: you will be get a BUG() if this condition is not met.
> >> + * You will typically need a ".align 3" directive in the assembler where the
> >> + * function to be copied is defined, and ensure that your allocator for the
> >> + * destination buffer returns 8-byte-aligned pointers.
> >> + *
> >> + * Typical usage example:
> >> + *
> >> + * extern int f(args);
> >> + * extern uint32_t size_of_f;
> >> + * int (*copied_f)(args);
> >> + * void *sram_buffer;
> >> + *
> >> + * copied_f = fncpy(sram_buffer, &f, size_of_f);
> >> + *
> >> + * ... later, call the function: ...
> >> + *
> >> + * copied_f(args);
> >> + *
> >> + * The size of the function to be copied can't be determined from C:
> >> + * this must be determined by other means, such as adding assmbler directives
> >> + * in the file where f is defined.
> >> + */
> >> +
> >> +#ifndef __ASM_FNCPY_H
> >> +#define __ASM_FNCPY_H
> >> +
> >> +#include <linux/types.h>
> >> +#include <linux/string.h>
> >> +
> >> +#include <asm/bug.h>
> >> +#include <asm/cacheflush.h>
> >> +
> >> +/*
> >> + * Minimum alignment requirement for the source and destination addresses
> >> + * for function copying.
> >> + */
> >> +#define FNCPY_ALIGN 8
> > 
> > From now this is not arm-only, and it's possible that some architectures
> > might want to redefine it in their arch/xxx/include/asm/fncpy.h files.
> > So it will be easier for them if you'll wrap FNCPY_ALIGN here with #ifdef
> > guards.
> > 
> > By the way, compiler already has an information on the proper alignment.
> > Maybe it's better to use it as the default value here instead of the
> > hardcoded value?
> > 
> > #ifndef FNCPY_ALIGN
> > #define FNCPY_ALIGN ({void foo(); __alignof__(&foo);})
> > #endif
> > 
> >> +
> >> +#define fncpy(dest_buf, funcp, size) ({					\
> > 
> > Do you really need to check types inside the macro? If not, you can
> > declare it as function, which is better in general, with the memcpy-like
> > prototype.
> > 
> >> +	uintptr_t __funcp_address;					\
> >> +	typeof(funcp) __result;						\
> >> +									\
> >> +	asm("" : "=r" (__funcp_address) : "0" (funcp));			\
> >> +									\
> >> +	/*								\
> >> +	 * Ensure alignment of source and destination addresses.	\
> >> +	 */								\
> >> +	BUG_ON((uintptr_t)(dest_buf) & (FNCPY_ALIGN - 1) ||		\
> > 
> > People don't like new BUG_ONs. Maybe it's better to use BUILD_BUG_ON()
> > at compile time and WARN_ON() at runtime?
> 
> If you have a BUILD_BUG_ON() what's the point of the WARN_ON()?

To warn user if bad destination address comes at runtime.

> > 
> >> +		(__funcp_address & (FNCPY_ALIGN - 1)));			\
> > 
> > There is IS_ALIGNED() macro for things like this.
> 
> Sure, makes sense.
> 
> > 
> > And I frankly don't understand the 2nd check. One can imagine the
> > situation when someone wants copy the function from the packed blob or
> > some intermediate location were the function is unaligned, and it's
> > impossible with the current implementation.
> 
> That's a good point, I am not sure if this is historical, or if there is
> a reason for that from the ARM/Linux implementation. It sounds unlikely
> that the function would be unaligned though considering that you'd have
> to refer to the function being copied by its symbolic name, which
> assumes it's in the kernel image or a module, and highly probable that
> it is also aligned.
> 
> > 
> >> +									\
> >> +	memcpy(dest_buf, (void const *)__funcp_address, size);		\
> >> +	flush_icache_range((unsigned long)(dest_buf),			\
> >> +		(unsigned long)(dest_buf) + (size));			\
> >> +									\
> >> +	asm("" : "=r" (__result)					\
> >> +		: "0" ((uintptr_t)(dest_buf)));				\
> >> +									\
> >> +	__result;							\
> >> +})
> >> +
> >> +#endif /* !__ASM_FNCPY_H */
> >> -- 
> >> 2.9.3
> 
> 
> -- 
> Florian

[PATCH v3 3/4] arm64: Provide a fncpy implementation

[Florian Fainelli]

Utilize the asm-generic/fncpy.h implementation for ARM64 to allow the
use of drivers/misc/sram*.c on these platforms as well.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm64/include/asm/Kbuild | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
index a7a97a608033..5cfd822d3256 100644
--- a/arch/arm64/include/asm/Kbuild
+++ b/arch/arm64/include/asm/Kbuild
@@ -7,6 +7,7 @@ generic-y += dma-contiguous.h
 generic-y += early_ioremap.h
 generic-y += emergency-restart.h
 generic-y += errno.h
+generic-y += fncpy.h
 generic-y += hw_irq.h
 generic-y += ioctl.h
 generic-y += ioctls.h
-- 
2.9.3

[PATCH v3 4/4] misc: sram: Allow ARM64 to select SRAM_EXEC

[Florian Fainelli]

Now that ARM64 also has a fncpy() implementation, allow selection
SRAM_EXEC for ARM64 as well.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/misc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 07bbd4cc1852..ac8779278c0c 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -464,7 +464,7 @@ config SRAM
 	bool "Generic on-chip SRAM driver"
 	depends on HAS_IOMEM
 	select GENERIC_ALLOCATOR
-	select SRAM_EXEC if ARM
+	select SRAM_EXEC if ARM || ARM64
 	help
 	  This driver allows you to declare a memory region to be managed by
 	  the genalloc API. It is supposed to be used for small on-chip SRAM
-- 
2.9.3

[PATCH v3 4/4] misc: sram: Allow ARM64 to select SRAM_EXEC

[Mark Rutland]

On Fri, Jun 16, 2017 at 05:07:44PM -0700, Florian Fainelli wrote:
> Now that ARM64 also has a fncpy() implementation, allow selection
> SRAM_EXEC for ARM64 as well.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/misc/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 07bbd4cc1852..ac8779278c0c 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -464,7 +464,7 @@ config SRAM
>  	bool "Generic on-chip SRAM driver"
>  	depends on HAS_IOMEM
>  	select GENERIC_ALLOCATOR
> -	select SRAM_EXEC if ARM
> +	select SRAM_EXEC if ARM || ARM64
>  	help
>  	  This driver allows you to declare a memory region to be managed by
>  	  the genalloc API. It is supposed to be used for small on-chip SRAM

As stated in another thread [1], NAK to this patch.

Currently there are no users of this interface that we wish to enable
for arm64, and this is liable to be abused to add platform-specific
stuff that we expect to live in PSCI or other secure FW.

Until we have a user that does not fall into that bucket, I see no
reason to enable this for arm64.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/516161.html

[PATCH v3 0/4] Generalize fncpy availability

[Mark Rutland]

On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> Hi all,

Hi Florian,

> This patch series makes ARM's fncpy() implementation more generic (dropping the
> Thumb-specifics) and available in an asm-generic header file.
> 
> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
> 
> Changes in v3 (thanks Doug!):
> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
> - utilize Kbuild to provide the fncpy.h header on ARM64
> 
> Changes in v2:
> - leave the ARM implementation where it is
> - make the generic truly generic (no)
> 
> This is helpful in making SoC-specific power management code become true drivers
> that can be shared between different architectures.

Could you elaborate on what this is needed for?

My understanding was that on 32-bit, this was to handle idle / suspend
cases, whereas for arm64 that should be handled by PSCI.

what exactly do you intend to use this for?

Thanks,
Mark.

[PATCH v3 0/4] Generalize fncpy availability

[Tony Lindgren]

* Mark Rutland <mark.rutland@arm.com> [170619 05:25]:
> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> > 
> > This is helpful in making SoC-specific power management code become true drivers
> > that can be shared between different architectures.
> 
> Could you elaborate on what this is needed for?
> 
> My understanding was that on 32-bit, this was to handle idle / suspend
> cases, whereas for arm64 that should be handled by PSCI.
> 
> what exactly do you intend to use this for?

Well idle / suspend can have multiple needs such as running
core while DDR is in self-refresh mode and saving and restoring
of some context registers in that state.

Also clock drivers may need this to reprogram some core clocks.

Regards,

Tony

[PATCH v3 0/4] Generalize fncpy availability

[Florian Fainelli]

On 06/19/2017 05:24 AM, Mark Rutland wrote:
> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>> Hi all,
> 
> Hi Florian,
> 
>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>> Thumb-specifics) and available in an asm-generic header file.
>>
>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>
>> Changes in v3 (thanks Doug!):
>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>
>> Changes in v2:
>> - leave the ARM implementation where it is
>> - make the generic truly generic (no)
>>
>> This is helpful in making SoC-specific power management code become true drivers
>> that can be shared between different architectures.
> > Could you elaborate on what this is needed for?

Several uses cases come to mind:

- it could be used as a trampoline code prior to entering S2 for systems
that do not support PSCI 1.0

- any code that has a specific need to relocate a performance, security
sensitive code into SRAM and use it as another pool of memory.

> 
> My understanding was that on 32-bit, this was to handle idle / suspend
> cases, whereas for arm64 that should be handled by PSCI.

For systems that support PSCI 1.0, I agree, but it may not be possible
to update those systems easily, still use case 2 is completely valid.

> 
> what exactly do you intend to use this for?

At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)
which are PSCI 0.2 only. And yes, we do have a plan to evaluate
upgrading to PSCI 1.0, but in general, any SoC which as an addressable
SRAM could use it for whatever purpose it sees fit.
-- 
Florian

[PATCH v3 0/4] Generalize fncpy availability

[Lorenzo Pieralisi]

[+Sudeep]

On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
> On 06/19/2017 05:24 AM, Mark Rutland wrote:
> > On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> >> Hi all,
> > 
> > Hi Florian,
> > 
> >> This patch series makes ARM's fncpy() implementation more generic (dropping the
> >> Thumb-specifics) and available in an asm-generic header file.
> >>
> >> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
> >>
> >> Changes in v3 (thanks Doug!):
> >> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
> >> - utilize Kbuild to provide the fncpy.h header on ARM64
> >>
> >> Changes in v2:
> >> - leave the ARM implementation where it is
> >> - make the generic truly generic (no)
> >>
> >> This is helpful in making SoC-specific power management code become true drivers
> >> that can be shared between different architectures.
> > > Could you elaborate on what this is needed for?
> 
> Several uses cases come to mind:
> 
> - it could be used as a trampoline code prior to entering S2 for systems
> that do not support PSCI 1.0

I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
states through platform specific hooks on PSCI based systems, consider
upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
states that allow to achieve same power savings as PM_SUSPEND_MEM
by just entering suspend-to-idle).

> - any code that has a specific need to relocate a performance, security
> sensitive code into SRAM and use it as another pool of memory.
> 
> > 
> > My understanding was that on 32-bit, this was to handle idle / suspend
> > cases, whereas for arm64 that should be handled by PSCI.
> 
> For systems that support PSCI 1.0, I agree, but it may not be possible
> to update those systems easily, still use case 2 is completely valid.

Just to be clear, thinking of using platform specific suspend hooks
on PSCI systems is not a viable solution, I will let other people
comment on option 2.

> > what exactly do you intend to use this for?
> 
> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)

"At the moment", where ?

> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
> SRAM could use it for whatever purpose it sees fit.

Not to implement suspend hooks on PSCI 0.2 systems.

Thanks,
Lorenzo

[PATCH v3 0/4] Generalize fncpy availability

[Florian Fainelli]

On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
> [+Sudeep]
> 
> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>> Hi all,
>>>
>>> Hi Florian,
>>>
>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>
>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>
>>>> Changes in v3 (thanks Doug!):
>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>
>>>> Changes in v2:
>>>> - leave the ARM implementation where it is
>>>> - make the generic truly generic (no)
>>>>
>>>> This is helpful in making SoC-specific power management code become true drivers
>>>> that can be shared between different architectures.
>>>> Could you elaborate on what this is needed for?
>>
>> Several uses cases come to mind:
>>
>> - it could be used as a trampoline code prior to entering S2 for systems
>> that do not support PSCI 1.0
> 
> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
> states through platform specific hooks on PSCI based systems, consider
> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
> states that allow to achieve same power savings as PM_SUSPEND_MEM
> by just entering suspend-to-idle).

S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
read it. I would rather we update to PSCI 1.0 (at least) to properly
support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
state into CPU_SUSPEND since that seems wrong.

> 
>> - any code that has a specific need to relocate a performance, security
>> sensitive code into SRAM and use it as another pool of memory.
>>
>>>
>>> My understanding was that on 32-bit, this was to handle idle / suspend
>>> cases, whereas for arm64 that should be handled by PSCI.
>>
>> For systems that support PSCI 1.0, I agree, but it may not be possible
>> to update those systems easily, still use case 2 is completely valid.
> 
> Just to be clear, thinking of using platform specific suspend hooks
> on PSCI systems is not a viable solution, I will let other people
> comment on option 2.
> 
>>> what exactly do you intend to use this for?
>>
>> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)
> 
> "At the moment", where ?

Obviously not in tree, since fncpy() is not available on ARM64, here is
what it looks like for ARM & MIPS systems though:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/513953.html

> 
>> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
>> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
>> SRAM could use it for whatever purpose it sees fit.
> 
> Not to implement suspend hooks on PSCI 0.2 systems.

You have made your point and it is very valid, still that does not mean
fncpy() does not have any usefulness on ARM64 systems, this was one
although not "ARM approved" use case (system suspend), but use case
where you have code that should execute from SRAM is something that
should be possible using the standard facilities offered in
drives/misc/sram*.c
-- 
Florian

[PATCH v3 0/4] Generalize fncpy availability

[Lorenzo Pieralisi]

On Tue, Jun 20, 2017 at 09:20:34AM -0700, Florian Fainelli wrote:
> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
> > [+Sudeep]
> > 
> > On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
> >> On 06/19/2017 05:24 AM, Mark Rutland wrote:
> >>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
> >>>> Hi all,
> >>>
> >>> Hi Florian,
> >>>
> >>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
> >>>> Thumb-specifics) and available in an asm-generic header file.
> >>>>
> >>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
> >>>>
> >>>> Changes in v3 (thanks Doug!):
> >>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
> >>>> - utilize Kbuild to provide the fncpy.h header on ARM64
> >>>>
> >>>> Changes in v2:
> >>>> - leave the ARM implementation where it is
> >>>> - make the generic truly generic (no)
> >>>>
> >>>> This is helpful in making SoC-specific power management code become true drivers
> >>>> that can be shared between different architectures.
> >>>> Could you elaborate on what this is needed for?
> >>
> >> Several uses cases come to mind:
> >>
> >> - it could be used as a trampoline code prior to entering S2 for systems
> >> that do not support PSCI 1.0
> > 
> > I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
> > states through platform specific hooks on PSCI based systems, consider
> > upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
> > states that allow to achieve same power savings as PM_SUSPEND_MEM
> > by just entering suspend-to-idle).
> 
> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
> read it. I would rather we update to PSCI 1.0 (at least) to properly
> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
> state into CPU_SUSPEND since that seems wrong.

I am not asking to retrofit anything, just implementing CPU_SUSPEND
according to platform capabilities, suspend-to-idle would allow you to
enter a system suspend state where all cores are in the deepest idle
state (which, as far as the core is concerned is identical to the state
reached on S2R). Upgrading to PSCI 1.0 is what has to be done anyway.

> >> - any code that has a specific need to relocate a performance, security
> >> sensitive code into SRAM and use it as another pool of memory.
> >>
> >>>
> >>> My understanding was that on 32-bit, this was to handle idle / suspend
> >>> cases, whereas for arm64 that should be handled by PSCI.
> >>
> >> For systems that support PSCI 1.0, I agree, but it may not be possible
> >> to update those systems easily, still use case 2 is completely valid.
> > 
> > Just to be clear, thinking of using platform specific suspend hooks
> > on PSCI systems is not a viable solution, I will let other people
> > comment on option 2.
> > 
> >>> what exactly do you intend to use this for?
> >>
> >> At the moment we use it to enter S2 on ARM64 systems (ARCH_BRCMSTB)
> > 
> > "At the moment", where ?
> 
> Obviously not in tree, since fncpy() is not available on ARM64, here is
> what it looks like for ARM & MIPS systems though:
> 
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-June/513953.html
> 
> > 
> >> which are PSCI 0.2 only. And yes, we do have a plan to evaluate
> >> upgrading to PSCI 1.0, but in general, any SoC which as an addressable
> >> SRAM could use it for whatever purpose it sees fit.
> > 
> > Not to implement suspend hooks on PSCI 0.2 systems.
> 
> You have made your point and it is very valid, still that does not mean
> fncpy() does not have any usefulness on ARM64 systems, this was one
> although not "ARM approved" use case (system suspend), but use case
> where you have code that should execute from SRAM is something that
> should be possible using the standard facilities offered in
> drives/misc/sram*.c

fncpy() won't be used to implement a suspend ops hook on ARM64, that's
the point I made so that it is clear from the beginning.

Thanks,
Lorenzo

[PATCH v3 0/4] Generalize fncpy availability

[Sudeep Holla]

On 20/06/17 17:20, Florian Fainelli wrote:
> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
>> [+Sudeep]
>>
>> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>>> Hi all,
>>>>
>>>> Hi Florian,
>>>>
>>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>>
>>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>>
>>>>> Changes in v3 (thanks Doug!):
>>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>>
>>>>> Changes in v2:
>>>>> - leave the ARM implementation where it is
>>>>> - make the generic truly generic (no)
>>>>>
>>>>> This is helpful in making SoC-specific power management code become true drivers
>>>>> that can be shared between different architectures.
>>>>> Could you elaborate on what this is needed for?
>>>
>>> Several uses cases come to mind:
>>>
>>> - it could be used as a trampoline code prior to entering S2 for systems
>>> that do not support PSCI 1.0
>>
>> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
>> states through platform specific hooks on PSCI based systems, consider
>> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
>> states that allow to achieve same power savings as PM_SUSPEND_MEM
>> by just entering suspend-to-idle).
> 
> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
> read it. I would rather we update to PSCI 1.0 (at least) to properly
> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
> state into CPU_SUSPEND since that seems wrong.
> 

This has been discussed multiple times in the past. No one has come back
with strong reason to add that to the PSCI SYSTEM_SUSPEND API.

Care to explain the difference between PM_SUSPEND_STANDBY and S3 is
PM_SUSPEND_MEM on your platform. And why it can't be achieved with
suspend-to-idle ?

You can always report any issue with PSCI specification at
errata at arm.com as mentioned in the document.
-- 
Regards,
Sudeep

[PATCH v3 0/4] Generalize fncpy availability

[Florian Fainelli]

On 06/20/2017 09:54 AM, Sudeep Holla wrote:
> 
> 
> On 20/06/17 17:20, Florian Fainelli wrote:
>> On 06/20/2017 02:10 AM, Lorenzo Pieralisi wrote:
>>> [+Sudeep]
>>>
>>> On Mon, Jun 19, 2017 at 10:32:38AM -0700, Florian Fainelli wrote:
>>>> On 06/19/2017 05:24 AM, Mark Rutland wrote:
>>>>> On Fri, Jun 16, 2017 at 05:07:40PM -0700, Florian Fainelli wrote:
>>>>>> Hi all,
>>>>>
>>>>> Hi Florian,
>>>>>
>>>>>> This patch series makes ARM's fncpy() implementation more generic (dropping the
>>>>>> Thumb-specifics) and available in an asm-generic header file.
>>>>>>
>>>>>> Tested on a Broadcom ARM64 STB platform with code that is written to SRAM.
>>>>>>
>>>>>> Changes in v3 (thanks Doug!):
>>>>>> - correct include guard names in asm-generic/fncpy.h to __ASM_FNCPY_H
>>>>>> - utilize Kbuild to provide the fncpy.h header on ARM64
>>>>>>
>>>>>> Changes in v2:
>>>>>> - leave the ARM implementation where it is
>>>>>> - make the generic truly generic (no)
>>>>>>
>>>>>> This is helpful in making SoC-specific power management code become true drivers
>>>>>> that can be shared between different architectures.
>>>>>> Could you elaborate on what this is needed for?
>>>>
>>>> Several uses cases come to mind:
>>>>
>>>> - it could be used as a trampoline code prior to entering S2 for systems
>>>> that do not support PSCI 1.0
>>>
>>> I think S2 here means PM_SUSPEND_MEM. It is very wrong to manage power
>>> states through platform specific hooks on PSCI based systems, consider
>>> upgrading to PSCI 1.0 please (or implement PSCI CPU_SUSPEND power
>>> states that allow to achieve same power savings as PM_SUSPEND_MEM
>>> by just entering suspend-to-idle).
>>
>> S2 is PM_SUSPEND_STANDBY and S3 is PM_SUSPEND_MEM, at least that how I
>> read it. I would rather we update to PSCI 1.0 (at least) to properly
>> support SYSTEM_SUSPEND rather than retrofitting a system-wide suspend
>> state into CPU_SUSPEND since that seems wrong.
>>
> 
> This has been discussed multiple times in the past. No one has come back
> with strong reason to add that to the PSCI SYSTEM_SUSPEND API.
> 
> Care to explain the difference between PM_SUSPEND_STANDBY and S3 is
> PM_SUSPEND_MEM on your platform. And why it can't be achieved with
> suspend-to-idle ?

S2 preserves the ON/OFF island power and allows wake logic to wake the
system (infrared, GPIOs, Wake-on-LAN/WLAN, etc.) whereas S3 allows
powering off the ON/OFF island entirely, and allows for a lower power
consumption, with a subset of the wake peripherals to actually wake the
system. S5 is also implemented although its use case is narrower (soft-off).

The higher latency involved in S3 entry/exit is totally accepted due to
the higher power savings that it yields.

> 
> You can always report any issue with PSCI specification at
> errata at arm.com as mentioned in the document.

I just did because there are a few other things missing.
-- 
Florian

[PATCH v3 0/4] Generalize fncpy availability

[David Howells]

This should be have something put in the Documentation/ directory.

David