misc cleanups

misc cleanups

[Christoph Hellwig]

Hi Palmer,

below is a number of small cleanups and improvements that started
piling up during work to support the RISC-V kernel in nommu port.
While that port isn't quite ready yet these patches should be useful
on their own.

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

[PATCH 1/9] riscv: use asm-generic/extable.h

[Christoph Hellwig]

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/Kbuild    | 1 +
 arch/riscv/include/asm/uaccess.h | 7 +------
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index cccd12cf27d4..5a7a19d9aa7f 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -4,6 +4,7 @@ generic-y += compat.h
 generic-y += cputime.h
 generic-y += device.h
 generic-y += div64.h
+generic-y += extable.h
 generic-y += dma.h
 generic-y += dma-contiguous.h
 generic-y += dma-mapping.h
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index fb53a8089e76..cc5b253d4c57 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -23,6 +23,7 @@
 #include <linux/compiler.h>
 #include <linux/thread_info.h>
 #include <asm/byteorder.h>
+#include <asm/extable.h>
 #include <asm/asm.h>
 
 #define __enable_user_access()							\
@@ -98,12 +99,6 @@ static inline int __access_ok(unsigned long addr, unsigned long size)
  * on our cache or tlb entries.
  */
 
-struct exception_table_entry {
-	unsigned long insn, fixup;
-};
-
-extern int fixup_exception(struct pt_regs *state);
-
 #if defined(__LITTLE_ENDIAN)
 #define __MSW	1
 #define __LSW	0
-- 
2.20.1


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

Re: [PATCH 1/9] riscv: use asm-generic/extable.h

[Paul Walmsley]

On Thu, 11 Apr 2019, Christoph Hellwig wrote:

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Usually we'd nag about 

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n102


Otherwise,

Reviewed-by: Paul Walmsley <paul.walmsley@sifive.com>


- Paul

> ---
>  arch/riscv/include/asm/Kbuild    | 1 +
>  arch/riscv/include/asm/uaccess.h | 7 +------
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
> index cccd12cf27d4..5a7a19d9aa7f 100644
> --- a/arch/riscv/include/asm/Kbuild
> +++ b/arch/riscv/include/asm/Kbuild
> @@ -4,6 +4,7 @@ generic-y += compat.h
>  generic-y += cputime.h
>  generic-y += device.h
>  generic-y += div64.h
> +generic-y += extable.h
>  generic-y += dma.h
>  generic-y += dma-contiguous.h
>  generic-y += dma-mapping.h
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index fb53a8089e76..cc5b253d4c57 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -23,6 +23,7 @@
>  #include <linux/compiler.h>
>  #include <linux/thread_info.h>
>  #include <asm/byteorder.h>
> +#include <asm/extable.h>
>  #include <asm/asm.h>
>  
>  #define __enable_user_access()							\
> @@ -98,12 +99,6 @@ static inline int __access_ok(unsigned long addr, unsigned long size)
>   * on our cache or tlb entries.
>   */
>  
> -struct exception_table_entry {
> -	unsigned long insn, fixup;
> -};
> -
> -extern int fixup_exception(struct pt_regs *state);
> -
>  #if defined(__LITTLE_ENDIAN)
>  #define __MSW	1
>  #define __LSW	0
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

[PATCH 2/9] riscv: remove dead big endian code

[Christoph Hellwig]

RISC-V is always little endian.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/uaccess.h | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index cc5b253d4c57..5c50ed6f8c93 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -99,15 +99,8 @@ static inline int __access_ok(unsigned long addr, unsigned long size)
  * on our cache or tlb entries.
  */
 
-#if defined(__LITTLE_ENDIAN)
-#define __MSW	1
 #define __LSW	0
