Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains

From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-pm@vger.kernel.org,
	"Rafael J . Wysocki" <rjw@rjwysocki.net>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Lina Iyer <ilina@codeaurora.org>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Stephen Boyd <sboyd@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Kevin Hilman <khilman@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, Lina Iyer <lina.iyer@linaro.org>
Subject: Re: [PATCH v3 10/13] cpuidle: psci: Prepare to use OS initiated suspend mode via PM domains
Date: Thu, 5 Dec 2019 18:35:44 +0000	[thread overview]
Message-ID: <20191205183544.GB1516@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <20191127102914.18729-11-ulf.hansson@linaro.org>

On Wed, Nov 27, 2019 at 11:29:11AM +0100, Ulf Hansson wrote:

[...]

> -static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
> +static int __init psci_dt_cpu_init_idle(struct cpuidle_driver *drv,
> +					struct device_node *cpu_node,
>  					unsigned int state_count, int cpu)
>  {
>  	int i, ret = 0;
> @@ -118,6 +152,11 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
>  		goto free_mem;
>  	}
>  
> +	/* Manage the deepest state via a dedicated enter-function. */
> +	if (dev)
> +		drv->states[state_count - 1].enter =
> +			psci_enter_domain_idle_state;

It is unfortunate to make this arbitrary choice, it would be best
if you could detect which states are "domain" states aka are governed
by multiple cpus.

This inizialization though does not belong in here, it is done at driver
level, it should not be done in this per-cpu path. IIUC the logic the
enter pointer should only be overridden if and only if all cpus managed
by the driver have a corresponding device associated.

To be frank I would even move the psci_has_osi_support() check from
psci_dt_attach_cpu() to this path and prevent calling
psci_dt_attach_cpu() and the tail of the function if
(!psci_has_osi_support()).

>  	data->dev = dev;

I think Sudeep already mentioned that, by using psci_has_osi_support()
as above you can prevent running this code, there is really no point,
the data->dev NULL sentinel is already initialized.

Lorenzo

>  	/* Idle states parsed correctly, store them in the per-cpu struct. */
> @@ -129,7 +168,8 @@ static int __init psci_dt_cpu_init_idle(struct device_node *cpu_node,
>  	return ret;
>  }
>  
> -static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_count)
> +static __init int psci_cpu_init_idle(struct cpuidle_driver *drv,
> +				     unsigned int cpu, unsigned int state_count)
>  {
>  	struct device_node *cpu_node;
>  	int ret;
> @@ -145,7 +185,7 @@ static __init int psci_cpu_init_idle(unsigned int cpu, unsigned int state_count)
>  	if (!cpu_node)
>  		return -ENODEV;
>  
> -	ret = psci_dt_cpu_init_idle(cpu_node, state_count, cpu);
> +	ret = psci_dt_cpu_init_idle(drv, cpu_node, state_count, cpu);
>  
>  	of_node_put(cpu_node);
>  
> @@ -201,7 +241,7 @@ static int __init psci_idle_init_cpu(int cpu)
>  	/*
>  	 * Initialize PSCI idle states.
>  	 */
> -	ret = psci_cpu_init_idle(cpu, ret);
> +	ret = psci_cpu_init_idle(drv, cpu, ret);
>  	if (ret) {
>  		pr_err("CPU %d failed to PSCI idle\n", cpu);
>  		goto out_kfree_drv;
> -- 
> 2.17.1
> 