linux-kernel - Re: [PATCH 09/10] timer-riscv: Fix undefined riscv_time

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAhSdy2wsnkYMLpT1_2OAjr334t6dAy9LCPNefC=h+aPuRqFrg@mail.gmail.com>
Date:   Wed, 20 May 2020 06:44:44 +0530
From:   Anup Patel <anup@...infault.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     Kefeng Wang <wangkefeng.wang@...wei.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org List" <linux-kernel@...r.kernel.org>,
        hulkci@...wei.com
Subject: Re: [PATCH 09/10] timer-riscv: Fix undefined riscv_time_val

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

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

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