-#elif defined(__BIG_ENDIAN)
-#define __MSW	0
-#define	__LSW	1
-#else
-#error "Unknown endianness"
-#endif
+#define __MSW	1
 
 /*
  * The "__xxx" versions of the user access functions do not verify the address
-- 
2.20.1


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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Nick Kossifidis]

Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
> RISC-V is always little endian.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/include/asm/uaccess.h | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h 
> b/arch/riscv/include/asm/uaccess.h
> index cc5b253d4c57..5c50ed6f8c93 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -99,15 +99,8 @@ static inline int __access_ok(unsigned long addr,
> unsigned long size)
>   * on our cache or tlb entries.
>   */
> 
> -#if defined(__LITTLE_ENDIAN)
> -#define __MSW	1
>  #define __LSW	0
> -#elif defined(__BIG_ENDIAN)
> -#define __MSW	0
> -#define	__LSW	1
> -#else
> -#error "Unknown endianness"
> -#endif
> +#define __MSW	1
> 
>  /*
>   * The "__xxx" versions of the user access functions do not verify the 
> address

 From the ISA manual:

"We chose little-endian byte ordering for the RISC-V memory system 
because
little-endian systems are currently dominant commercially (all x86 
systems;
iOS, Android, and Windows for ARM). A minor point is that we have also 
found
little-endian memory systems to be more natural for hardware designers.
However, certain application areas, such as IP networking, operate on
big-endian data structures, and certain legacy code bases have been 
built
assuming big-endian processors, so we expect that future specifications 
will
describe big-endian or bi-endian variants of RISC-V."

I don't think we can definitely say that RISC-V will always have a 
little-endian,
memory system only that it is little-endian for now. Also this code acts 
as a check,
I'd prefer if you put an error (something like "unsupported endianess, 
check your
toolchain") in case __BIG_ENDIAN was defined instead of completely 
removing the
check.

Regards,
Nick

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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 06:40:07PM +0300, Nick Kossifidis wrote:
> I don't think we can definitely say that RISC-V will always have a 
> little-endian,
> memory system only that it is little-endian for now. Also this code acts as 
> a check,

And we don't know if Linux will be around if that ever changes.

The point is:

 a) the current RISC-V spec is LE only
 b) the current linux port is LE only except for this little bit

There is no point in leaving just this bitrotting code around.  It
just confuses developers, (very very slightly) slows down compiles
and will bitrot.  It also won't be any significant help to a future
developer down the road doing a hypothetical BE RISC-V Linux port.

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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Nick Kossifidis]

Στις 2019-04-11 18:47, Christoph Hellwig έγραψε:
> On Thu, Apr 11, 2019 at 06:40:07PM +0300, Nick Kossifidis wrote:
>> I don't think we can definitely say that RISC-V will always have a
>> little-endian,
>> memory system only that it is little-endian for now. Also this code 
>> acts as
>> a check,
> 
> And we don't know if Linux will be around if that ever changes.
> 
> The point is:
> 
>  a) the current RISC-V spec is LE only
>  b) the current linux port is LE only except for this little bit
> 
> There is no point in leaving just this bitrotting code around.  It
> just confuses developers, (very very slightly) slows down compiles
> and will bitrot.  It also won't be any significant help to a future
> developer down the road doing a hypothetical BE RISC-V Linux port.

I think we should at least warn the user about unsupported endianess,
also I suggest you also clean up include/asm/elf.h.

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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 07:08:30PM +0300, Nick Kossifidis wrote:
> I think we should at least warn the user about unsupported endianess,

The endianess is selected by arch/riscv/include/uapi/asm/byteorder.h
including include/uapi/linux/byteorder/little_endian.h, which then
defines __LITTLE_ENDIAN.  So we'd really need to check one part of
the kernel port agrees with others, which is a little odd.  And in
which place do we check that and in which one not?  It isn't like
other ports like x86 are full of such just because checks..

> also I suggest you also clean up include/asm/elf.h.

Agreed.

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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Nick Kossifidis]

Στις 2019-04-11 19:31, Christoph Hellwig έγραψε:
> On Thu, Apr 11, 2019 at 07:08:30PM +0300, Nick Kossifidis wrote:
>> I think we should at least warn the user about unsupported endianess,
> 
> The endianess is selected by arch/riscv/include/uapi/asm/byteorder.h
> including include/uapi/linux/byteorder/little_endian.h, which then
> defines __LITTLE_ENDIAN.  So we'd really need to check one part of
> the kernel port agrees with others, which is a little odd.  And in
> which place do we check that and in which one not?  It isn't like
> other ports like x86 are full of such just because checks..
> 

You got a point there, how about something like:

#if __BYTE_ORDER__ != __ORDER_LITTLE_ENDIAN__
#error "Unsupported endianess, check your toolchain"
#endif

on arch/riscv/include/uapi/asm/byteorder.h ?

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

Re: [PATCH 2/9] riscv: remove dead big endian code

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 07:47:20PM +0300, Nick Kossifidis wrote:
>> The endianess is selected by arch/riscv/include/uapi/asm/byteorder.h
>> including include/uapi/linux/byteorder/little_endian.h, which then
>> defines __LITTLE_ENDIAN.  So we'd really need to check one part of
>> the kernel port agrees with others, which is a little odd.  And in
>> which place do we check that and in which one not?  It isn't like
>> other ports like x86 are full of such just because checks..
>>
>
> You got a point there, how about something like:
>
> #if __BYTE_ORDER__ != __ORDER_LITTLE_ENDIAN__
> #error "Unsupported endianess, check your toolchain"
> #endif
>
> on arch/riscv/include/uapi/asm/byteorder.h ?

I'm not really sold on it.  But I can do it as a separate patch and
we'll see if anyone likes it..

In fact it might make sense to just add this to the generic
byte order headers so that RISC-V doesn't stand out too much, which
would seem a little more sensible.

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

[PATCH 3/9] riscv: remove CONFIG_RISCV_ISA_A

[Christoph Hellwig]

This option is always enabled, and not supporting the A extensions would
create a complete ABI trainwreck, so there is point in even slightly
encouraging such an idea by keeping this unselectable code around.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/Kconfig             |  6 +-----
 arch/riscv/Makefile            |  5 ++---
 arch/riscv/include/asm/futex.h | 13 -------------
 3 files changed, 3 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index eb56c82d8aa1..a65b2a43f7ba 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -27,7 +27,7 @@ config RISCV
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
 	select GENERIC_SMP_IDLE_THREAD
-	select GENERIC_ATOMIC64 if !64BIT || !RISCV_ISA_A
+	select GENERIC_ATOMIC64 if !64BIT
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_MEMBLOCK_NODE_MAP
 	select HAVE_DMA_CONTIGUOUS
@@ -35,7 +35,6 @@ config RISCV
 	select HAVE_PERF_EVENTS
 	select HAVE_SYSCALL_TRACEPOINTS
 	select IRQ_DOMAIN
-	select RISCV_ISA_A if SMP
 	select SPARSE_IRQ
 	select SYSCTL_EXCEPTION_TRACE
 	select HAVE_ARCH_TRACEHOOK
@@ -197,9 +196,6 @@ config RISCV_ISA_C
 
 	   If you don't know what to do here, say Y.
 
-config RISCV_ISA_A
-	def_bool y
-
 menu "supported PMU type"
 	depends on PERF_EVENTS
 
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index c6342e638ef7..6b0741c9f348 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -39,9 +39,8 @@ endif
 KBUILD_CFLAGS += -Wall
 
 # ISA string setting
-riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32im
-riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64im
-riscv-march-$(CONFIG_RISCV_ISA_A)	:= $(riscv-march-y)a
+riscv-march-$(CONFIG_ARCH_RV32I)	:= rv32ima
+riscv-march-$(CONFIG_ARCH_RV64I)	:= rv64ima
 riscv-march-$(CONFIG_FPU)		:= $(riscv-march-y)fd
 riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
 KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
diff --git a/arch/riscv/include/asm/futex.h b/arch/riscv/include/asm/futex.h
index 66641624d8a5..4ad6409c4647 100644
--- a/arch/riscv/include/asm/futex.h
+++ b/arch/riscv/include/asm/futex.h
@@ -7,18 +7,6 @@
 #ifndef _ASM_FUTEX_H
 #define _ASM_FUTEX_H
 
-#ifndef CONFIG_RISCV_ISA_A
-/*
- * Use the generic interrupt disabling versions if the A extension
- * is not supported.
- */
-#ifdef CONFIG_SMP
-#error "Can't support generic futex calls without A extension on SMP"
-#endif
-#include <asm-generic/futex.h>
-
-#else /* CONFIG_RISCV_ISA_A */
-
 #include <linux/futex.h>
 #include <linux/uaccess.h>
 #include <linux/errno.h>
@@ -124,5 +112,4 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 	return ret;
 }
 
-#endif /* CONFIG_RISCV_ISA_A */
 #endif /* _ASM_FUTEX_H */
-- 
2.20.1


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

Re: [PATCH 3/9] riscv: remove CONFIG_RISCV_ISA_A

[Andreas Schwab]

On Apr 11 2019, Christoph Hellwig <hch@lst.de> wrote:

> This option is always enabled, and not supporting the A extensions would
> create a complete ABI trainwreck, so there is point in even slightly

+no

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

Re: [PATCH 3/9] riscv: remove CONFIG_RISCV_ISA_A

[Palmer Dabbelt]

On Thu, 11 Apr 2019 05:21:23 PDT (-0700), schwab@suse.de wrote:
> On Apr 11 2019, Christoph Hellwig <hch@lst.de> wrote:
>
>> This option is always enabled, and not supporting the A extensions would
>> create a complete ABI trainwreck, so there is point in even slightly
>
> +no

Sorry, but I can't parse your answer.  Do you agree with or disagree with the 
proposed patch?

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

Re: [PATCH 3/9] riscv: remove CONFIG_RISCV_ISA_A

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 06:23:23PM -0700, Palmer Dabbelt wrote:
> On Thu, 11 Apr 2019 05:21:23 PDT (-0700), schwab@suse.de wrote:
>> On Apr 11 2019, Christoph Hellwig <hch@lst.de> wrote:
>>
>>> This option is always enabled, and not supporting the A extensions would
>>> create a complete ABI trainwreck, so there is point in even slightly
>>
>> +no
>
> Sorry, but I can't parse your answer.  Do you agree with or disagree with 
> the proposed patch?

They way I understood it it is that he wants me to insert a no into the
sentence above where it is missing to make sense:

".., so there is _no_ point in even slightly.."

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

[PATCH 4/9] riscv: turn mm_segment_t into a struct

[Christoph Hellwig]

This matches what other heavily used architectures do, and will allow us
to easily use <asm-generic/uaccess.h> for the nommu case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/thread_info.h |  4 +++-
 arch/riscv/include/asm/uaccess.h     | 12 +++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
