linux-kernel - Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

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]

Date:   Thu, 17 Nov 2016 18:03:05 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     linux-arm-kernel@...ts.infradead.org
Cc:     "pankaj.dubey" <pankaj.dubey@...sung.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>, Heiko Stuebner <heiko@...ech.de>,
        geert+renesas@...der.be, Linus Walleij <linus.walleij@...aro.org>,
        Liviu Dudau <liviu.dudau@....com>,
        Patrice Chotard <patrice.chotard@...com>, krzk@...nel.org,
        Jisheng Zhang <jszhang@...vell.com>, vireshk@...nel.org,
        magnus.damm@...il.com, Michal Simek <michal.simek@...inx.com>,
        Wei Xu <xuwei5@...ilicon.com>, thomas.ab@...sung.com,
        "cpgs ." <cpgs@...sung.com>,
        Stephen Warren <swarren@...dotorg.org>,
        Ray Jui <rjui@...adcom.com>, horms@...ge.net.au,
        Dinh Nguyen <dinguyen@...nsource.altera.com>,
        Shawn Guo <shawnguo@...nel.org>, linux-kernel@...r.kernel.org,
        Jun Nie <jun.nie@...aro.org>, shiraz.linux.kernel@...il.com
Subject: Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
> 
> >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> >>> it if it finds it, otherwise returning failure.
> >>>
> >>> a9_scu_enable() which tries to use the A9 provided SCU address and
> >>> enables it if it finds it, otherwise returning failure.
> >>>
> 
> OK, In that case I can see need for following four helpers as:
> 
> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
> enables it if it finds, otherwise return -ENOMEM failure.
> This helper APIs is required and sufficient for most of platforms such
> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
> mvebu
> 
> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
> enables it, if address mapped successfully, otherwise returning failure.
> This helper APIs is required and sufficient for two ARM platforms as of
> now tegra and hisi.
> 
> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
> found maps address and returns ioremapped address to caller.
> This helper APIs is required for three ARM plaforms rockchip, mvebu and
> ux500, along with scu_enable() API to enable and find number_of_cores.
> 
> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
> do ioremap of scu address and returns ioremapped address to the caller
> along with ownership (caller has responsibility to unmap it).
> This helper APIs is required to simplify SCU enable and related code in
> two ARM plaforms BCM ans ZX.
>
> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
> are useful for the time-being, as they need SCU mapping very early of
> boot, where we can't use iomap APIs. So I will drop patches related to
> these platforms in v2 version.
> 
> Please let me know if any concern in this approach.

I think ideally we wouldn't even need to know the virtual address
outside of smp_scu.c. If we can move all users of the address
into that file directly, it could become a local variable and
we change scu_power_mode() and scu_get_core_count() instead to
not require the address argument.

The only user I could find outside of that file is

static int shmobile_smp_scu_psr_core_disabled(int cpu)
{
        unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);

        if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
                return 1;

        return 0;
}

which can be done in the same file as well.

> >>> Then callers can decide which of these to call, and what error messages
> >>> to print on their failures.
> >>
> >> Splitting the function in two is probably simpler overall, but
> >> we may still have to look at all the callers: Any platform that
> >> currently tries to map it on any CPU and doesn't warn about the
> >> absence of the device node (or about scu_a9_has_base() == false)
> >> should really continue not to warn about that.
> > 
> > Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> > should produce any warnings or errors to be printed.  It's up to the
> > caller to report the failure, otherwise doing this doesn't make sense:
> > 
> >       if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> >               pr_err("Failed to map and enable the SCU\n");
> > 
> > because if of_scu_enable() prints a warning/error, then it's patently
> > misleading.
> > 

That's why I said "otherwise we can leave the warning in the caller
after checking the return code of the new APIs." for the case where
we actually need it.

> I will move out error message out of these helpers and let caller
> (platform specific code) handle and print error if required.

Ok.

	Arnd