linux-kernel - Re: [RFC 3/3] arm: shmobile: Add the RZ/N1D SMP enabler driver.

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:   Mon, 16 Apr 2018 14:46:16 -0700
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Michel Pollet <michel.pollet@...renesas.com>,
        linux-renesas-soc@...r.kernel.org,
        Simon Horman <horms@...ge.net.au>
Cc:     phil.edworthy@...esas.com, buserror+upstream@...il.com,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Magnus Damm <magnus.damm@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Chen-Yu Tsai <wens@...e.org>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Andreas Färber <afaerber@...e.de>,
        Andy Gross <andy.gross@...aro.org>,
        Carlo Caione <carlo@...lessm.com>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Frank Rowand <frank.rowand@...y.com>,
        Juri Lelli <juri.lelli@....com>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC 3/3] arm: shmobile: Add the RZ/N1D SMP enabler driver.

Hi Michel,

On 04/16/2018 02:34 AM, Michel Pollet wrote:
> The Renesas RZ/N1D second CA7 is parked in a ROM pen at boot time, it
> requires a special enable method to get it started at boot time.
> 
> Signed-off-by: Michel Pollet <michel.pollet@...renesas.com>

Some few comments below. This patch should probably be re-ordered in
your patch series, I would expect you to have this become patch 2 and
have patch 2 be patch 3 (first you add infrastructure for using
something, then you make use of it).

> +static int rzn1_smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> +	if (!cpu_bootaddr)
> +		return -ENODEV;
> +
> +	spin_lock(&cpu_lock);
> +
> +	writel(virt_to_phys(secondary_startup), cpu_bootaddr);

Consider using __pa_symbol() instead of virt_to_phys() since
secondary_startup is part of the kernel's linear memory map.

> +	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> +
> +	spin_unlock(&cpu_lock);
> +
> +	return 0;
> +}
> +
> +static void __init rzn1_smp_prepare_cpus(unsigned int max_cpus)
> +{
> +	struct device_node *dn;
> +	int ret;
> +	u32 bootaddr;
> +
> +	dn = of_get_cpu_node(1, NULL);
> +	if (!dn) {
> +		pr_err("CPU#1: missing device tree node\n");
> +		return;
> +	}
> +	/*
> +	 * Determine the address from which the CPU is polling.
> +	 */
> +	ret = of_property_read_u32(dn, "cpu-release-addr", &bootaddr);
> +	if (ret)
> +		pr_err("CPU#1: invalid cpu-release-addr property\n");
> +
> +	of_node_put(dn);
> +	/* The bootloader *does* change this property */

This comment should probably be moved above the function that fetches
"cpu-release-addr"

> +	pr_info("CPU#1: cpu-release-addr %08x\n", (u32)bootaddr);
> +
> +	if (!bootaddr)
> +		return;

Would not you want to show a message here to help catch such conditions

> +
> +	cpu_bootaddr = ioremap(bootaddr, sizeof(bootaddr));

Relying on ioremap() to reject values that might be outside of the
allowed range may be a little fragile, but I can't suggest any better
alternative.

> +	if (!cpu_bootaddr)
> +		pr_err("CPU#1: cpu-release-addr map failed\n");
> +}
> +
> +static const struct smp_operations rzn1_smp_ops __initconst = {
> +	.smp_prepare_cpus = rzn1_smp_prepare_cpus,
> +	.smp_boot_secondary = rzn1_smp_boot_secondary,
> +};
> +CPU_METHOD_OF_DECLARE(rzn1_smp, "renesas,r9a06g032-smp", &rzn1_smp_ops);
> 


-- 
Florian