[PATCH 0/8] RISC-V: Assorted Cleanups

[PATCH 0/8] RISC-V: Assorted Cleanups

[Palmer Dabbelt]

I finally got around to answering a very old email in my inbox that was
a response to our original patch set.  These are meant to cause little
to no functional changes to the port, but I've given them very little
testing so I wouldn't be surprised if I've managed to screw something
up here.

I'll be sure to give these some testing before putting them on for-next,
but I figured it'd be better to send them out now rather than later.

[PATCH 1/8] RISC-V: Provide a cleaner raw_smp_processor_id()

[Palmer Dabbelt]

I'm not sure how I managed to miss this the first time, but this is much
better.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/include/asm/smp.h | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
index 36016845461d..27fd085188df 100644
--- a/arch/riscv/include/asm/smp.h
+++ b/arch/riscv/include/asm/smp.h
@@ -14,11 +14,6 @@
 #ifndef _ASM_RISCV_SMP_H
 #define _ASM_RISCV_SMP_H
 
-/* This both needs asm-offsets.h and is used when generating it. */
-#ifndef GENERATING_ASM_OFFSETS
-#include <asm/asm-offsets.h>
-#endif
-
 #include <linux/cpumask.h>
 #include <linux/irqreturn.h>
 
@@ -33,13 +28,12 @@ void arch_send_call_function_ipi_mask(struct cpumask *mask);
 /* Hook for the generic smp_call_function_single() routine. */
 void arch_send_call_function_single_ipi(int cpu);
 
-/*
- * This is particularly ugly: it appears we can't actually get the definition
- * of task_struct here, but we need access to the CPU this task is running on.
- * Instead of using C we're using asm-offsets.h to get the current processor
- * ID.
- */
-#define raw_smp_processor_id() (*((int*)((char*)get_current() + TASK_TI_CPU)))
+/* Obtains the hart ID of the currently executing task.  This relies on
+ * THREAD_INFO_IN_TASK, but we define that unconditionally. */
+#ifdef CONFIG_THREAD_INFO_IN_TASK
+#define get_ti()                ((struct thread_info *)get_current())
+#define raw_smp_processor_id()  (get_ti()->cpu)
+#endif
 
 #endif /* CONFIG_SMP */
 
-- 
2.16.4

Re: [PATCH 1/8] RISC-V: Provide a cleaner raw_smp_processor_id()

[Christoph Hellwig]

On Mon, Aug 27, 2018 at 11:42:36AM -0700, Palmer Dabbelt wrote:
> +/* Obtains the hart ID of the currently executing task.  This relies on
> + * THREAD_INFO_IN_TASK, but we define that unconditionally. */

Kernel comment style would be:

/*
 * Obtains the hart ID of the currently executing task.  This relies on
 * THREAD_INFO_IN_TASK, but we define that unconditionally.
 */

> +#ifdef CONFIG_THREAD_INFO_IN_TASK

THREAD_INFO_IN_TASK is always defined for riscv, so no need for this
ifdef.

> +#define get_ti()                ((struct thread_info *)get_current())
> +#define raw_smp_processor_id()  (get_ti()->cpu)

Please don't cast structs around.  This should be something like

#define raw_smp_processor_id() \
	(container_of(get_current(), struct task_struct, thread_info)->cpu)

[PATCH 2/8] RISC-V: Don't set cacheinfo.{physical_line_partition,attributes}

[Palmer Dabbelt]

These are just hard coded in the RISC-V port, which doesn't make any
sense.  We should probably be setting these from device tree entries
when they exist, but for now I think it's saner to just leave them all
as their default values.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/cacheinfo.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
index 0bc86e5f8f3f..cb35ffd8ec6b 100644
--- a/arch/riscv/kernel/cacheinfo.c
+++ b/arch/riscv/kernel/cacheinfo.c
@@ -22,13 +22,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
 {
 	this_leaf->level = level;
 	this_leaf->type = type;
-	/* not a sector cache */
-	this_leaf->physical_line_partition = 1;
-	/* TODO: Add to DTS */
-	this_leaf->attributes =
-		CACHE_WRITE_BACK
-		| CACHE_READ_ALLOCATE
-		| CACHE_WRITE_ALLOCATE;
 }
 
 static int __init_cache_level(unsigned int cpu)