index 1c9cc8389928..9c039870019b 100644
--- a/arch/riscv/include/asm/thread_info.h
+++ b/arch/riscv/include/asm/thread_info.h
@@ -28,7 +28,9 @@
 #include <asm/processor.h>
 #include <asm/csr.h>
 
-typedef unsigned long mm_segment_t;
+typedef struct {
+	unsigned long seg;
+} mm_segment_t;
 
 /*
  * low level task data that entry.S needs immediate access to
diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 5c50ed6f8c93..b26f407be5c8 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -39,8 +39,10 @@
  * For historical reasons, these macros are grossly misnamed.
  */
 
-#define KERNEL_DS	(~0UL)
-#define USER_DS		(TASK_SIZE)
+#define MAKE_MM_SEG(s)	((mm_segment_t) { (s) })
+
+#define KERNEL_DS	MAKE_MM_SEG(~0UL)
+#define USER_DS		MAKE_MM_SEG(TASK_SIZE)
 
 #define get_fs()	(current_thread_info()->addr_limit)
 
@@ -49,9 +51,9 @@ static inline void set_fs(mm_segment_t fs)
 	current_thread_info()->addr_limit = fs;
 }
 
-#define segment_eq(a, b) ((a) == (b))
+#define segment_eq(a, b) ((a).seg == (b).seg)
 
-#define user_addr_max()	(get_fs())
+#define user_addr_max()	(get_fs().seg)
 
 
 /**
@@ -83,7 +85,7 @@ static inline int __access_ok(unsigned long addr, unsigned long size)
 {
 	const mm_segment_t fs = get_fs();
 
-	return (size <= fs) && (addr <= (fs - size));
+	return size <= fs.seg && addr <= fs.seg - size;
 }
 
 /*
-- 
2.20.1


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

[PATCH 5/9] riscv: simplify stack pointer setup in head.S

[Christoph Hellwig]

We don't need THREAD_SIZE in asm-offsets.c as we can just calculate
the value of init_thread_union + THREAD_SIZE using cpp, just like
we do a few lines above.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/asm-offsets.c | 3 ---
 arch/riscv/kernel/head.S        | 5 +----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index dac98348c6a3..578bb5efc085 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -312,9 +312,6 @@ void asm_offsets(void)
 		- offsetof(struct task_struct, thread.fstate.f[0])
 	);
 
-	/* The assembler needs access to THREAD_SIZE as well. */
-	DEFINE(ASM_THREAD_SIZE, THREAD_SIZE);
-
 	/*
 	 * We allocate a pt_regs on the stack when entering the kernel.  This
 	 * ensures the alignment is sane.
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index fe884cd69abd..9344d9ceda24 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -68,10 +68,7 @@ clear_bss_done:
 	/* Restore C environment */
 	la tp, init_task
 	sw zero, TASK_TI_CPU(tp)
-
-	la sp, init_thread_union
-	li a0, ASM_THREAD_SIZE
-	add sp, sp, a0
+	la sp, init_thread_union + THREAD_SIZE
 
 	/* Start the kernel */
 	mv a0, s0
-- 
2.20.1


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

Re: [PATCH 5/9] riscv: simplify stack pointer setup in head.S

[Atish Patra]

On 4/11/19 4:57 AM, Christoph Hellwig wrote:
> We don't need THREAD_SIZE in asm-offsets.c as we can just calculate
> the value of init_thread_union + THREAD_SIZE using cpp, just like
> we do a few lines above.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/kernel/asm-offsets.c | 3 ---
>   arch/riscv/kernel/head.S        | 5 +----
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index dac98348c6a3..578bb5efc085 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -312,9 +312,6 @@ void asm_offsets(void)
>   		- offsetof(struct task_struct, thread.fstate.f[0])
>   	);
>   
> -	/* The assembler needs access to THREAD_SIZE as well. */
> -	DEFINE(ASM_THREAD_SIZE, THREAD_SIZE);
> -
>   	/*
>   	 * We allocate a pt_regs on the stack when entering the kernel.  This
>   	 * ensures the alignment is sane.
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index fe884cd69abd..9344d9ceda24 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -68,10 +68,7 @@ clear_bss_done:
>   	/* Restore C environment */
>   	la tp, init_task
>   	sw zero, TASK_TI_CPU(tp)
> -
> -	la sp, init_thread_union
> -	li a0, ASM_THREAD_SIZE
> -	add sp, sp, a0
> +	la sp, init_thread_union + THREAD_SIZE
>   
>   	/* Start the kernel */
>   	mv a0, s0
> 
Reviewed-by: Atish Patra <atish.patra@wdc.com>

Regards,
Atish

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

[PATCH 6/9] riscv: also clear all pending interrupts when booting

[Christoph Hellwig]

Just in case an old interrupt is pending make sure we clear everything
asserted before this kernel started.  Based on similar M-mode code in
opensbi.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/head.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 9344d9ceda24..62d2c9c85433 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -22,8 +22,9 @@
 
 __INIT
 ENTRY(_start)
-	/* Mask all interrupts */
+	/* Mask and clear all interrupts */
 	csrw sie, zero
+	csrw sip, zero
 
 	/* Load the global pointer */
 .option push
-- 
2.20.1


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

Re: [PATCH 6/9] riscv: also clear all pending interrupts when booting

[Nick Kossifidis]

Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
> Just in case an old interrupt is pending make sure we clear everything
> asserted before this kernel started.  Based on similar M-mode code in
> opensbi.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/kernel/head.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 9344d9ceda24..62d2c9c85433 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -22,8 +22,9 @@
> 
>  __INIT
>  ENTRY(_start)
> -	/* Mask all interrupts */
> +	/* Mask and clear all interrupts */
>  	csrw sie, zero
> +	csrw sip, zero
> 
>  	/* Load the global pointer */
>  .option push

Reviewed-by: Nick Kossifidis <mick@ics.forth.gr>

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

Re: [PATCH 6/9] riscv: also clear all pending interrupts when booting

[Atish Patra]

On 4/11/19 4:57 AM, Christoph Hellwig wrote:
> Just in case an old interrupt is pending make sure we clear everything
> asserted before this kernel started.  Based on similar M-mode code in
> opensbi.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/kernel/head.S | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 9344d9ceda24..62d2c9c85433 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -22,8 +22,9 @@
>   
>   __INIT
>   ENTRY(_start)
> -	/* Mask all interrupts */
> +	/* Mask and clear all interrupts */
>   	csrw sie, zero
> +	csrw sip, zero
>   
>   	/* Load the global pointer */
>   .option push
> 

Reviewed-by: Atish Patra <atish.patra@wdc.com>

Regards,
Atish

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

[PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Christoph Hellwig]

No need to have two names for the same thing.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/include/asm/ptrace.h | 21 ++++++---------------
 arch/riscv/kernel/stacktrace.c  |  8 ++++----
 arch/riscv/kernel/traps.c       |  2 +-
 3 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
