| Message ID | 20211129080248.46240-1-wangxiongfeng2@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | cpufreq: Fix get_cpu_device() failed in add_cpu_dev_symlink() | expand |
On 29-11-21, 16:02, Xiongfeng Wang wrote: > When I hot added a CPU, I found 'cpufreq' directory is not created below > /sys/devices/system/cpu/cpuX/. It is because get_cpu_device() failed in > add_cpu_dev_symlink(). > > cpufreq_add_dev() is the .add_dev callback of a CPU subsys interface. It > will be called when the CPU device registered into the system. The stack > is as follows. > register_cpu() > ->device_register() > ->device_add() > ->bus_probe_device() > ->cpufreq_add_dev() > > But only after the CPU device has been registered, we can get the CPU > device by get_cpu_device(), otherwise it will return NULL. Since we > already have the CPU device in cpufreq_add_dev(), pass it to > add_cpu_dev_symlink(). I noticed that the 'kobj' of the cpu device has > been added into the system before cpufreq_add_dev(). > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > --- > drivers/cpufreq/cpufreq.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index e338d2f010fe..22aa2793e4d2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1004,10 +1004,9 @@ static struct kobj_type ktype_cpufreq = { > .release = cpufreq_sysfs_release, > }; > > -static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu) > +static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu, > + struct device *dev) > { > - struct device *dev = get_cpu_device(cpu); > - > if (unlikely(!dev)) > return; > > @@ -1391,7 +1390,7 @@ static int cpufreq_online(unsigned int cpu) > if (new_policy) { > for_each_cpu(j, policy->related_cpus) { > per_cpu(cpufreq_cpu_data, j) = policy; > - add_cpu_dev_symlink(policy, j); > + add_cpu_dev_symlink(policy, j, get_cpu_device(j)); > } > > policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), > @@ -1565,7 +1564,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > /* Create sysfs link on CPU registration */ > policy = per_cpu(cpufreq_cpu_data, cpu); > if (policy) > - add_cpu_dev_symlink(policy, cpu); > + add_cpu_dev_symlink(policy, cpu, dev); > > return 0; > } Interesting that I never hit it earlier despite doing rigorous testing of hotplug stuff :( Anyway the patch is okay, Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Mon, Nov 29, 2021 at 10:10 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 29-11-21, 16:02, Xiongfeng Wang wrote: > > When I hot added a CPU, I found 'cpufreq' directory is not created below > > /sys/devices/system/cpu/cpuX/. It is because get_cpu_device() failed in > > add_cpu_dev_symlink(). > > > > cpufreq_add_dev() is the .add_dev callback of a CPU subsys interface. It > > will be called when the CPU device registered into the system. The stack > > is as follows. > > register_cpu() > > ->device_register() > > ->device_add() > > ->bus_probe_device() > > ->cpufreq_add_dev() > > > > But only after the CPU device has been registered, we can get the CPU > > device by get_cpu_device(), otherwise it will return NULL. Since we > > already have the CPU device in cpufreq_add_dev(), pass it to > > add_cpu_dev_symlink(). I noticed that the 'kobj' of the cpu device has > > been added into the system before cpufreq_add_dev(). > > > > Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> > > --- > > drivers/cpufreq/cpufreq.c | 9 ++++----- > > 1 file changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index e338d2f010fe..22aa2793e4d2 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -1004,10 +1004,9 @@ static struct kobj_type ktype_cpufreq = { > > .release = cpufreq_sysfs_release, > > }; > > > > -static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu) > > +static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu, > > + struct device *dev) > > { > > - struct device *dev = get_cpu_device(cpu); > > - > > if (unlikely(!dev)) > > return; > > > > @@ -1391,7 +1390,7 @@ static int cpufreq_online(unsigned int cpu) > > if (new_policy) { > > for_each_cpu(j, policy->related_cpus) { > > per_cpu(cpufreq_cpu_data, j) = policy; > > - add_cpu_dev_symlink(policy, j); > > + add_cpu_dev_symlink(policy, j, get_cpu_device(j)); > > } > > > > policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), > > @@ -1565,7 +1564,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) > > /* Create sysfs link on CPU registration */ > > policy = per_cpu(cpufreq_cpu_data, cpu); > > if (policy) > > - add_cpu_dev_symlink(policy, cpu); > > + add_cpu_dev_symlink(policy, cpu, dev); > > > > return 0; > > } > > Interesting that I never hit it earlier despite doing rigorous testing of > hotplug stuff :( This is the real hot-add path which isn't tested on a regular basis. > Anyway the patch is okay, It would be good to add a Fixes: tag to it, though. Any idea about the commit this should point to? > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Hi, On 2021/11/30 19:42, Rafael J. Wysocki wrote: > On Mon, Nov 29, 2021 at 10:10 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: >> >> On 29-11-21, 16:02, Xiongfeng Wang wrote: >>> When I hot added a CPU, I found 'cpufreq' directory is not created below >>> /sys/devices/system/cpu/cpuX/. It is because get_cpu_device() failed in >>> add_cpu_dev_symlink(). >>> >>> cpufreq_add_dev() is the .add_dev callback of a CPU subsys interface. It >>> will be called when the CPU device registered into the system. The stack >>> is as follows. >>> register_cpu() >>> ->device_register() >>> ->device_add() >>> ->bus_probe_device() >>> ->cpufreq_add_dev() >>> >>> But only after the CPU device has been registered, we can get the CPU >>> device by get_cpu_device(), otherwise it will return NULL. Since we >>> already have the CPU device in cpufreq_add_dev(), pass it to >>> add_cpu_dev_symlink(). I noticed that the 'kobj' of the cpu device has >>> been added into the system before cpufreq_add_dev(). >>> >>> Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> >>> --- >>> drivers/cpufreq/cpufreq.c | 9 ++++----- >>> 1 file changed, 4 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> index e338d2f010fe..22aa2793e4d2 100644 >>> --- a/drivers/cpufreq/cpufreq.c >>> +++ b/drivers/cpufreq/cpufreq.c >>> @@ -1004,10 +1004,9 @@ static struct kobj_type ktype_cpufreq = { >>> .release = cpufreq_sysfs_release, >>> }; >>> >>> -static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu) >>> +static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu, >>> + struct device *dev) >>> { >>> - struct device *dev = get_cpu_device(cpu); >>> - >>> if (unlikely(!dev)) >>> return; >>> >>> @@ -1391,7 +1390,7 @@ static int cpufreq_online(unsigned int cpu) >>> if (new_policy) { >>> for_each_cpu(j, policy->related_cpus) { >>> per_cpu(cpufreq_cpu_data, j) = policy; >>> - add_cpu_dev_symlink(policy, j); >>> + add_cpu_dev_symlink(policy, j, get_cpu_device(j)); >>> } >>> >>> policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), >>> @@ -1565,7 +1564,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) >>> /* Create sysfs link on CPU registration */ >>> policy = per_cpu(cpufreq_cpu_data, cpu); >>> if (policy) >>> - add_cpu_dev_symlink(policy, cpu); >>> + add_cpu_dev_symlink(policy, cpu, dev); >>> >>> return 0; >>> } >> >> Interesting that I never hit it earlier despite doing rigorous testing of >> hotplug stuff :( > > This is the real hot-add path which isn't tested on a regular basis. > >> Anyway the patch is okay, > > It would be good to add a Fixes: tag to it, though. Any idea about > the commit this should point to? When I look up the commit history, I found this one. 2f0ba790df51 ("cpufreq: Fix creation of symbolic links to policy directories") Before this commit, the 'dev' is passed to add_cpu_dev_symlink() in cpufreq_add_dev(). Maybe we can point to this one. > >> Acked-by: Viresh Kumar <viresh.kumar@linaro.org> > . >
On 30-11-21, 12:42, Rafael J. Wysocki wrote: > This is the real hot-add path which isn't tested on a regular basis. Ahh, I thought this is a simple offline/online thing. Makes sense now. > > Anyway the patch is okay, > > It would be good to add a Fixes: tag to it, though. Any idea about > the commit this should point to? This is broken since a very long time then, we need to get this into all stable kernels we care about. As Xiongfeng pointed out, 2f0ba790df51 ("cpufreq: Fix creation of symbolic links to policy directories") looks to be a good candidate.
On Wed, Dec 1, 2021 at 8:23 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 30-11-21, 12:42, Rafael J. Wysocki wrote: > > This is the real hot-add path which isn't tested on a regular basis. > > Ahh, I thought this is a simple offline/online thing. Makes sense now. > > > > Anyway the patch is okay, > > > > It would be good to add a Fixes: tag to it, though. Any idea about > > the commit this should point to? > > This is broken since a very long time then, we need to get this into all stable > kernels we care about. > > As Xiongfeng pointed out, 2f0ba790df51 ("cpufreq: Fix creation of symbolic links > to policy directories") looks to be a good candidate. Applied as 5.16-rc material, thanks!
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index e338d2f010fe..22aa2793e4d2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1004,10 +1004,9 @@ static struct kobj_type ktype_cpufreq = { .release = cpufreq_sysfs_release, }; -static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu) +static void add_cpu_dev_symlink(struct cpufreq_policy *policy, unsigned int cpu, + struct device *dev) { - struct device *dev = get_cpu_device(cpu); - if (unlikely(!dev)) return; @@ -1391,7 +1390,7 @@ static int cpufreq_online(unsigned int cpu) if (new_policy) { for_each_cpu(j, policy->related_cpus) { per_cpu(cpufreq_cpu_data, j) = policy; - add_cpu_dev_symlink(policy, j); + add_cpu_dev_symlink(policy, j, get_cpu_device(j)); } policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req), @@ -1565,7 +1564,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) /* Create sysfs link on CPU registration */ policy = per_cpu(cpufreq_cpu_data, cpu); if (policy) - add_cpu_dev_symlink(policy, cpu); + add_cpu_dev_symlink(policy, cpu, dev); return 0; }
When I hot added a CPU, I found 'cpufreq' directory is not created below /sys/devices/system/cpu/cpuX/. It is because get_cpu_device() failed in add_cpu_dev_symlink(). cpufreq_add_dev() is the .add_dev callback of a CPU subsys interface. It will be called when the CPU device registered into the system. The stack is as follows. register_cpu() ->device_register() ->device_add() ->bus_probe_device() ->cpufreq_add_dev() But only after the CPU device has been registered, we can get the CPU device by get_cpu_device(), otherwise it will return NULL. Since we already have the CPU device in cpufreq_add_dev(), pass it to add_cpu_dev_symlink(). I noticed that the 'kobj' of the cpu device has been added into the system before cpufreq_add_dev(). Signed-off-by: Xiongfeng Wang <wangxiongfeng2@huawei.com> --- drivers/cpufreq/cpufreq.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)