* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
[not found] <CAAhSdy1gNB0sMAq4mtGSNJ96BND4tMxHShq==3B1hzL9ebs=oQ@mail.gmail.com>
@ 2018-12-08 20:25 ` Palmer Dabbelt
0 siblings, 0 replies; 4+ messages in thread
From: Palmer Dabbelt @ 2018-12-08 20:25 UTC (permalink / raw)
To: anup
Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy,
daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
tglx
On Fri, 07 Dec 2018 09:20:57 PST (-0800), anup@brainfault.org wrote:
> On Fri, 7 Dec, 2018, 10:30 PM Palmer Dabbelt <palmer@sifive.com wrote:
>
>> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com wrote:
>> > Currently, clocksource registration happens for an invalid cpu
>> > for non-smp kernels. This lead to kernel panic as cpu hotplug
>> > registration will fail for those cpus.
>> >
>> > Do not proceed if hartid is invalid. Take this opprtunity to
>> > print appropriate error strings for different failure cases.
>> >
>> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> > ---
>> > drivers/clocksource/riscv_timer.c | 13 ++++++++++---
>> > 1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/clocksource/riscv_timer.c
>> b/drivers/clocksource/riscv_timer.c
>> > index 39de6e49..4af4af47 100644
>> > --- a/drivers/clocksource/riscv_timer.c
>> > +++ b/drivers/clocksource/riscv_timer.c
>> > @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct
>> device_node *n)
>> > int cpuid, hartid, error;
>> >
>> > hartid = riscv_of_processor_hartid(n);
>> > + if (hartid < 0)
>> > + return hartid;
>>
>> This seems like it's just hiding a bug somewhere else. We should at least
>> put
>> out a WARN here, as I'm not sure the error will propagate anywhere useful.
>>
>
> We need separate DT node for riscv_timer. The riscv_timer is nothing but
> SOC timer accessed via rdtime and SBI calls. It can be viewed as one
> device. In fact, this is how it's done in ARM/ARM64.
We had that at some point, but this was changed. The logic was that, since the
RISC-V ISA mandates the presence of this timer for all harts, the RISC-V CPU
node is sufficient to encode the presence of a RISC-V timer.
I'm OK changing this, but you should look at the old thread (which I can't
find) to make sure all the arguments are taken into account.
>> > cpuid = riscv_hartid_to_cpuid(hartid);
>> >
>> > if (cpuid != smp_processor_id())
>> > @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct
>> device_node *n)
>> >
>> > /* This should be called only for boot cpu */
>> > riscv_timebase = riscv_timebase_frequency(n);
>> > - clocksource_register_hz(&riscv_clocksource, riscv_timebase);
>> > + error = clocksource_register_hz(&riscv_clocksource,
>> riscv_timebase);
>> >
>> > + if (error) {
>> > + pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>> > + error, cpuid);
>> > + return error;
>> > + }
>> > error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
>> > "clockevents/riscv/timer:starting",
>> > riscv_timer_starting_cpu, riscv_timer_dying_cpu);
>> > if (error)
>> > - pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>> > - error, cpuid);
>> > + pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
>> > + error);
>> > return error;
>> > }
>>
>
> Regards,
> Anup
>
>>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
2018-12-07 17:00 ` Palmer Dabbelt
@ 2018-12-07 23:31 ` Atish Patra
0 siblings, 0 replies; 4+ messages in thread
From: Atish Patra @ 2018-12-07 23:31 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: mark.rutland, devicetree, Damien Le Moal, aou, dmitriy, anup,
daniel.lezcano, linux-kernel, robh+dt, linux-riscv, tglx
On 12/7/18 9:00 AM, Palmer Dabbelt wrote:
> On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com wrote:
>> Currently, clocksource registration happens for an invalid cpu
>> for non-smp kernels. This lead to kernel panic as cpu hotplug
>> registration will fail for those cpus.
>>
>> Do not proceed if hartid is invalid. Take this opprtunity to
>> print appropriate error strings for different failure cases.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>> drivers/clocksource/riscv_timer.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
>> index 39de6e49..4af4af47 100644
>> --- a/drivers/clocksource/riscv_timer.c
>> +++ b/drivers/clocksource/riscv_timer.c
>> @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>> int cpuid, hartid, error;
>>
>> hartid = riscv_of_processor_hartid(n);
>> + if (hartid < 0)
>> + return hartid;
>
> This seems like it's just hiding a bug somewhere else. We should at least put
> out a WARN here, as I'm not sure the error will propagate anywhere useful.
>
ok. I will add a warning here. That's what we are doing in plic as well.
Regards,
Atish
>> cpuid = riscv_hartid_to_cpuid(hartid);
>>
>> if (cpuid != smp_processor_id())
>> @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>>
>> /* This should be called only for boot cpu */
>> riscv_timebase = riscv_timebase_frequency(n);
>> - clocksource_register_hz(&riscv_clocksource, riscv_timebase);
>> + error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
>>
>> + if (error) {
>> + pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>> + error, cpuid);
>> + return error;
>> + }
>> error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
>> "clockevents/riscv/timer:starting",
>> riscv_timer_starting_cpu, riscv_timer_dying_cpu);
>> if (error)
>> - pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
>> - error, cpuid);
>> + pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
>> + error);
>> return error;
>> }
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
2018-12-03 20:57 ` [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems Atish Patra
@ 2018-12-07 17:00 ` Palmer Dabbelt
2018-12-07 23:31 ` Atish Patra
0 siblings, 1 reply; 4+ messages in thread
From: Palmer Dabbelt @ 2018-12-07 17:00 UTC (permalink / raw)
To: atish.patra
Cc: mark.rutland, devicetree, Damien.LeMoal, aou, dmitriy, anup,
daniel.lezcano, linux-kernel, atish.patra, robh+dt, linux-riscv,
tglx
On Mon, 03 Dec 2018 12:57:31 PST (-0800), atish.patra@wdc.com wrote:
> Currently, clocksource registration happens for an invalid cpu
> for non-smp kernels. This lead to kernel panic as cpu hotplug
> registration will fail for those cpus.
>
> Do not proceed if hartid is invalid. Take this opprtunity to
> print appropriate error strings for different failure cases.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
> drivers/clocksource/riscv_timer.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
> index 39de6e49..4af4af47 100644
> --- a/drivers/clocksource/riscv_timer.c
> +++ b/drivers/clocksource/riscv_timer.c
> @@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
> int cpuid, hartid, error;
>
> hartid = riscv_of_processor_hartid(n);
> + if (hartid < 0)
> + return hartid;
This seems like it's just hiding a bug somewhere else. We should at least put
out a WARN here, as I'm not sure the error will propagate anywhere useful.
> cpuid = riscv_hartid_to_cpuid(hartid);
>
> if (cpuid != smp_processor_id())
> @@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
>
> /* This should be called only for boot cpu */
> riscv_timebase = riscv_timebase_frequency(n);
> - clocksource_register_hz(&riscv_clocksource, riscv_timebase);
> + error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
>
> + if (error) {
> + pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> + error, cpuid);
> + return error;
> + }
> error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
> "clockevents/riscv/timer:starting",
> riscv_timer_starting_cpu, riscv_timer_dying_cpu);
> if (error)
> - pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
> - error, cpuid);
> + pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
> + error);
> return error;
> }
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems
2018-12-03 20:57 [PATCH 0/4] Timer code cleanup Atish Patra
@ 2018-12-03 20:57 ` Atish Patra
2018-12-07 17:00 ` Palmer Dabbelt
0 siblings, 1 reply; 4+ messages in thread
From: Atish Patra @ 2018-12-03 20:57 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, devicetree, Damien Le Moal, Albert Ou,
Dmitriy Cherkasov, Anup Patel, Daniel Lezcano, Atish Patra,
Rob Herring, Palmer Dabbelt, linux-riscv, Thomas Gleixner
Currently, clocksource registration happens for an invalid cpu
for non-smp kernels. This lead to kernel panic as cpu hotplug
registration will fail for those cpus.
Do not proceed if hartid is invalid. Take this opprtunity to
print appropriate error strings for different failure cases.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
drivers/clocksource/riscv_timer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/riscv_timer.c b/drivers/clocksource/riscv_timer.c
index 39de6e49..4af4af47 100644
--- a/drivers/clocksource/riscv_timer.c
+++ b/drivers/clocksource/riscv_timer.c
@@ -108,6 +108,8 @@ static int __init riscv_timer_init_dt(struct device_node *n)
int cpuid, hartid, error;
hartid = riscv_of_processor_hartid(n);
+ if (hartid < 0)
+ return hartid;
cpuid = riscv_hartid_to_cpuid(hartid);
if (cpuid != smp_processor_id())
@@ -115,14 +117,19 @@ static int __init riscv_timer_init_dt(struct device_node *n)
/* This should be called only for boot cpu */
riscv_timebase = riscv_timebase_frequency(n);
- clocksource_register_hz(&riscv_clocksource, riscv_timebase);
+ error = clocksource_register_hz(&riscv_clocksource, riscv_timebase);
+ if (error) {
+ pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
+ error, cpuid);
+ return error;
+ }
error = cpuhp_setup_state(CPUHP_AP_RISCV_TIMER_STARTING,
"clockevents/riscv/timer:starting",
riscv_timer_starting_cpu, riscv_timer_dying_cpu);
if (error)
- pr_err("RISCV timer register failed [%d] for cpu = [%d]\n",
- error, cpuid);
+ pr_err("cpu hp setup state failed for RISCV timer [%d]\n",
+ error);
return error;
}
--
2.7.4
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-12-08 20:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAAhSdy1gNB0sMAq4mtGSNJ96BND4tMxHShq==3B1hzL9ebs=oQ@mail.gmail.com>
2018-12-08 20:25 ` [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems Palmer Dabbelt
2018-12-03 20:57 [PATCH 0/4] Timer code cleanup Atish Patra
2018-12-03 20:57 ` [PATCH 4/4] RISC-V: Fix non-smp kernel boot on SMP systems Atish Patra
2018-12-07 17:00 ` Palmer Dabbelt
2018-12-07 23:31 ` Atish Patra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).