Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <c60806d5480b7311ced8db07ff239aa5af7a050d.1573702497.git.viresh.kumar@linaro.org>
In-Reply-To: <c60806d5480b7311ced8db07ff239aa5af7a050d.1573702497.git.viresh.kumar@linaro.org>
From:   Amit Kucheria <amit.kucheria@linaro.org>
Date:   Thu, 14 Nov 2019 12:33:18 +0530
Message-ID: <CAP245DXgrnkVWp2ycObP+b5MV7CT8JzQdgxe8CeWktFT-eE6GQ@mail.gmail.com>
Subject: Re: [PATCH] cpufreq: Register cpufreq drivers only after CPU devices
 are registered
To:     Viresh Kumar <viresh.kumar@linaro.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Cc:     Rafael Wysocki <rjw@rjwysocki.net>,
        Linux PM list <linux-pm@vger.kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 14, 2019 at 9:06 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> The cpufreq core heavily depends on the availability of the struct
> device for CPUs and if they aren't available at the time cpufreq driver
> is registered, we will never succeed in making cpufreq work.
>
> This happens due to following sequence of events:
>
> - cpufreq_register_driver()
>   - subsys_interface_register()
>   - return 0; //successful registration of driver
>
> ... at a later point of time
>
> - register_cpu();
>   - device_register();
>     - bus_probe_device();
>       - sif->add_dev();
>         - cpufreq_add_dev();
>           - get_cpu_device(); //FAILS
>   - per_cpu(cpu_sys_devices, num) = &cpu->dev; //used by get_cpu_device()
>   - return 0; //CPU registered successfully
>
> Because the per-cpu variable cpu_sys_devices is set only after the CPU
> device is regsitered, cpufreq will never be able to get it when
> cpufreq_add_dev() is called.
>
> This patch avoids this failure by making sure device structure of at
> least CPU0 is available when the cpufreq driver is registered, else
> return -EPROBE_DEFER.
>
> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Tested-by: Amit Kucheria <amit.kucheria@linaro.org>

> ---
> @Amit: I have added your sob without asking as you were involved in
> getting us to this patch, you did a lot of testing yesterday to find the
> root cause.

SoB for the SoB :-)

> @Rafael: This fixes the issues reported by Bjorn on Amit's series and so
> should land before Amit's series, if at all this is acceptable to you.
> Thanks.

My series was going to get merged through the thermal tree. Currently
it is hosted here[1] for linux-next testing. We could sign-off this
patch to the thermal tree or bring the series into the PM tree. Up to
Rafael and you.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git/log/?h=thermal/linux-next

>  drivers/cpufreq/cpufreq.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 681c1b5f0a1a..05293b43e56d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2641,6 +2641,13 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
>         if (cpufreq_disabled())
>                 return -ENODEV;
>
> +       /*
> +        * The cpufreq core depends heavily on the availability of device
> +        * structure, make sure they are available before proceeding further.
> +        */
> +       if (!get_cpu_device(0))
> +               return -EPROBE_DEFER;
> +
>         if (!driver_data || !driver_data->verify || !driver_data->init ||
>             !(driver_data->setpolicy || driver_data->target_index ||
>                     driver_data->target) ||
> --
> 2.21.0.rc0.269.g1a574e7a288b
>