On Mon, Jan 18, 2021 at 03:38:23PM +0000, Ionela Voinescu wrote:
> Hi,
>
> On Monday 18 Jan 2021 at 21:06:03 (+0800), Shawn Guo wrote:
> > Commit f17b3e44320b ("cpufreq: qcom-hw: Use
> > devm_platform_ioremap_resource() to simplify code") introduces
> > a regression on platforms using the driver, by failing to initialise
> > a policy, when one is created post hotplug.
> >
> > When all the CPUs of a policy are hoptplugged out, the call to .exit()
> > and later to devm_iounmap() does not release the memory region that was
> > requested during devm_platform_ioremap_resource(). Therefore,
> > a subsequent call to .init() will result in the following error, which
> > will prevent a new policy to be initialised:
> >
> > [ 3395.915416] CPU4: shutdown
> > [ 3395.938185] psci: CPU4 killed (polled 0 ms)
> > [ 3399.071424] CPU5: shutdown
> > [ 3399.094316] psci: CPU5 killed (polled 0 ms)
> > [ 3402.139358] CPU6: shutdown
> > [ 3402.161705] psci: CPU6 killed (polled 0 ms)
> > [ 3404.742939] CPU7: shutdown
> > [ 3404.765592] psci: CPU7 killed (polled 0 ms)
> > [ 3411.492274] Detected VIPT I-cache on CPU4
> > [ 3411.492337] GICv3: CPU4: found redistributor 400 region 0:0x0000000017ae0000
> > [ 3411.492448] CPU4: Booted secondary processor 0x0000000400 [0x516f802d]
> > [ 3411.503654] qcom-cpufreq-hw 17d43000.cpufreq: can't request region for resource [mem 0x17d45800-0x17d46bff]
> >
> > With that being said, the original code was tricky and skipping memory
> > region request intentionally to hide this issue. The true cause is that
> > those devm_xxx() device managed functions shouldn't be used for cpufreq
> > init/exit hooks, because &pdev->dev is alive across the hooks and will
> > not trigger auto resource free-up. Let's drop the use of device managed
> > functions and manually allocate/free resources, so that the issue can be
> > fixed properly.
> >
> > Fixes: f17b3e44320b ("cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code")
> > Suggested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > Signed-off-by: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > ---
> >
> > I took some of the commit log from Ionela.
> >
> > drivers/cpufreq/qcom-cpufreq-hw.c | 43 ++++++++++++++++++++++++-------
> > 1 file changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index 9ed5341dc515..b529b49649e0 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -32,6 +32,7 @@ struct qcom_cpufreq_soc_data {
> >
> > struct qcom_cpufreq_data {
> > void __iomem *base;
> > + struct resource *res;
> > const struct qcom_cpufreq_soc_data *soc_data;
> > };
> >
> > @@ -280,6 +281,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > struct of_phandle_args args;
> > struct device_node *cpu_np;
> > struct device *cpu_dev;
> > + struct resource *res;
> > void __iomem *base;
> > struct qcom_cpufreq_data *data;
> > int ret, index;
> > @@ -303,18 +305,33 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >
> > index = args.args[0];
> >
> > - base = devm_platform_ioremap_resource(pdev, index);
> > - if (IS_ERR(base))
> > - return PTR_ERR(base);
> > + data = kzalloc(sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> >
>
> Nit: you could move this allocation after all resource reservation and
> mapping below, possibly to avoid doing it unless the base address and
> the memory resource is actually valid. Or you can keep it here and
> remove the use of the local variables, especially the "base" variable.
It's a reasonable suggestion. I will send a new version to kill `base`
variable, but still want to keep `res` as it saves some two level
indirection.
Shawn