-- 
2.16.4

Re: [PATCH 2/8] RISC-V: Don't set cacheinfo.{physical_line_partition, attributes}

[Christoph Hellwig]

On Mon, Aug 27, 2018 at 11:42:37AM -0700, Palmer Dabbelt wrote:
> These are just hard coded in the RISC-V port, which doesn't make any
> sense.  We should probably be setting these from device tree entries
> when they exist, but for now I think it's saner to just leave them all
> as their default values.
> 
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>

Looks fine:

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

Re: [PATCH 2/8] RISC-V: Don't set cacheinfo.{physical_line_partition,attributes}

[Jeremy Linton]

Hi,

On 08/27/2018 01:42 PM, Palmer Dabbelt wrote:
> These are just hard coded in the RISC-V port, which doesn't make any
> sense.  We should probably be setting these from device tree entries
> when they exist, but for now I think it's saner to just leave them all
> as their default values.

Default value here means unset and not visible in /sys.

Which looks fine to me.

Reviewed-by: Jeremy Linton <jeremy.linton@arm.com>

Thanks,


> 
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>   arch/riscv/kernel/cacheinfo.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cacheinfo.c b/arch/riscv/kernel/cacheinfo.c
> index 0bc86e5f8f3f..cb35ffd8ec6b 100644
> --- a/arch/riscv/kernel/cacheinfo.c
> +++ b/arch/riscv/kernel/cacheinfo.c
> @@ -22,13 +22,6 @@ static void ci_leaf_init(struct cacheinfo *this_leaf,
>   {
>   	this_leaf->level = level;
>   	this_leaf->type = type;
> -	/* not a sector cache */
> -	this_leaf->physical_line_partition = 1;
> -	/* TODO: Add to DTS */
> -	this_leaf->attributes =
> -		CACHE_WRITE_BACK
> -		| CACHE_READ_ALLOCATE
> -		| CACHE_WRITE_ALLOCATE;
>   }
>   
>   static int __init_cache_level(unsigned int cpu)
> 

[PATCH 3/8] RISC-V: Rename riscv_of_processor_hart to riscv_of_processor_hartid

[Palmer Dabbelt]

It's a bit confusing exactly what this function does: it actually
returns the hartid of an OF processor node, failing with -1 on invalid
nodes.  I've changed the name to _hartid() in order to make that a bit
more clear, as well as adding a comment.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/include/asm/processor.h | 4 +++-
 arch/riscv/kernel/cpu.c            | 2 +-
 arch/riscv/kernel/smpboot.c        | 2 +-
 drivers/clocksource/riscv_timer.c  | 2 +-
 drivers/irqchip/irq-sifive-plic.c  | 2 +-
 5 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 3fe4af8147d2..9d32670d84a4 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -88,7 +88,9 @@ static inline void wait_for_interrupt(void)
 }
 
 struct device_node;
-extern int riscv_of_processor_hart(struct device_node *node);
+/* Returns the hart ID of the given device tree node, or -1 if the device tree
+ * node isn't a RISC-V hart. */
+extern int riscv_of_processor_hartid(struct device_node *node);
 
 extern void riscv_fill_hwcap(void);
 
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index ca6c81e54e37..19e98c1710dd 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -16,7 +16,7 @@
 #include <linux/of.h>
 
 /* Return -1 if not a valid hart */
