linux-kernel - Re: [PATCH 3/7] x86/dt: Support the ACPI multiprocessor wakeup for device tree

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]

Message-ID: <87frrg2s6i.ffs@tglx>
Date: Wed, 07 Aug 2024 18:50:29 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Yunhong Jiang <yunhong.jiang@...ux.intel.com>, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 kys@...rosoft.com, haiyangz@...rosoft.com, wei.liu@...nel.org,
 decui@...rosoft.com, rafael@...nel.org, lenb@...nel.org,
 kirill.shutemov@...ux.intel.com
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-hyperv@...r.kernel.org, linux-acpi@...r.kernel.org,
 yunhong.jiang@...ux.intel.com
Subject: Re: [PATCH 3/7] x86/dt: Support the ACPI multiprocessor wakeup for
 device tree

On Tue, Aug 06 2024 at 15:12, Yunhong Jiang wrote:
>  
> -static int __init acpi_mp_setup_reset(u64 reset_vector)
> +static int __init __maybe_unused acpi_mp_setup_reset(u64 reset_vector)
>  {
>  	struct x86_mapping_info info = {
>  		.alloc_pgt_page = alloc_pgt_page,
> @@ -226,7 +228,7 @@ static int acpi_wakeup_cpu(u32 apicid, unsigned long start_ip)
>  	return 0;
>  }
>  
> -static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake)
> +static void __maybe_unused acpi_mp_disable_offlining(struct
> acpi_madt_multiproc_wakeup *mp_wake)

Please move those functions into the #ifdef CONFIG_ACPI

>  {
>  	cpu_hotplug_disable_offlining();
>  
> @@ -248,6 +250,7 @@ static void acpi_mp_disable_offlining(struct acpi_madt_multiproc_wakeup *mp_wake
>  	mp_wake->mailbox_address = 0;
>  }
>  
> +#ifdef CONFIG_ACPI
>  int __init acpi_parse_mp_wake(union acpi_subtable_headers *header,
> +
> +#ifdef CONFIG_OF
> +int __init dtb_parse_mp_wake(u64 *wake_mailbox_paddr)

Why not returning the mailbox physical address and 0 on failure instead
of that pointer and a integer return value which is ignored at the call
site?

> +{
> +	struct device_node *node;
> +	u64 mailaddr;
> +	int ret = 0;
> +
> +	node = of_find_node_by_path("/cpus");
> +	if (!node)
> +		return -ENODEV;
> +
> +	if (of_property_match_string(node, "enable-method",
> +				     "acpi-wakeup-mailbox") < 0) {

Please use the 100 characters line width and spare the line break.

> +		pr_err("No acpi wakeup mailbox enable-method\n");
> +		ret = -ENODEV;
> +		goto done;
> +	}
> +
> +	/*
> +	 * No support to the MADT reset vector yet.

s/to/for/

Also a single line comment is sufficient for this.

> +	 */
> +	cpu_hotplug_disable_offlining();
> +
> +	if (of_property_read_u64(node, "wakeup-mailbox-addr", &mailaddr)) {
> +		pr_err("Invalid wakeup mailbox addr\n");
> +		ret = -EINVAL;
> +		goto done;
> +	}
> +	acpi_mp_wake_mailbox_paddr = mailaddr;
> +	if (wake_mailbox_paddr)
> +		*wake_mailbox_paddr = mailaddr;
> +	pr_info("dt wakeup-mailbox: addr 0x%llx\n", mailaddr);
> +	apic_update_callback(wakeup_secondary_cpu_64, acpi_wakeup_cpu);
> +done:

newline before the label please. Up there you waste 3 lines for a
trivial comment and here you make the code unreadable...

Thanks,

        tglx