| Message ID | 000001cf9771$e2f465c0$a8dd3140$@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jonghwan, On Fri, Jul 4, 2014 at 3:52 PM, Jonghwan Choi <jhbird.choi@samsung.com> wrote: > Cpumasks should be cleared before using. Please explain why and what is issue observed without this. > > Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com> > ---
On Fri, Jul 4, 2014 at 8:23 PM, Sachin Kamat <spk.linux@gmail.com> wrote: > > Cpumasks should be cleared before using. > > Please explain why and what is issue observed without this. > -> When I checked the mask value, I knew that unwanted bit is set. Test code without cpumask_clear. + cpumask_set_cpu(0, &mask_val); + cpulist_scnprintf(buf, 64, &mask_val); + printk("--ID [ %d] = %s \n", id, buf); + th_zone->cool_dev[id] = cpufreq_cooling_register(&mask_val); Console message-> 4.861157] [c6] --ID [ 1] = 0,4-5,7 (4,5,7 cpu bit was set.) And when I tried to register two cooling devices with cpumask_set_cpu(0, &mask_val) and cpumask_set_cpu(4, &mask_val). I found that cpu 0 bit is also set in latter cpumask. (I hope latter cpumask has a cpu 4 bit.) So I think that cpumask_clear should be inserted. Thanks.
Hi Jonghwan, On 05.07.2014 01:37, Jonghwan Choi wrote: > On Fri, Jul 4, 2014 at 8:23 PM, Sachin Kamat <spk.linux@gmail.com> wrote: > >>> Cpumasks should be cleared before using. >> >> Please explain why and what is issue observed without this. >> > > -> When I checked the mask value, I knew that unwanted bit is set. > > Test code without cpumask_clear. > > + cpumask_set_cpu(0, &mask_val); > + cpulist_scnprintf(buf, 64, &mask_val); > + printk("--ID [ %d] = %s \n", id, buf); > + th_zone->cool_dev[id] = cpufreq_cooling_register(&mask_val); > > > Console message-> 4.861157] [c6] --ID [ 1] = 0,4-5,7 (4,5,7 cpu bit was set.) > > And when I tried to register two cooling devices with cpumask_set_cpu(0, &mask_val) and cpumask_set_cpu(4, &mask_val). > > I found that cpu 0 bit is also set in latter cpumask. (I hope latter cpumask has a cpu 4 bit.) > > So I think that cpumask_clear should be inserted. I believe Sachin's concern was related to your patch description. A good description should say what the patch changes and what is the rationale behind this change. Also for fixes it is a good practice to specify observed issues in patch description as well. Best regards, Tomasz
On Fri, Jul 5, 2014 at 9:34 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > Console message-> 4.861157] [c6] --ID [ 1] = 0,4-5,7 (4,5,7 cpu bit > > was set.) > > > > And when I tried to register two cooling devices with cpumask_set_cpu(0, &mask_val) and > cpumask_set_cpu(4, &mask_val). > > > > I found that cpu 0 bit is also set in latter cpumask. (I hope latter > > cpumask has a cpu 4 bit.) > > > > So I think that cpumask_clear should be inserted. > > I believe Sachin's concern was related to your patch description. A good description should say what > the patch changes and what is the rationale behind this change. Also for fixes it is a good practice > to specify observed issues in patch description as well. > > Best regards, -> You are right. Thanks for your kind advice. Best regards.
On Sat, Jul 5, 2014 at 6:04 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Hi Jonghwan, > > On 05.07.2014 01:37, Jonghwan Choi wrote: >> On Fri, Jul 4, 2014 at 8:23 PM, Sachin Kamat <spk.linux@gmail.com> wrote: >> >>>> Cpumasks should be cleared before using. >>> >>> Please explain why and what is issue observed without this. >>> >> >> -> When I checked the mask value, I knew that unwanted bit is set. >> >> Test code without cpumask_clear. >> >> + cpumask_set_cpu(0, &mask_val); >> + cpulist_scnprintf(buf, 64, &mask_val); >> + printk("--ID [ %d] = %s \n", id, buf); >> + th_zone->cool_dev[id] = cpufreq_cooling_register(&mask_val); >> >> >> Console message-> 4.861157] [c6] --ID [ 1] = 0,4-5,7 (4,5,7 cpu bit was set.) >> >> And when I tried to register two cooling devices with cpumask_set_cpu(0, &mask_val) and cpumask_set_cpu(4, &mask_val). >> >> I found that cpu 0 bit is also set in latter cpumask. (I hope latter cpumask has a cpu 4 bit.) >> >> So I think that cpumask_clear should be inserted. > > I believe Sachin's concern was related to your patch description. A good > description should say what the patch changes and what is the rationale > behind this change. Also for fixes it is a good practice to specify > observed issues in patch description as well. Yes, that is correct. Sorry if it wasn't clear in my previous mail.
On Fri, Jul 04, 2014 at 11:22:40AM +0100, Jonghwan Choi wrote: > Cpumasks should be cleared before using. > > Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com> > --- > drivers/thermal/db8500_cpufreq_cooling.c | 1 + > drivers/thermal/imx_thermal.c | 1 + > drivers/thermal/samsung/exynos_thermal_common.c | 1 + > 3 files changed, 3 insertions(+) Reviewed-by: Javi Merino <javi.merino@arm.com> You should send this patch to linux-pm, I think it should go via the thermal tree. Cheers, Javi
diff --git a/drivers/thermal/db8500_cpufreq_cooling.c b/drivers/thermal/db8500_cpufreq_cooling.c index 786d192..1fa46ff 100644 --- a/drivers/thermal/db8500_cpufreq_cooling.c +++ b/drivers/thermal/db8500_cpufreq_cooling.c @@ -34,6 +34,7 @@ static int db8500_cpufreq_cooling_probe(struct platform_device *pdev) if (!cpufreq_frequency_get_table(0)) return -EPROBE_DEFER; + cpumask_clear(&mask_val); cpumask_set_cpu(0, &mask_val); cdev = cpufreq_cooling_register(&mask_val); diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index a99c631..a21acf8 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -449,6 +449,7 @@ static int imx_thermal_probe(struct platform_device *pdev) regmap_write(map, MISC0 + REG_SET, MISC0_REFTOP_SELBIASOFF); regmap_write(map, TEMPSENSE0 + REG_SET, TEMPSENSE0_POWER_DOWN); + cpumask_clear(&clip_cpus); cpumask_set_cpu(0, &clip_cpus); data->cdev = cpufreq_cooling_register(&clip_cpus); if (IS_ERR(data->cdev)) { diff --git a/drivers/thermal/samsung/exynos_thermal_common.c b/drivers/thermal/samsung/exynos_thermal_common.c index 3f5ad25..47efa4c 100644 --- a/drivers/thermal/samsung/exynos_thermal_common.c +++ b/drivers/thermal/samsung/exynos_thermal_common.c @@ -361,6 +361,7 @@ int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf) return -ENOMEM; th_zone->sensor_conf = sensor_conf; + cpumask_clear(&mask_val); /* * TODO: 1) Handle multiple cooling devices in a thermal zone * 2) Add a flag/name in cooling info to map to specific
Cpumasks should be cleared before using. Signed-off-by: Jonghwan Choi <jhbird.choi@samsung.com> --- drivers/thermal/db8500_cpufreq_cooling.c | 1 + drivers/thermal/imx_thermal.c | 1 + drivers/thermal/samsung/exynos_thermal_common.c | 1 + 3 files changed, 3 insertions(+)