index d35ec2f41381..9c867a4bac83 100644
--- a/arch/riscv/include/asm/ptrace.h
+++ b/arch/riscv/include/asm/ptrace.h
@@ -70,47 +70,38 @@ struct pt_regs {
 
 
 /* Helpers for working with the instruction pointer */
-#define GET_IP(regs) ((regs)->sepc)
-#define SET_IP(regs, val) (GET_IP(regs) = (val))
-
 static inline unsigned long instruction_pointer(struct pt_regs *regs)
 {
-	return GET_IP(regs);
+	return regs->sepc;
 }
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
 {
-	SET_IP(regs, val);
+	regs->sepc = val;
 }
 
 #define profile_pc(regs) instruction_pointer(regs)
 
 /* Helpers for working with the user stack pointer */
-#define GET_USP(regs) ((regs)->sp)
-#define SET_USP(regs, val) (GET_USP(regs) = (val))
-
 static inline unsigned long user_stack_pointer(struct pt_regs *regs)
 {
-	return GET_USP(regs);
+	return regs->sp;
 }
 static inline void user_stack_pointer_set(struct pt_regs *regs,
 					  unsigned long val)
 {
-	SET_USP(regs, val);
+	regs->sp =  val;
 }
 
 /* Helpers for working with the frame pointer */
-#define GET_FP(regs) ((regs)->s0)
-#define SET_FP(regs, val) (GET_FP(regs) = (val))
-
 static inline unsigned long frame_pointer(struct pt_regs *regs)
 {
-	return GET_FP(regs);
+	return regs->s0;
 }
 static inline void frame_pointer_set(struct pt_regs *regs,
 				     unsigned long val)
 {
-	SET_FP(regs, val);
+	regs->s0 = val;
 }
 
 static inline unsigned long regs_return_value(struct pt_regs *regs)
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index a4b1d94371a0..25fe0ff81f9e 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -33,9 +33,9 @@ static void notrace walk_stackframe(struct task_struct *task,
 	unsigned long fp, sp, pc;
 
 	if (regs) {
-		fp = GET_FP(regs);
-		sp = GET_USP(regs);
-		pc = GET_IP(regs);
+		fp = frame_pointer(regs);
+		sp = user_stack_pointer(regs);
+		pc = instruction_pointer(regs);
 	} else if (task == NULL || task == current) {
 		const register unsigned long current_sp __asm__ ("sp");
 		fp = (unsigned long)__builtin_frame_address(0);
@@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct task_struct *task,
 
 	if (regs) {
 		sp = GET_USP(regs);
-		pc = GET_IP(regs);
+		pc = instruction_pointer(regs);
 	} else if (task == NULL || task == current) {
 		const register unsigned long current_sp __asm__ ("sp");
 		sp = current_sp;
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 24a9333dda2c..86731a2fa218 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -70,7 +70,7 @@ void do_trap(struct pt_regs *regs, int signo, int code,
 	    && printk_ratelimit()) {
 		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
 			tsk->comm, task_pid_nr(tsk), signo, code, addr);
-		print_vma_addr(KERN_CONT " in ", GET_IP(regs));
+		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
 		pr_cont("\n");
 		show_regs(regs);
 	}
-- 
2.20.1


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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Nick Kossifidis]

Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
> No need to have two names for the same thing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/include/asm/ptrace.h | 21 ++++++---------------
>  arch/riscv/kernel/stacktrace.c  |  8 ++++----
>  arch/riscv/kernel/traps.c       |  2 +-
>  3 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/ptrace.h 
> b/arch/riscv/include/asm/ptrace.h
> index d35ec2f41381..9c867a4bac83 100644
> --- a/arch/riscv/include/asm/ptrace.h
> +++ b/arch/riscv/include/asm/ptrace.h
> @@ -70,47 +70,38 @@ struct pt_regs {
> 
> 
>  /* Helpers for working with the instruction pointer */
> -#define GET_IP(regs) ((regs)->sepc)
> -#define SET_IP(regs, val) (GET_IP(regs) = (val))
> -
>  static inline unsigned long instruction_pointer(struct pt_regs *regs)
>  {
> -	return GET_IP(regs);
> +	return regs->sepc;
>  }
>  static inline void instruction_pointer_set(struct pt_regs *regs,
>  					   unsigned long val)
>  {
> -	SET_IP(regs, val);
> +	regs->sepc = val;
>  }
> 
>  #define profile_pc(regs) instruction_pointer(regs)
> 
>  /* Helpers for working with the user stack pointer */
> -#define GET_USP(regs) ((regs)->sp)
> -#define SET_USP(regs, val) (GET_USP(regs) = (val))
> -
>  static inline unsigned long user_stack_pointer(struct pt_regs *regs)
>  {
> -	return GET_USP(regs);
> +	return regs->sp;
>  }
>  static inline void user_stack_pointer_set(struct pt_regs *regs,
>  					  unsigned long val)
>  {
> -	SET_USP(regs, val);
> +	regs->sp =  val;
>  }
> 
>  /* Helpers for working with the frame pointer */
> -#define GET_FP(regs) ((regs)->s0)
> -#define SET_FP(regs, val) (GET_FP(regs) = (val))
> -
>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>  {
> -	return GET_FP(regs);
> +	return regs->s0;
>  }
>  static inline void frame_pointer_set(struct pt_regs *regs,
>  				     unsigned long val)
>  {
> -	SET_FP(regs, val);
> +	regs->s0 = val;
>  }
> 
>  static inline unsigned long regs_return_value(struct pt_regs *regs)
> diff --git a/arch/riscv/kernel/stacktrace.c 
> b/arch/riscv/kernel/stacktrace.c
> index a4b1d94371a0..25fe0ff81f9e 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -33,9 +33,9 @@ static void notrace walk_stackframe(struct 
> task_struct *task,
>  	unsigned long fp, sp, pc;
> 
>  	if (regs) {
> -		fp = GET_FP(regs);
> -		sp = GET_USP(regs);
> -		pc = GET_IP(regs);
> +		fp = frame_pointer(regs);
> +		sp = user_stack_pointer(regs);
> +		pc = instruction_pointer(regs);
>  	} else if (task == NULL || task == current) {
>  		const register unsigned long current_sp __asm__ ("sp");
>  		fp = (unsigned long)__builtin_frame_address(0);
> @@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct 
> task_struct *task,
> 
>  	if (regs) {
>  		sp = GET_USP(regs);
> -		pc = GET_IP(regs);
> +		pc = instruction_pointer(regs);
>  	} else if (task == NULL || task == current) {
>  		const register unsigned long current_sp __asm__ ("sp");
>  		sp = current_sp;
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 24a9333dda2c..86731a2fa218 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -70,7 +70,7 @@ void do_trap(struct pt_regs *regs, int signo, int 
> code,
>  	    && printk_ratelimit()) {
>  		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
>  			tsk->comm, task_pid_nr(tsk), signo, code, addr);
> -		print_vma_addr(KERN_CONT " in ", GET_IP(regs));
> +		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
>  		pr_cont("\n");
>  		show_regs(regs);
>  	}

Reviewed-by: Nick Kossifidis <mick@ics.forth.gr>

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Nick Kossifidis]

Στις 2019-04-11 18:46, Nick Kossifidis έγραψε:
> Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
>> No need to have two names for the same thing.
>> 
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>  arch/riscv/include/asm/ptrace.h | 21 ++++++---------------
>>  arch/riscv/kernel/stacktrace.c  |  8 ++++----
>>  arch/riscv/kernel/traps.c       |  2 +-
>>  3 files changed, 11 insertions(+), 20 deletions(-)
>> 
>> diff --git a/arch/riscv/include/asm/ptrace.h 
>> b/arch/riscv/include/asm/ptrace.h
>> index d35ec2f41381..9c867a4bac83 100644
>> --- a/arch/riscv/include/asm/ptrace.h
>> +++ b/arch/riscv/include/asm/ptrace.h
>> @@ -70,47 +70,38 @@ struct pt_regs {
>> 
>> 
>>  /* Helpers for working with the instruction pointer */
>> -#define GET_IP(regs) ((regs)->sepc)
>> -#define SET_IP(regs, val) (GET_IP(regs) = (val))
>> -
>>  static inline unsigned long instruction_pointer(struct pt_regs *regs)
>>  {
>> -	return GET_IP(regs);
>> +	return regs->sepc;
>>  }
>>  static inline void instruction_pointer_set(struct pt_regs *regs,
>>  					   unsigned long val)
>>  {
>> -	SET_IP(regs, val);
>> +	regs->sepc = val;
>>  }
>> 
>>  #define profile_pc(regs) instruction_pointer(regs)
>> 
>>  /* Helpers for working with the user stack pointer */
>> -#define GET_USP(regs) ((regs)->sp)
>> -#define SET_USP(regs, val) (GET_USP(regs) = (val))
>> -
>>  static inline unsigned long user_stack_pointer(struct pt_regs *regs)
>>  {
>> -	return GET_USP(regs);
>> +	return regs->sp;
>>  }
>>  static inline void user_stack_pointer_set(struct pt_regs *regs,
>>  					  unsigned long val)
>>  {
>> -	SET_USP(regs, val);
>> +	regs->sp =  val;
>>  }
>> 
>>  /* Helpers for working with the frame pointer */
>> -#define GET_FP(regs) ((regs)->s0)
>> -#define SET_FP(regs, val) (GET_FP(regs) = (val))
>> -
>>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>>  {
>> -	return GET_FP(regs);
>> +	return regs->s0;
>>  }
>>  static inline void frame_pointer_set(struct pt_regs *regs,
>>  				     unsigned long val)
>>  {
>> -	SET_FP(regs, val);
>> +	regs->s0 = val;
>>  }
>> 
>>  static inline unsigned long regs_return_value(struct pt_regs *regs)
>> diff --git a/arch/riscv/kernel/stacktrace.c 
>> b/arch/riscv/kernel/stacktrace.c
>> index a4b1d94371a0..25fe0ff81f9e 100644
>> --- a/arch/riscv/kernel/stacktrace.c
>> +++ b/arch/riscv/kernel/stacktrace.c
>> @@ -33,9 +33,9 @@ static void notrace walk_stackframe(struct 
>> task_struct *task,
>>  	unsigned long fp, sp, pc;
>> 
>>  	if (regs) {
>> -		fp = GET_FP(regs);
>> -		sp = GET_USP(regs);
>> -		pc = GET_IP(regs);
>> +		fp = frame_pointer(regs);
>> +		sp = user_stack_pointer(regs);
>> +		pc = instruction_pointer(regs);
>>  	} else if (task == NULL || task == current) {
>>  		const register unsigned long current_sp __asm__ ("sp");
>>  		fp = (unsigned long)__builtin_frame_address(0);
>> @@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct 
>> task_struct *task,
>> 
>>  	if (regs) {
>>  		sp = GET_USP(regs);

Ah just noticed this, it should be user_stack_pointer(regs) instead 
since you removed
GET_USP.

>> -		pc = GET_IP(regs);
>> +		pc = instruction_pointer(regs);
>>  	} else if (task == NULL || task == current) {
>>  		const register unsigned long current_sp __asm__ ("sp");
>>  		sp = current_sp;
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index 24a9333dda2c..86731a2fa218 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -70,7 +70,7 @@ void do_trap(struct pt_regs *regs, int signo, int 
>> code,
>>  	    && printk_ratelimit()) {
>>  		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
>>  			tsk->comm, task_pid_nr(tsk), signo, code, addr);
>> -		print_vma_addr(KERN_CONT " in ", GET_IP(regs));
>> +		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
>>  		pr_cont("\n");
>>  		show_regs(regs);
>>  	}
> 
> Reviewed-by: Nick Kossifidis <mick@ics.forth.gr>
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Nick Kossifidis]

Στις 2019-04-11 18:55, Nick Kossifidis έγραψε:
> Στις 2019-04-11 18:46, Nick Kossifidis έγραψε:
>> Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
>>> No need to have two names for the same thing.
>>> 
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>> ---
>>>  arch/riscv/include/asm/ptrace.h | 21 ++++++---------------
>>>  arch/riscv/kernel/stacktrace.c  |  8 ++++----
>>>  arch/riscv/kernel/traps.c       |  2 +-
>>>  3 files changed, 11 insertions(+), 20 deletions(-)
>>> 
>>> diff --git a/arch/riscv/include/asm/ptrace.h 
>>> b/arch/riscv/include/asm/ptrace.h
>>> index d35ec2f41381..9c867a4bac83 100644
>>> --- a/arch/riscv/include/asm/ptrace.h
>>> +++ b/arch/riscv/include/asm/ptrace.h
>>> @@ -70,47 +70,38 @@ struct pt_regs {
>>> 
>>> 
>>>  /* Helpers for working with the instruction pointer */
>>> -#define GET_IP(regs) ((regs)->sepc)
>>> -#define SET_IP(regs, val) (GET_IP(regs) = (val))
>>> -
>>>  static inline unsigned long instruction_pointer(struct pt_regs 
>>> *regs)
>>>  {
>>> -	return GET_IP(regs);
>>> +	return regs->sepc;
>>>  }
>>>  static inline void instruction_pointer_set(struct pt_regs *regs,
>>>  					   unsigned long val)
>>>  {
>>> -	SET_IP(regs, val);
>>> +	regs->sepc = val;
>>>  }
>>> 
>>>  #define profile_pc(regs) instruction_pointer(regs)
>>> 
>>>  /* Helpers for working with the user stack pointer */
>>> -#define GET_USP(regs) ((regs)->sp)
>>> -#define SET_USP(regs, val) (GET_USP(regs) = (val))
>>> -
>>>  static inline unsigned long user_stack_pointer(struct pt_regs *regs)
>>>  {
>>> -	return GET_USP(regs);
>>> +	return regs->sp;
>>>  }
>>>  static inline void user_stack_pointer_set(struct pt_regs *regs,
>>>  					  unsigned long val)
>>>  {
>>> -	SET_USP(regs, val);
>>> +	regs->sp =  val;
>>>  }
>>> 
>>>  /* Helpers for working with the frame pointer */
>>> -#define GET_FP(regs) ((regs)->s0)
>>> -#define SET_FP(regs, val) (GET_FP(regs) = (val))
>>> -
>>>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>>>  {
>>> -	return GET_FP(regs);
>>> +	return regs->s0;
>>>  }
>>>  static inline void frame_pointer_set(struct pt_regs *regs,
>>>  				     unsigned long val)
>>>  {
>>> -	SET_FP(regs, val);
>>> +	regs->s0 = val;
>>>  }
>>> 
>>>  static inline unsigned long regs_return_value(struct pt_regs *regs)
>>> diff --git a/arch/riscv/kernel/stacktrace.c 
>>> b/arch/riscv/kernel/stacktrace.c
>>> index a4b1d94371a0..25fe0ff81f9e 100644
>>> --- a/arch/riscv/kernel/stacktrace.c
>>> +++ b/arch/riscv/kernel/stacktrace.c
>>> @@ -33,9 +33,9 @@ static void notrace walk_stackframe(struct 
>>> task_struct *task,
>>>  	unsigned long fp, sp, pc;
>>> 
>>>  	if (regs) {
>>> -		fp = GET_FP(regs);
>>> -		sp = GET_USP(regs);
>>> -		pc = GET_IP(regs);
>>> +		fp = frame_pointer(regs);
>>> +		sp = user_stack_pointer(regs);
>>> +		pc = instruction_pointer(regs);
>>>  	} else if (task == NULL || task == current) {
>>>  		const register unsigned long current_sp __asm__ ("sp");
>>>  		fp = (unsigned long)__builtin_frame_address(0);
>>> @@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct 
>>> task_struct *task,
>>> 
>>>  	if (regs) {
>>>  		sp = GET_USP(regs);
> 
> Ah just noticed this, it should be user_stack_pointer(regs) instead
> since you removed
> GET_USP.
> 
>>> -		pc = GET_IP(regs);
>>> +		pc = instruction_pointer(regs);
>>>  	} else if (task == NULL || task == current) {
>>>  		const register unsigned long current_sp __asm__ ("sp");
>>>  		sp = current_sp;
>>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>>> index 24a9333dda2c..86731a2fa218 100644
>>> --- a/arch/riscv/kernel/traps.c
>>> +++ b/arch/riscv/kernel/traps.c
>>> @@ -70,7 +70,7 @@ void do_trap(struct pt_regs *regs, int signo, int 
>>> code,
>>>  	    && printk_ratelimit()) {
>>>  		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
>>>  			tsk->comm, task_pid_nr(tsk), signo, code, addr);
>>> -		print_vma_addr(KERN_CONT " in ", GET_IP(regs));
>>> +		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
>>>  		pr_cont("\n");
>>>  		show_regs(regs);
>>>  	}
>> 
>> Reviewed-by: Nick Kossifidis <mick@ics.forth.gr>
>> 
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

I'm sorry I added the Reviewed-by tag too soon, I'm all in for the 
cleanup but I
believe it's much simpler, all of these are re-defined on
include/asm-generic/ptrace.h anyway, all we have to do is define GET_FP 
and GET_USP
(or rename regs->sp to regs->usp).

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 07:03:48PM +0300, Nick Kossifidis wrote:
> I'm sorry I added the Reviewed-by tag too soon, I'm all in for the cleanup 
> but I
> believe it's much simpler, all of these are re-defined on
> include/asm-generic/ptrace.h anyway, all we have to do is define GET_FP and 
> GET_USP
> (or rename regs->sp to regs->usp).

We don't use include/asm-generic/ptrace.h on RISC-V, and I don't really
see a good reason to start using it.  Quite to the contrary, I think we
can remove that file and apply a similar cleanup as in this patch to the
few architectures currently using it.

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Nick Kossifidis]

Στις 2019-04-11 19:17, Christoph Hellwig έγραψε:
> On Thu, Apr 11, 2019 at 07:03:48PM +0300, Nick Kossifidis wrote:
>> I'm sorry I added the Reviewed-by tag too soon, I'm all in for the 
>> cleanup
>> but I
>> believe it's much simpler, all of these are re-defined on
>> include/asm-generic/ptrace.h anyway, all we have to do is define 
>> GET_FP and
>> GET_USP
>> (or rename regs->sp to regs->usp).
> 
> We don't use include/asm-generic/ptrace.h on RISC-V, and I don't really
> see a good reason to start using it.  Quite to the contrary, I think we
> can remove that file and apply a similar cleanup as in this patch to 
> the
> few architectures currently using it.

I think the point is to avoid code-duplication and enforce a standard 
way
of accessing these registers (these static inline functions are also 
used
outside arch/, e.g. on syscall.c, signal.c uprobes etc). I agree that at
this point what we have is not good, IMHO we should just have define 
macros
for the register names on the arch-specific ptrace.h instead of GET/SET
macros + the static inline functions should have get_ an set_ prefixes
because e.g. frame_pointer can also be a variable.

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 06:55:09PM +0300, Nick Kossifidis wrote:
>>> @@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct task_struct 
>>> *task,
>>>
>>>  	if (regs) {
>>>  		sp = GET_USP(regs);
>
> Ah just noticed this, it should be user_stack_pointer(regs) instead since 
> you removed
> GET_USP.

It should.  And given that at least my full nommu tree had plenty of
buildbot exposure that made me wonder why it didn't blow up.

It turns out RISC-V unconditionally selects ARCH_WANT_FRAME_POINTERS,
which then turns on the FRAME_POINTER option unconditonally.

So this branch is entirely dead code and I should just remove it with
the next respin of the series.

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Paul Walmsley]

On Thu, 11 Apr 2019, Christoph Hellwig wrote:

> No need to have two names for the same thing.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This is probably worth cleaning up across the entire tree.  

asm-{generic,x86,arm64,powerpc}/ptrace.h all define similar GET_IP/SET_IP
macros - although it seems that arch/riscv is the only architecture that 
uses them.


- Paul


> ---
>  arch/riscv/include/asm/ptrace.h | 21 ++++++---------------
>  arch/riscv/kernel/stacktrace.c  |  8 ++++----
>  arch/riscv/kernel/traps.c       |  2 +-
>  3 files changed, 11 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
> index d35ec2f41381..9c867a4bac83 100644
> --- a/arch/riscv/include/asm/ptrace.h
> +++ b/arch/riscv/include/asm/ptrace.h
> @@ -70,47 +70,38 @@ struct pt_regs {
>  
>  
>  /* Helpers for working with the instruction pointer */
> -#define GET_IP(regs) ((regs)->sepc)
> -#define SET_IP(regs, val) (GET_IP(regs) = (val))
> -
>  static inline unsigned long instruction_pointer(struct pt_regs *regs)
>  {
> -	return GET_IP(regs);
> +	return regs->sepc;
>  }
>  static inline void instruction_pointer_set(struct pt_regs *regs,
>  					   unsigned long val)
>  {
> -	SET_IP(regs, val);
> +	regs->sepc = val;
>  }
>  
>  #define profile_pc(regs) instruction_pointer(regs)
>  
>  /* Helpers for working with the user stack pointer */
> -#define GET_USP(regs) ((regs)->sp)
> -#define SET_USP(regs, val) (GET_USP(regs) = (val))
> -
>  static inline unsigned long user_stack_pointer(struct pt_regs *regs)
>  {
> -	return GET_USP(regs);
> +	return regs->sp;
>  }
>  static inline void user_stack_pointer_set(struct pt_regs *regs,
>  					  unsigned long val)
>  {
> -	SET_USP(regs, val);
> +	regs->sp =  val;
>  }
>  
>  /* Helpers for working with the frame pointer */
> -#define GET_FP(regs) ((regs)->s0)
> -#define SET_FP(regs, val) (GET_FP(regs) = (val))
> -
>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>  {
> -	return GET_FP(regs);
> +	return regs->s0;
>  }
>  static inline void frame_pointer_set(struct pt_regs *regs,
>  				     unsigned long val)
>  {
> -	SET_FP(regs, val);
> +	regs->s0 = val;
>  }
>  
>  static inline unsigned long regs_return_value(struct pt_regs *regs)
> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
> index a4b1d94371a0..25fe0ff81f9e 100644
> --- a/arch/riscv/kernel/stacktrace.c
> +++ b/arch/riscv/kernel/stacktrace.c
> @@ -33,9 +33,9 @@ static void notrace walk_stackframe(struct task_struct *task,
>  	unsigned long fp, sp, pc;
>  
>  	if (regs) {
> -		fp = GET_FP(regs);
> -		sp = GET_USP(regs);
> -		pc = GET_IP(regs);
> +		fp = frame_pointer(regs);
> +		sp = user_stack_pointer(regs);
> +		pc = instruction_pointer(regs);
>  	} else if (task == NULL || task == current) {
>  		const register unsigned long current_sp __asm__ ("sp");
>  		fp = (unsigned long)__builtin_frame_address(0);
> @@ -83,7 +83,7 @@ static void notrace walk_stackframe(struct task_struct *task,
>  
>  	if (regs) {
>  		sp = GET_USP(regs);
> -		pc = GET_IP(regs);
> +		pc = instruction_pointer(regs);
>  	} else if (task == NULL || task == current) {
>  		const register unsigned long current_sp __asm__ ("sp");
>  		sp = current_sp;
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 24a9333dda2c..86731a2fa218 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -70,7 +70,7 @@ void do_trap(struct pt_regs *regs, int signo, int code,
>  	    && printk_ratelimit()) {
>  		pr_info("%s[%d]: unhandled signal %d code 0x%x at 0x" REG_FMT,
>  			tsk->comm, task_pid_nr(tsk), signo, code, addr);
> -		print_vma_addr(KERN_CONT " in ", GET_IP(regs));
> +		print_vma_addr(KERN_CONT " in ", instruction_pointer(regs));
>  		pr_cont("\n");
>  		show_regs(regs);
>  	}
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Christoph Hellwig]

On Thu, Apr 25, 2019 at 12:13:18PM -0700, Paul Walmsley wrote:
> 
> 
> On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> 
> > No need to have two names for the same thing.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> This is probably worth cleaning up across the entire tree.  
> 
> asm-{generic,x86,arm64,powerpc}/ptrace.h all define similar GET_IP/SET_IP
> macros - although it seems that arch/riscv is the only architecture that 
> uses them.

Yes, we had that discussion before.  I've started on a series for that,
but it is unlikely to touch riscv.

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

Re: [PATCH 7/9] riscv: remove duplicate macros from ptrace.h

[Paul Walmsley]

On Thu, 25 Apr 2019, Christoph Hellwig wrote:

> On Thu, Apr 25, 2019 at 12:13:18PM -0700, Paul Walmsley wrote:
> > 
> > 
> > On Thu, 11 Apr 2019, Christoph Hellwig wrote:
> > 
> > > No need to have two names for the same thing.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > This is probably worth cleaning up across the entire tree.  
> > 
> > asm-{generic,x86,arm64,powerpc}/ptrace.h all define similar GET_IP/SET_IP
> > macros - although it seems that arch/riscv is the only architecture that 
> > uses them.
> 
> Yes, we had that discussion before.  I've started on a series for that,
> but it is unlikely to touch riscv.

OK, if you're planning to post a separate series for that across the tree, 
then:

Reviewed-by: Paul Walmsley <paul.walmsley@sifive.com>

for this RISC-V patch.


- Paul

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

[PATCH 8/9] riscv: print the unexpected interrupt cause

[Christoph Hellwig]

This has been helpful when debugging my pending nommu port.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
index 48e6b7db83a1..b10af8ccd656 100644
--- a/arch/riscv/kernel/irq.c
+++ b/arch/riscv/kernel/irq.c
@@ -54,7 +54,8 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs)
 		handle_arch_irq(regs);
 		break;
 	default:
-		panic("unexpected interrupt cause");
+		pr_alert("unexpected interrupt cause 0x%lx", regs->xcause);
+		BUG();
 	}
 	irq_exit();
 
-- 
2.20.1


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

Re: [PATCH 8/9] riscv: print the unexpected interrupt cause

[Nick Kossifidis]

Στις 2019-04-11 14:56, Christoph Hellwig έγραψε:
> This has been helpful when debugging my pending nommu port.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/kernel/irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index 48e6b7db83a1..b10af8ccd656 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -54,7 +54,8 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs 
> *regs)
>  		handle_arch_irq(regs);
>  		break;
>  	default:
> -		panic("unexpected interrupt cause");
> +		pr_alert("unexpected interrupt cause 0x%lx", regs->xcause);
> +		BUG();
>  	}
>  	irq_exit();

Did you mean regs->scause ? There is no xcause field on struct pt_regs 
and
also there is no such register name:
https://github.com/torvalds/linux/blob/master/arch/riscv/include/asm/ptrace.h


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

Re: [PATCH 8/9] riscv: print the unexpected interrupt cause

[Christoph Hellwig]

On Thu, Apr 11, 2019 at 06:52:06PM +0300, Nick Kossifidis wrote:
>>  	default:
>> -		panic("unexpected interrupt cause");
>> +		pr_alert("unexpected interrupt cause 0x%lx", regs->xcause);
>> +		BUG();
>>  	}
>>  	irq_exit();
>
> Did you mean regs->scause ? There is no xcause field on struct pt_regs and
> also there is no such register name:
> https://github.com/torvalds/linux/blob/master/arch/riscv/include/asm/ptrace.h

Oops.  The field gets renamed in my nommu tree to deal with the fact
that depending on the kernel build it could hold scause or mcause.
I'll respin it without this rename.

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

Re: [PATCH 8/9] riscv: print the unexpected interrupt cause

[Paul Walmsley]

On Thu, 11 Apr 2019, Christoph Hellwig wrote:

> This has been helpful when debugging my pending nommu port.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Paul Walmsley <paul.walmsley@sifive.com>

> ---
>  arch/riscv/kernel/irq.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/irq.c b/arch/riscv/kernel/irq.c
> index 48e6b7db83a1..b10af8ccd656 100644
> --- a/arch/riscv/kernel/irq.c
> +++ b/arch/riscv/kernel/irq.c
> @@ -54,7 +54,8 @@ asmlinkage void __irq_entry do_IRQ(struct pt_regs *regs)
>  		handle_arch_irq(regs);
>  		break;
>  	default:
> -		panic("unexpected interrupt cause");
> +		pr_alert("unexpected interrupt cause 0x%lx", regs->xcause);
> +		BUG();
>  	}
>  	irq_exit();
>  
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

[PATCH 9/9] riscv: call pm_power_off from machine_halt / machine_power_off

[Christoph Hellwig]

This way any override of pm_power_off also affects the halt path and
we don't need additional infrastructure for it.

Also remove the pm_power_off export - at least for now we don't have
any modular drivers overriding it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/riscv/kernel/reset.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
index 2a53d26ffdd6..ed637aee514b 100644
--- a/arch/riscv/kernel/reset.c
+++ b/arch/riscv/kernel/reset.c
@@ -12,11 +12,15 @@
  */
 
 #include <linux/reboot.h>
-#include <linux/export.h>
 #include <asm/sbi.h>
 
-void (*pm_power_off)(void) = machine_power_off;
-EXPORT_SYMBOL(pm_power_off);
+static void default_power_off(void)
+{
+	sbi_shutdown();
+	while (1);
+}
+
+void (*pm_power_off)(void) = default_power_off;
 
 void machine_restart(char *cmd)
 {
@@ -26,11 +30,10 @@ void machine_restart(char *cmd)
 
 void machine_halt(void)
 {
-	machine_power_off();
+	pm_power_off();
 }
 
 void machine_power_off(void)
 {
-	sbi_shutdown();
-	while (1);
+	pm_power_off();
 }
-- 
2.20.1


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

Re: [PATCH 9/9] riscv: call pm_power_off from machine_halt / machine_power_off

[Atish Patra]

On 4/11/19 4:57 AM, Christoph Hellwig wrote:
> This way any override of pm_power_off also affects the halt path and
> we don't need additional infrastructure for it.
> 
> Also remove the pm_power_off export - at least for now we don't have
> any modular drivers overriding it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   arch/riscv/kernel/reset.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
> index 2a53d26ffdd6..ed637aee514b 100644
> --- a/arch/riscv/kernel/reset.c
> +++ b/arch/riscv/kernel/reset.c
> @@ -12,11 +12,15 @@
>    */
>   
>   #include <linux/reboot.h>
> -#include <linux/export.h>
>   #include <asm/sbi.h>
>   
> -void (*pm_power_off)(void) = machine_power_off;
> -EXPORT_SYMBOL(pm_power_off);
> +static void default_power_off(void)
> +{
> +	sbi_shutdown();
> +	while (1);
> +}
> +
> +void (*pm_power_off)(void) = default_power_off;
>   
>   void machine_restart(char *cmd)
>   {
> @@ -26,11 +30,10 @@ void machine_restart(char *cmd)
>   
>   void machine_halt(void)
>   {
> -	machine_power_off();
> +	pm_power_off();
>   }
>   
>   void machine_power_off(void)
>   {
> -	sbi_shutdown();
> -	while (1);
> +	pm_power_off();
>   }
> 

Reviewed-by: Atish Patra <atish.patra@wdc.com>

Regards,
Atish

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

Re: [PATCH 9/9] riscv: call pm_power_off from machine_halt / machine_power_off

[Paul Walmsley]

On Thu, 11 Apr 2019, Christoph Hellwig wrote:

> This way any override of pm_power_off also affects the halt path and
> we don't need additional infrastructure for it.
> 
> Also remove the pm_power_off export - at least for now we don't have
> any modular drivers overriding it.

I'd propose that we keep the pm_power_off export - both to align with 
other architectures:

$ fgrep -r pm_power_off arch/ | grep EXPORT
arch/s390/kernel/setup.c:EXPORT_SYMBOL_GPL(pm_power_off);
arch/m68k/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/hexagon/kernel/reset.c:EXPORT_SYMBOL(pm_power_off);
arch/ia64/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/sparc/kernel/process_32.c:EXPORT_SYMBOL(pm_power_off);
arch/sparc/kernel/reboot.c:EXPORT_SYMBOL(pm_power_off);
arch/sh/kernel/reboot.c:EXPORT_SYMBOL(pm_power_off);
arch/um/kernel/reboot.c:EXPORT_SYMBOL(pm_power_off);
arch/openrisc/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/nios2/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/arc/kernel/reset.c:EXPORT_SYMBOL(pm_power_off);
arch/arm/kernel/reboot.c:EXPORT_SYMBOL(pm_power_off);
arch/nds32/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/csky/kernel/power.c:EXPORT_SYMBOL(pm_power_off);
arch/alpha/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/arm64/kernel/process.c:EXPORT_SYMBOL_GPL(pm_power_off);
arch/unicore32/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/c6x/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/xtensa/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/h8300/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/powerpc/kernel/setup-common.c:EXPORT_SYMBOL_GPL(pm_power_off);
arch/mips/kernel/reset.c:EXPORT_SYMBOL(pm_power_off);
arch/riscv/kernel/reset.c:EXPORT_SYMBOL(pm_power_off);
arch/microblaze/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
arch/x86/kernel/reboot.c:EXPORT_SYMBOL(pm_power_off);
arch/parisc/kernel/process.c:EXPORT_SYMBOL(pm_power_off);
$

and to make sure there's no hassle with the drivers that expect to assign 
something to it:

$ fgrep -r pm_power_off drivers/ | fgrep =  | cut -f1 -d:  | sort -u | wc -l
39
$

For what it's worth, I agree with the implied criticism that reassigning 
pm_power_off is not a good interface.  There is the obvious problem that 
more than one chunk of independent code could all try to assign to 
pm_power_off.  However, to avoid creating a RISC-V-specific mess when 
someone uses a TI PMIC driver on a RISC-V board, or tries to use ACPI with 
RISC-V, fixing that seems best done as a separate tree-wide series.

...

Replacing machine_power_off() with a call to your default_power_off() 
looks fine to me.  However I think it makes sense to change our existing 
machine_halt() (which was not added by your patch).  Looking at other 
major architectures - x86, ARM64, and ARM - they don't actually try to 
power down the system in machine_halt(), instead just entering an 
infinite loop, WFI, or calling into firmware or a hypervisor.  I'd propose 
that we align with that approach.


- Paul

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  arch/riscv/kernel/reset.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/kernel/reset.c b/arch/riscv/kernel/reset.c
> index 2a53d26ffdd6..ed637aee514b 100644
> --- a/arch/riscv/kernel/reset.c
> +++ b/arch/riscv/kernel/reset.c
> @@ -12,11 +12,15 @@
>   */
>  
>  #include <linux/reboot.h>
> -#include <linux/export.h>
>  #include <asm/sbi.h>
>  
> -void (*pm_power_off)(void) = machine_power_off;
> -EXPORT_SYMBOL(pm_power_off);
> +static void default_power_off(void)
> +{
> +	sbi_shutdown();
> +	while (1);
> +}
> +
> +void (*pm_power_off)(void) = default_power_off;
>  
>  void machine_restart(char *cmd)
>  {
> @@ -26,11 +30,10 @@ void machine_restart(char *cmd)
>  
>  void machine_halt(void)
>  {
> -	machine_power_off();
> +	pm_power_off();
>  }
>  
>  void machine_power_off(void)
>  {
> -	sbi_shutdown();
> -	while (1);
> +	pm_power_off();
>  }
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

Re: [PATCH 9/9] riscv: call pm_power_off from machine_halt / machine_power_off

[Andreas Schwab]

On Apr 11 2019, Christoph Hellwig <hch@lst.de> wrote:

> Also remove the pm_power_off export - at least for now we don't have
> any modular drivers overriding it.

ERROR: "pm_power_off" [drivers/mfd/rk808.ko] undefined!
ERROR: "pm_power_off" [drivers/mfd/max8907.ko] undefined!
ERROR: "pm_power_off" [drivers/mfd/axp20x.ko] undefined!
ERROR: "pm_power_off" [drivers/char/ipmi/ipmi_poweroff.ko] undefined!

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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