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

[Thomas Gleixner <tglx@linutronix.de>]

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