linux-kernel - Re: [PATCH v2 3/4] clocksource/drivers/exynos

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 8 Mar 2022 15:57:55 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To:     Vincent Whitchurch <vincent.whitchurch@...s.com>,
        tglx@...utronix.de, daniel.lezcano@...aro.org
Cc:     kernel@...s.com, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
        alim.akhtar@...sung.com, robh+dt@...nel.org
Subject: Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support
 local-timer-index property

On 08/03/2022 15:24, Vincent Whitchurch wrote:
> Support the documented semantics of the local-timer-index property: Use
> it as the first index of the local timer, ensure that global timer clock
> events device is not registered, and don't write to the global FRC if it
> is already started.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@...s.com>
> ---
> 
> Notes:
>     v2: Use devicetree property instead of module parameter.
> 
>  drivers/clocksource/exynos_mct.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index f29c812b70c9..5f8b516614eb 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -33,7 +33,7 @@
>  #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
>  #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
>  #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
> -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
> +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
>  #define EXYNOS4_MCT_L_MASK		(0xffffff00)
>  
>  #define MCT_L_TCNTB_OFFSET		(0x00)
> @@ -75,6 +75,7 @@ enum {
>  static void __iomem *reg_base;
>  static unsigned long clk_rate;
>  static unsigned int mct_int_type;
> +static unsigned int mct_local_idx;

No more static variables. This was wrong design, happens, but let's not
grow the list.

I propose to conditionally (depending on property samsung,frc-shared)
assign .resume callback to NULL or exynos4_frc_resume. The init can
receive an argument whether to call frc_start().


>  static int mct_irqs[MCT_NR_IRQS];
>  
>  struct mct_clock_event_device {
> @@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
>  	u32 reg;
>  
>  	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
> +
> +	/*
> +	 * If the FRC is already running, we don't need to start it again.  We
> +	 * could probably just do this on all systems, but, to avoid any risk
> +	 * for regressions, we only do it on systems where it's absolutely
> +	 * necessary (i.e., on systems where writes to the global registers
> +	 * need to be avoided).
> +	 */
> +	if (mct_local_idx && (reg & MCT_G_TCON_START))

This contradicts your intentions in commit #2 msg, where you described
that A53 will be started first.

1. If A53 is always started first, is it possible to be here from A5?
2. If above is possible, how do you handle locking? For example:
a. A53 started with some delay, entered exynos4_mct_frc_start() pass
this check;
b. A5 gets to exynos4_mct_frc_start(), check is still false, so A5
enables the FRC,
c. A53 also enables the FRC.

Having here relaxed reads and writes makes it even worse, unfortunately.

Best regards,
Krzysztof