-int riscv_of_processor_hart(struct device_node *node)
+int riscv_of_processor_hartid(struct device_node *node)
 {
 	const char *isa, *status;
 	u32 hart;
diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 56abab6a9812..5f29f8562cf6 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -53,7 +53,7 @@ void __init setup_smp(void)
 	int hart, im_okay_therefore_i_am = 0;
 
 	while ((dn = of_find_node_by_type(dn, "cpu"))) {
-		hart = riscv_of_processor_hart(dn);
+		hart = riscv_of_processor_hartid(dn);
 		if (hart >= 0) {
 			set_cpu_possible(hart, true);
 			set_cpu_present(hart, true);
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 4e8b347e43e2..ad7453fc3129 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -84,7 +84,7 @@ void riscv_timer_interrupt(void)
 
 static int __init riscv_timer_init_dt(struct device_node *n)
 {
-	int cpu_id = riscv_of_processor_hart(n), error;
+	int cpu_id = riscv_of_processor_hartid(n), error;
 	struct clocksource *cs;
 
 	if (cpu_id != smp_processor_id())
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 532e9d68c704..c55eaa31cde2 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -176,7 +176,7 @@ static int plic_find_hart_id(struct device_node *node)
 {
 	for (; node; node = node->parent) {
 		if (of_device_is_compatible(node, "riscv"))
-			return riscv_of_processor_hart(node);
+			return riscv_of_processor_hartid(node);
 	}
 
 	return -1;
-- 
2.16.4

Re: [PATCH 3/8] RISC-V: Rename riscv_of_processor_hart to riscv_of_processor_hartid

[Atish Patra]

On 8/27/18 11:58 AM, Palmer Dabbelt wrote:
> It's a bit confusing exactly what this function does: it actually
> returns the hartid of an OF processor node, failing with -1 on invalid
> nodes.  I've changed the name to _hartid() in order to make that a bit
> more clear, as well as adding a comment.
> 
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---
>   arch/riscv/include/asm/processor.h | 4 +++-
>   arch/riscv/kernel/cpu.c            | 2 +-
>   arch/riscv/kernel/smpboot.c        | 2 +-
>   drivers/clocksource/riscv_timer.c  | 2 +-
>   drivers/irqchip/irq-sifive-plic.c  | 2 +-
>   5 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
> index 3fe4af8147d2..9d32670d84a4 100644
> --- a/arch/riscv/include/asm/processor.h
> +++ b/arch/riscv/include/asm/processor.h
> @@ -88,7 +88,9 @@ static inline void wait_for_interrupt(void)
>   }
>   
>   struct device_node;
> -extern int riscv_of_processor_hart(struct device_node *node);
> +/* Returns the hart ID of the given device tree node, or -1 if the device tree
> + * node isn't a RISC-V hart. */

I think multi-line commenting format should be:
/*
  *
  */

Otherwise, Reviewed-by: Atish Patra <atish.patra@wdc.com>
> +extern int riscv_of_processor_hartid(struct device_node *node);
>   
>   extern void riscv_fill_hwcap(void);
>   
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index ca6c81e54e37..19e98c1710dd 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -16,7 +16,7 @@
>   #include <linux/of.h>
>   
>   /* Return -1 if not a valid hart */
> -int riscv_of_processor_hart(struct device_node *node)
> +int riscv_of_processor_hartid(struct device_node *node)
>   {
>   	const char *isa, *status;
>   	u32 hart;
> diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
> index 56abab6a9812..5f29f8562cf6 100644
> --- a/arch/riscv/kernel/smpboot.c
> +++ b/arch/riscv/kernel/smpboot.c
> @@ -53,7 +53,7 @@ void __init setup_smp(void)
>   	int hart, im_okay_therefore_i_am = 0;
>   
>   	while ((dn = of_find_node_by_type(dn, "cpu"))) {
> -		hart = riscv_of_processor_hart(dn);
> +		hart = riscv_of_processor_hartid(dn);
>   		if (hart >= 0) {
>   			set_cpu_possible(hart, true);
>   			set_cpu_present(hart, true);
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> index 4e8b347e43e2..ad7453fc3129 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -84,7 +84,7 @@ void riscv_timer_interrupt(void)
>   
>   static int __init riscv_timer_init_dt(struct device_node *n)
>   {
> -	int cpu_id = riscv_of_processor_hart(n), error;
> +	int cpu_id = riscv_of_processor_hartid(n), error;
>   	struct clocksource *cs;
>   
>   	if (cpu_id != smp_processor_id())
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 532e9d68c704..c55eaa31cde2 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -176,7 +176,7 @@ static int plic_find_hart_id(struct device_node *node)
>   {
>   	for (; node; node = node->parent) {
>   		if (of_device_is_compatible(node, "riscv"))
> -			return riscv_of_processor_hart(node);
> +			return riscv_of_processor_hartid(node);
>   	}
>   
>   	return -1;
> 

Re: [PATCH 3/8] RISC-V: Rename riscv_of_processor_hart to riscv_of_processor_hartid

[Christoph Hellwig]

> +/* Returns the hart ID of the given device tree node, or -1 if the device tree
> + * node isn't a RISC-V hart. */

In addition to the comment formatting: we usually keep the description
near the implementation, not the header.  It could also become a
kerneldoc comment for added clarity.

> +extern int riscv_of_processor_hartid(struct device_node *node);

No need to declare function prototypes in headers with the extern
attribute, btw.

[PATCH 4/8] RISC-V: Filter ISA and MMU values in cpuinfo

[Palmer Dabbelt]

We shouldn't be directly passing device tree values to userspace, both
because there could be mistakes in device trees and because the kernel
doesn't support arbitrary ISAs.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/cpu.c | 62 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index 19e98c1710dd..a18b4e3962a1 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -58,6 +58,57 @@ int riscv_of_processor_hartid(struct device_node *node)
 
 #ifdef CONFIG_PROC_FS
 
+static void print_isa(struct seq_file *f, const char *orig_isa)
+{
+    static const char *ext = "mafdc";
+    const char *isa = orig_isa;
+    const char *e;
+
+    /* Linux doesn't support rv32e or rv128i, and we only support booting
+     * kernels on harts with the same ISA that the kernel is compiled for. */
+#if defined(CONFIG_32BIT)
+    if (strncmp(isa, "rv32i", 5) != 0)
+        return;
+#elif defined(CONFIG_64BIT)
+    if (strncmp(isa, "rv64i", 5) != 0)
+        return;
+#endif
+
+    /* Print the base ISA, as we already know it's legal. */
+    seq_printf(f, "isa\t: ");
+    seq_write(f, isa, 5);
+    isa += 5;
+
+    /* Check the rest of the ISA string for valid extensions, printing those we
+     * find.  RISC-V ISA strings define an order, so we only print the
+     * extension bits when they're in order. */
+    for (e = ext; *e != '\0'; ++e) {
+        if (isa[0] == e[0]) {
+            seq_write(f, isa, 1);
+            isa++;
+        }
+    }
+
+    /* If we were given an unsupported ISA in the device tree then print a bit
+     * of info describing what went wrong. */
+    if (isa[0] != '\0')
+        pr_info("unsupported ISA \"%s\" in device tree", orig_isa);
+}
+
+static void print_mmu(struct seq_file *f, const char *mmu_type)
+{
+#if defined(CONFIG_32BIT)
+    if (strcmp(mmu_type, "riscv,sv32") != 0)
+        return;
+#elif defined(CONFIG_64BIT)
+    if ((strcmp(mmu_type, "riscv,sv39") != 0)
+        && (strcmp(mmu_type, "riscv,sv48") != 0))
+        return;
+#endif
+
+    seq_printf(f, "mmu\t: %s\n", mmu_type+6);
+}
+
 static void *c_start(struct seq_file *m, loff_t *pos)
 {
 	*pos = cpumask_next(*pos - 1, cpu_online_mask);
@@ -83,13 +134,10 @@ static int c_show(struct seq_file *m, void *v)
 	const char *compat, *isa, *mmu;
 
 	seq_printf(m, "hart\t: %lu\n", hart_id);
-	if (!of_property_read_string(node, "riscv,isa", &isa)
-	    && isa[0] == 'r'
-	    && isa[1] == 'v')
-		seq_printf(m, "isa\t: %s\n", isa);
-	if (!of_property_read_string(node, "mmu-type", &mmu)
-	    && !strncmp(mmu, "riscv,", 6))
-		seq_printf(m, "mmu\t: %s\n", mmu+6);
+	if (!of_property_read_string(node, "riscv,isa", &isa))
+        print_isa(m, isa);
+	if (!of_property_read_string(node, "mmu-type", &mmu))
+        print_mmu(m, mmu);
 	if (!of_property_read_string(node, "compatible", &compat)
 	    && strcmp(compat, "riscv"))
 		seq_printf(m, "uarch\t: %s\n", compat);
-- 
2.16.4

[PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Palmer Dabbelt]

The old name was a bit odd.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/smpboot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 5f29f8562cf6..e1f6a5ad0416 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -50,7 +50,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 void __init setup_smp(void)
 {
 	struct device_node *dn = NULL;
-	int hart, im_okay_therefore_i_am = 0;
+	int hart, found_boot_cpu = 0;
 
 	while ((dn = of_find_node_by_type(dn, "cpu"))) {
 		hart = riscv_of_processor_hartid(dn);
@@ -58,13 +58,13 @@ void __init setup_smp(void)
 			set_cpu_possible(hart, true);
 			set_cpu_present(hart, true);
 			if (hart == smp_processor_id()) {
-				BUG_ON(im_okay_therefore_i_am);
-				im_okay_therefore_i_am = 1;
+				BUG_ON(found_boot_cpu);
+				found_boot_cpu = 1;
 			}
 		}
 	}
 
-	BUG_ON(!im_okay_therefore_i_am);
+	BUG_ON(!found_boot_cpu);
 }
 
 int __cpu_up(unsigned int cpu, struct task_struct *tidle)
-- 
2.16.4

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Christoph Hellwig]

>  	struct device_node *dn = NULL;
> -	int hart, im_okay_therefore_i_am = 0;
> +	int hart, found_boot_cpu = 0;

If you rename this anyway please switch to use a bool.

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Atish Patra]

On 8/30/18 7:41 AM, Christoph Hellwig wrote:
>>   	struct device_node *dn = NULL;
>> -	int hart, im_okay_therefore_i_am = 0;
>> +	int hart, found_boot_cpu = 0;
> 
> If you rename this anyway please switch to use a bool.
> 

I can address the comment on this patch and fold it in my series to 
avoid unnecessary conflict.

@Palmer: You can drop this patch.

Regards,
Atish

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Christoph Hellwig]

On Thu, Aug 30, 2018 at 09:11:11AM -0700, Atish Patra wrote:
> On 8/30/18 7:41 AM, Christoph Hellwig wrote:
> > >   	struct device_node *dn = NULL;
> > > -	int hart, im_okay_therefore_i_am = 0;
> > > +	int hart, found_boot_cpu = 0;
> > 
> > If you rename this anyway please switch to use a bool.
> > 
> 
> I can address the comment on this patch and fold it in my series to avoid
> unnecessary conflict.
> 
> @Palmer: You can drop this patch.

If Palmer is ok with it how about you include a respin of his whole
series?  There are some overlaps I think and having a single person
in charge might be helpful.

Btw, I've found your series in my linux-kernel folder, but it did
not make it to linux-riscv or my personal inbox despite the CC.

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Atish Patra]

On 8/30/18 10:54 PM, Christoph Hellwig wrote:
> On Thu, Aug 30, 2018 at 09:11:11AM -0700, Atish Patra wrote:
>> On 8/30/18 7:41 AM, Christoph Hellwig wrote:
>>>>    	struct device_node *dn = NULL;
>>>> -	int hart, im_okay_therefore_i_am = 0;
>>>> +	int hart, found_boot_cpu = 0;
>>>
>>> If you rename this anyway please switch to use a bool.
>>>
>>
>> I can address the comment on this patch and fold it in my series to avoid
>> unnecessary conflict.
>>
>> @Palmer: You can drop this patch.
> 
> If Palmer is ok with it how about you include a respin of his whole
> series?  There are some overlaps I think and having a single person
> in charge might be helpful.
> 

Sure. I am fine with it. Palmer ?

> Btw, I've found your series in my linux-kernel folder, but it did
> not make it to linux-riscv or my personal inbox despite the CC.
> 

Thanks for the review.
I checked my scripts. I couldn't figure out what went wrong.
Not sure if there was some issue with infradead.org at that time.

Anyways, sorry again for the mess. I will keep a watch on this next time.

Regards,
Atish

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Palmer Dabbelt]

On Fri, 31 Aug 2018 14:18:02 PDT (-0700), atish.patra@wdc.com wrote:
> On 8/30/18 10:54 PM, Christoph Hellwig wrote:
>> On Thu, Aug 30, 2018 at 09:11:11AM -0700, Atish Patra wrote:
>>> On 8/30/18 7:41 AM, Christoph Hellwig wrote:
>>>>>    	struct device_node *dn = NULL;
>>>>> -	int hart, im_okay_therefore_i_am = 0;
>>>>> +	int hart, found_boot_cpu = 0;
>>>>
>>>> If you rename this anyway please switch to use a bool.
>>>>
>>>
>>> I can address the comment on this patch and fold it in my series to avoid
>>> unnecessary conflict.
>>>
>>> @Palmer: You can drop this patch.
>>
>> If Palmer is ok with it how about you include a respin of his whole
>> series?  There are some overlaps I think and having a single person
>> in charge might be helpful.
>>
>
> Sure. I am fine with it. Palmer ?

Ya, I have no problem with it.

>
>> Btw, I've found your series in my linux-kernel folder, but it did
>> not make it to linux-riscv or my personal inbox despite the CC.
>>
>
> Thanks for the review.
> I checked my scripts. I couldn't figure out what went wrong.
> Not sure if there was some issue with infradead.org at that time.
>
> Anyways, sorry again for the mess. I will keep a watch on this next time.
>
> Regards,
> Atish

Re: [PATCH 5/8] RISC-V: Rename im_okay_therefore_i_am to found_boot_cpu

[Palmer Dabbelt]

On Thu, 30 Aug 2018 09:11:11 PDT (-0700), atish.patra@wdc.com wrote:
> On 8/30/18 7:41 AM, Christoph Hellwig wrote:
>>>   	struct device_node *dn = NULL;
>>> -	int hart, im_okay_therefore_i_am = 0;
>>> +	int hart, found_boot_cpu = 0;
>>
>> If you rename this anyway please switch to use a bool.
>>
>
> I can address the comment on this patch and fold it in my series to
> avoid unnecessary conflict.
>
> @Palmer: You can drop this patch.

Thanks!

[PATCH 6/8] RISC-V: Use mmgrab()

[Palmer Dabbelt]

f1f1007644ff ("mm: add new mmgrab() helper") added a helper that we
missed out on.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/smpboot.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index e1f6a5ad0416..5437a04babcd 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -30,6 +30,7 @@
 #include <linux/irq.h>
 #include <linux/of.h>
 #include <linux/sched/task_stack.h>
+#include <linux/sched/mm.h>
 #include <asm/irq.h>
 #include <asm/mmu_context.h>
 #include <asm/tlbflush.h>
@@ -79,8 +80,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
 	 * the spinning harts that they can continue the boot process.
 	 */
 	smp_mb();
-	__cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
-	__cpu_up_task_pointer[cpu] = tidle;
+	WRITE_ONCE(__cpu_up_stack_pointer[cpu], task_stack_page(tidle) + THREAD_SIZE);
+	WRITE_ONCE(__cpu_up_task_pointer[cpu], tidle);
 
 	while (!cpu_online(cpu))
 		cpu_relax();
@@ -100,7 +101,7 @@ asmlinkage void __init smp_callin(void)
 	struct mm_struct *mm = &init_mm;
 
 	/* All kernel threads share the same mm context.  */
-	atomic_inc(&mm->mm_count);
+	mmgrab(mm);
 	current->active_mm = mm;
 
 	trap_init();
-- 
2.16.4

Re: [PATCH 6/8] RISC-V: Use mmgrab()

[Christoph Hellwig]

> +#include <linux/sched/mm.h>
>  #include <asm/irq.h>
>  #include <asm/mmu_context.h>
>  #include <asm/tlbflush.h>
> @@ -79,8 +80,8 @@ int __cpu_up(unsigned int cpu, struct task_struct *tidle)
>  	 * the spinning harts that they can continue the boot process.
>  	 */
>  	smp_mb();
> -	__cpu_up_stack_pointer[cpu] = task_stack_page(tidle) + THREAD_SIZE;
> -	__cpu_up_task_pointer[cpu] = tidle;
> +	WRITE_ONCE(__cpu_up_stack_pointer[cpu], task_stack_page(tidle) + THREAD_SIZE);
> +	WRITE_ONCE(__cpu_up_task_pointer[cpu], tidle);

This looks unrelated.

> @@ -100,7 +101,7 @@ asmlinkage void __init smp_callin(void)
>  	struct mm_struct *mm = &init_mm;
>  
>  	/* All kernel threads share the same mm context.  */
> -	atomic_inc(&mm->mm_count);
> +	mmgrab(mm);
>  	current->active_mm = mm;

This part looks fine to me:

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

[PATCH 7/8] RISC-V: Comment on the TLB flush in smp_callin()

[Palmer Dabbelt]

This isn't readily apparent from reading the code.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/smpboot.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 5437a04babcd..953bc540207d 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -107,6 +107,8 @@ asmlinkage void __init smp_callin(void)
 	trap_init();
 	notify_cpu_starting(smp_processor_id());
 	set_cpu_online(smp_processor_id(), 1);
+	/* Remote TLB flushes are ignored while the CPU is offline, so emit a local
+	 * TLB flush right now just in case. */
 	local_flush_tlb_all();
 	local_irq_enable();
 	preempt_disable();
-- 
2.16.4

Re: [PATCH 7/8] RISC-V: Comment on the TLB flush in smp_callin()

[Christoph Hellwig]

>  	notify_cpu_starting(smp_processor_id());
>  	set_cpu_online(smp_processor_id(), 1);
> +	/* Remote TLB flushes are ignored while the CPU is offline, so emit a local
> +	 * TLB flush right now just in case. */

Please use the normal kernel comment style and break the line after 80
characters.

[PATCH 8/8] RISC-V: Disable preemption before enabling interrupts when booting secondary harts

[Palmer Dabbelt]

I'm not sure, but I think this was a bug: if the scheduler fired right
here then I believe it would blow up.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 arch/riscv/kernel/smpboot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/smpboot.c b/arch/riscv/kernel/smpboot.c
index 953bc540207d..45515cc70181 100644
--- a/arch/riscv/kernel/smpboot.c
+++ b/arch/riscv/kernel/smpboot.c
@@ -110,7 +110,9 @@ asmlinkage void __init smp_callin(void)
 	/* Remote TLB flushes are ignored while the CPU is offline, so emit a local
 	 * TLB flush right now just in case. */
 	local_flush_tlb_all();
-	local_irq_enable();
+	/* Disable preemption before enabling interrupts, so we don't try to
+	 * schedule a CPU that hasn't actually started yet. */
 	preempt_disable();
+	local_irq_enable();
 	cpu_startup_entry(CPUHP_AP_ONLINE_IDLE);
 }
-- 
2.16.4