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

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2020/5/20 9:14, Anup Patel wrote:
> On Tue, May 19, 2020 at 7:21 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> 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@huawei.com
>>>>>>> wrote:
>>>>>>>> ERROR: modpost: "riscv_time_val" [crypto/tcrypt.ko] undefined!
>>>>>>>>
>>>>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>>>>>> ---
>>>>>>>>     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@google.com>
>>>>>>> Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>
>>>>>>>
>>>>>>> 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
> .
>