Re: [PATCH 09/10] timer-riscv: Fix undefined riscv_time_val

From: Kefeng Wang
Date: Tue May 19 2020 - 22:57:49 EST


On 2020/5/20 9:14, Anup Patel wrote:
On Tue, May 19, 2020 at 7:21 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
On 19/05/2020 14:39, Kefeng Wang wrote:
On 2020/5/19 4:23, Daniel Lezcano wrote:
Hi Kefeng,

On 18/05/2020 17:40, Kefeng Wang wrote:
On 2020/5/18 22:09, Daniel Lezcano wrote:
On 13/05/2020 23:14, Palmer Dabbelt wrote:
On Sun, 10 May 2020 19:20:00 PDT (-0700), wangkefeng.wang@xxxxxxxxxx
wrote:
ERROR: modpost: "riscv_time_val" [crypto/tcrypt.ko] undefined!

Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>
Signed-off-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
---
drivers/clocksource/timer-riscv.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-riscv.c
b/drivers/clocksource/timer-riscv.c
index c4f15c4068c0..071b8c144027 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -19,6 +19,7 @@

u64 __iomem *riscv_time_cmp;
u64 __iomem *riscv_time_val;
+EXPORT_SYMBOL(riscv_time_val);

static inline void mmio_set_timer(u64 val)
{
Reviewed-by: Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>
Acked-by: Palmer Dabbelt <palmerdabbelt@xxxxxxxxxx>

Adding the clocksource maintainers. Let me know if you want this
through my
tree, I'm assuming you want it through your tree.
How can we end up by an export symbol here ?!
Hi Danile,
s/Danile/Daniel/
Sorry for typing error.
Found this build error when CONFIG_RISCV_M_MODE=y and CONFIG_RISCV_SBI
is not,

see patch "4f9bbcefa142 riscv: add support for MMIO access to the timer
registers"
Thanks for the pointer.

The question still remains, how do we end up with this EXPORT_SYMBOL?

There is something wrong if the fix is an EXPORT_SYMBOL for a global
variable.
Not very clear, there are some global variable( eg, acpi_disabled,
memstart_addr in arm64,) is exported by EXPORT_SYMBOL, do you mean that
export riscv_time_val is wrong way?
I do not maintain acpi neither arm64.mm.

AFAICT, riscv_time_val is globally declared in
drivers/clocksource/timer-riscv.c

The driver does not use this variable at all. Then there is a readl on
it in the header file arch/riscv/include/asm/timex.h

And finally it is initialized in arch/riscv/kernel/clint.c

Same thing for riscv_time_cmp.

The correct fix is to initialize the variables in the place where they
belong to (drivers/clocksource/timer-riscv.c), create a function to read
their content and export-symbol-gpl the function.

ok, it's better. thanks for your explanation.


I agree with Daniel. Exporting riscv_time_val is a temporary fix.

yes. it's only for build, let's wait for Anup's patch.


The problem is timer-riscv.c is pretty convoluted right now. It is
implementing two different clocksources and clockevents in one-place.

I think we need two separate drivers for RISC-V world.

1. timer-riscv: This for regular S-mode kernel with MMU. The clocksource
will use TIME CSR and the clockevent device will use SBI calls.

2. timer-clint: This for M-mode kernel without MMU (or NoMMU kernel).
The clocksource will use MMIO counter for clocksource and the
clockevent device will use MMIO compare registers.

I will send a patch to have a separate timer-clint driver under
drivers/clocksource. (@Daniel, I hope you will be fine with this?)
Regards,
Anup


--
<http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
.