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: <20220311113543.GA17877@axis.com>
Date:   Fri, 11 Mar 2022 12:35:43 +0100
From:   Vincent Whitchurch <vincent.whitchurch@...s.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
CC:     "tglx@...utronix.de" <tglx@...utronix.de>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        kernel <kernel@...s.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-samsung-soc@...r.kernel.org" 
        <linux-samsung-soc@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>
Subject: Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support
 local-timer-index property

On Tue, Mar 08, 2022 at 03:57:55PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2022 15:24, Vincent Whitchurch wrote:
> > 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().

Could we just add the skip-write-register-if-already-started change in
exynos4_mct_frc_start() uncondtionally?  Perhaps it could be in a
separate patch too?  I was probably being over-cautious when I did it
conditionally on mct_local_idx.  Doing it uncondtionally would make it
easier to remove the global variable.

On my system the FRC is actually started long before Linux, and I assume
it's similar on other chips.

> 
> >  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.

Yes, you're right.  The case of the FRC not being running when the A5
starts up is only ever hit in our simulation environment where we are
able to start Linux on the A5 directly, without having to go via the
A53.

> 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.

The A5 is normally started from Linux on the A53 (using the remoteproc
framework).  This is long after exynos4_mct_frc_start() has been called
on the A53.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