This is from Ubuntu Saucy based on 3.10-rc7:
[ 12.911676] BUG: unable to handle kernel NULL pointer dereference at
0000000000000070
[ 12.911691] IP: [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
For completeness I added the attached debug patch and built a vanilla
3.10-rc7 with the following result:
[ 13.222262] od_set_powersave_bias !policy, cpu 0
[ 13.222843] od_set_powersave_bias !policy, cpu 1
[ 13.223380] od_set_powersave_bias !policy, cpu 2
[ 13.223922] od_set_powersave_bias !policy, cpu 3
Attachments:
dmesg.txt - ubuntu kernel rebased on 3.10-rc7
dmesg_dbg.txt - vanilla 3.10-rc7 with debug patch
0001-cpufreq_ondemand.c-Added-debug.patch - debug patch
config - 3.10-rc7 config
rtg
--
Tim Gardner [email protected]
On 24 June 2013 22:29, Tim Gardner <[email protected]> wrote:
> This is from Ubuntu Saucy based on 3.10-rc7:
>
> [ 12.911676] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000070
> [ 12.911691] IP: [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
>
> For completeness I added the attached debug patch and built a vanilla
> 3.10-rc7 with the following result:
>
> [ 13.222262] od_set_powersave_bias !policy, cpu 0
> [ 13.222843] od_set_powersave_bias !policy, cpu 1
> [ 13.223380] od_set_powersave_bias !policy, cpu 2
> [ 13.223922] od_set_powersave_bias !policy, cpu 3
>
> Attachments:
> dmesg.txt - ubuntu kernel rebased on 3.10-rc7
> dmesg_dbg.txt - vanilla 3.10-rc7 with debug patch
> 0001-cpufreq_ondemand.c-Added-debug.patch - debug patch
> config - 3.10-rc7 config
Can you please look into this bug? It occurred after your
patch... This is the boot log crash we have:
I believe this is somehow called before ondemand is initialized.
Also, I see one problem in your original patch:
commit fb30809efa3edeb692a6b29125a07c9eceb322dc
Author: Jacob Shin <[email protected]>
Date: Tue Apr 2 09:56:56 2013 -0500
cpufreq: ondemand: allow custom powersave_bias_target handler to
be registered
You are doing:
+ for_each_online_cpu(cpu) {
+ if (cpumask_test_cpu(cpu, &done))
+ continue;
+
+ policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
+ dbs_data = policy->governor_data;
+ od_tuners = dbs_data->tuners;
+ od_tuners->powersave_bias = powersave_bias;
+
+ cpumask_or(&done, &done, policy->cpus);
+ }
How can we do this for each online cpu? There might be two
clusters each using a separate governor and so this looks wrong.
Can you please send a fixup for this?
[ 12.911719] Modules linked in: amd_freq_sensitivity(+) kvm_amd kvm
snd_hda_intel(+) snd_hda_codec crc32_pclmul ghash_clmulni_intel
snd_hwdep snd_pcm aesni_intel ablk_helper snd_seq_midi cryptd lrw
snd_rawmidi snd_seq_midi_event gf128mul snd_seq glue_helper aes_x86_64
snd_timer snd_seq_device psmouse edac_core snd joydev microcode
i2c_piix4 soundcore snd_page_alloc video edac_mce_amd bcma mac_hid
fam15h_power serio_raw lp parport hid_generic usbhid hid sdhci_pci
sdhci ahci libahci alx mdio
[ 12.911782] CPU: 0 PID: 605 Comm: modprobe Not tainted 3.10.0-0-generic #6
[ 12.911789] Hardware name: AMD Larne/Larne, BIOS
WLR3206X_Weekly_13_02_0 02/06/2013
[ 12.911795] task: ffff880115cd2ee0 ti: ffff880118140000 task.ti:
ffff880118140000
[ 12.911800] RIP: 0010:[<ffffffff8156e572>] [<ffffffff8156e572>]
od_set_powersave_bias+0x92/0xc0
[ 12.911809] RSP: 0018:ffff880118141d00 EFLAGS: 00010246
[ 12.911814] RAX: ffff88011ec00000 RBX: 0000000000000000 RCX: 0000000000000100
[ 12.911819] RDX: 0000000000000000 RSI: ffff880118141d00 RDI: ffff880118141d00
[ 12.911824] RBP: ffff880118141d40 R08: ffffffff81cf2ee0 R09: 0000000000000004
[ 12.911829] R10: ffff88011ec14fc8 R11: 0000000000014480 R12: ffffffff81cf2ee0
[ 12.911835] R13: 0000000000010c80 R14: 0000000000000190 R15: ffff880118141ef0
[ 12.911842] FS: 00007f0688e8d700(0000) GS:ffff88011ec00000(0000)
knlGS:0000000000000000
[ 12.911849] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 12.911854] CR2: 0000000000000070 CR3: 0000000115cc4000 CR4: 00000000000407f0
[ 12.911860] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 12.911866] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 12.911871] Stack:
[ 12.911875] 0000000000000000 0000000000000000 0000000000000000
0000000000000000
[ 12.911885] 0000000000000000 0000000000000000 ffffffffa0280000
ffffffffa0280018
[ 12.911894] ffff880118141d50 ffffffff8156e5b7 ffff880118141d70
ffffffffa001e046
[ 12.911904] Call Trace:
[ 12.911914] [<ffffffff8156e5b7>]
od_register_powersave_bias_handler+0x17/0x20
[ 12.911925] [<ffffffffa001e046>]
amd_freq_sensitivity_init+0x46/0x1000 [amd_freq_sensitivity]
[ 12.911935] [<ffffffffa001e000>] ? 0xffffffffa001dfff
[ 12.911945] [<ffffffff81002102>] do_one_initcall+0x102/0x160
[ 12.911955] [<ffffffff810bee8c>] load_module+0x101c/0x1400
[ 12.911964] [<ffffffff810baa40>] ? store_uevent+0x40/0x40
[ 12.911973] [<ffffffff810bf31d>] SyS_init_module+0xad/0xd0
[ 12.911983] [<ffffffff816defef>] tracesys+0xe1/0xe6
[ 12.911988] Code: 78 00 89 c3 76 3b 0f a3 45 c0 19 d2 85 d2 75 d9
89 c0 48 8d 75 c0 b9 00 01 00 00 48 8b 04 c5 80 21 cf 81 48 89 f7 49
8b 54 05 20 <48> 8b 42 70 48 8b 40 10 44 89 70 14 e8 bd d5 dd ff eb ab
0f 1f
[ 12.912054] RIP [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
[ 12.912062] RSP <ffff880118141d00>
[ 12.912066] CR2: 0000000000000070
[ 12.912073] ---[ end trace 98b1cd8b10d00b40 ]---
[ 13.025093] input: HD-Audio Generic HDMI/DP,pcm=7 as
/devices/pci0000:00/0000:00:01.1/sound/card0/input9
[ 13.025267] input: HD-Audio Generic HDMI/DP,pcm=3 as
/devices/pci0000:00/0000:00:01.1/sound/card0/input10
[ 13.025892] hda-intel 0000:00:14.2: Using LPIB position fix
[ 13.030414] hda-intel 0000:00:14.2: Enable sync_write for stable
communication
[ 13.055253] hda_codec: CX20751/2: BIOS auto-probing.
[ 13.055499] autoconfig: line_outs=1 (0x17/0x0/0x0/0x0/0x0) type:speaker
[ 13.055506] speaker_outs=0 (0x0/0x0/0x0/0x0/0x0)
[ 13.055512] hp_outs=1 (0x16/0x0/0x0/0x0/0x0)
[ 13.055516] mono: mono_out=0x0
[ 13.055520] inputs:
[ 13.055525] Internal Mic=0x1a
[ 13.055530] Mic=0x18
On Tue, Jun 25, 2013 at 12:26:14PM +0530, Viresh Kumar wrote:
> On 24 June 2013 22:29, Tim Gardner <[email protected]> wrote:
> > This is from Ubuntu Saucy based on 3.10-rc7:
> >
> > [ 12.911676] BUG: unable to handle kernel NULL pointer dereference at
> > 0000000000000070
> > [ 12.911691] IP: [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
> >
>
> Can you please look into this bug? It occurred after your
> patch... This is the boot log crash we have:
Hi,
Yes, so sorry about that, it looks like I failed to test with:
CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
CONFIG_CPU_FREQ_GOV_ONDEMAND=y
CONFIG_X86_AMD_FREQ_SENSITIVITY=m
The following patch fixes this, Tim, could you please test ? :
---8<---
>From 3c727b1f775448599e67c5fb2121d79448e80c90 Mon Sep 17 00:00:00 2001
From: Jacob Shin <[email protected]>
Date: Tue, 25 Jun 2013 10:40:54 -0500
Subject: [PATCH 1/1] cpufreq: fix NULL pointer deference at
od_set_powersave_bias()
When initializing the default powersave_bias value, we need to first
make sure that this policy is running the ondemand governor.
Reported-by: Tim Gardner <[email protected]>
Signed-off-by: Jacob Shin <[email protected]>
---
drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 4b9bb5d..93eb5cb 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -47,6 +47,8 @@ static struct od_ops od_ops;
static struct cpufreq_governor cpufreq_gov_ondemand;
#endif
+static unsigned int default_powersave_bias;
+
static void ondemand_powersave_bias_init_cpu(int cpu)
{
struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
@@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
tuners->ignore_nice = 0;
- tuners->powersave_bias = 0;
+ tuners->powersave_bias = default_powersave_bias;
tuners->io_is_busy = should_io_be_busy();
dbs_data->tuners = tuners;
@@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
unsigned int cpu;
cpumask_t done;
+ default_powersave_bias = powersave_bias;
cpumask_clear(&done);
get_online_cpus();
@@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
continue;
policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
- dbs_data = policy->governor_data;
- od_tuners = dbs_data->tuners;
- od_tuners->powersave_bias = powersave_bias;
+ if (!policy)
+ continue;
cpumask_or(&done, &done, policy->cpus);
+
+ if (policy->governor != &cpufreq_gov_ondemand)
+ continue;
+
+ dbs_data = policy->governor_data;
+ od_tuners = dbs_data->tuners;
+ od_tuners->powersave_bias = default_powersave_bias;
}
put_online_cpus();
}
--
1.7.9.5
On 06/25/2013 10:19 AM, Jacob Shin wrote:
> On Tue, Jun 25, 2013 at 12:26:14PM +0530, Viresh Kumar wrote:
>> On 24 June 2013 22:29, Tim Gardner <[email protected]> wrote:
>>> This is from Ubuntu Saucy based on 3.10-rc7:
>>>
>>> [ 12.911676] BUG: unable to handle kernel NULL pointer dereference at
>>> 0000000000000070
>>> [ 12.911691] IP: [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
>>>
>>
>> Can you please look into this bug? It occurred after your
>> patch... This is the boot log crash we have:
>
> Hi,
>
> Yes, so sorry about that, it looks like I failed to test with:
>
> CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
> CONFIG_CPU_FREQ_GOV_ONDEMAND=y
> CONFIG_X86_AMD_FREQ_SENSITIVITY=m
>
> The following patch fixes this, Tim, could you please test ? :
>
> ---8<---
>
> From 3c727b1f775448599e67c5fb2121d79448e80c90 Mon Sep 17 00:00:00 2001
> From: Jacob Shin <[email protected]>
> Date: Tue, 25 Jun 2013 10:40:54 -0500
> Subject: [PATCH 1/1] cpufreq: fix NULL pointer deference at
> od_set_powersave_bias()
>
> When initializing the default powersave_bias value, we need to first
> make sure that this policy is running the ondemand governor.
>
> Reported-by: Tim Gardner <[email protected]>
> Signed-off-by: Jacob Shin <[email protected]>
> ---
> drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 4b9bb5d..93eb5cb 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> static struct cpufreq_governor cpufreq_gov_ondemand;
> #endif
>
> +static unsigned int default_powersave_bias;
> +
> static void ondemand_powersave_bias_init_cpu(int cpu)
> {
> struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> @@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
>
> tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
> tuners->ignore_nice = 0;
> - tuners->powersave_bias = 0;
> + tuners->powersave_bias = default_powersave_bias;
> tuners->io_is_busy = should_io_be_busy();
>
> dbs_data->tuners = tuners;
> @@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> unsigned int cpu;
> cpumask_t done;
>
> + default_powersave_bias = powersave_bias;
> cpumask_clear(&done);
>
> get_online_cpus();
> @@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> continue;
>
> policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
> - dbs_data = policy->governor_data;
> - od_tuners = dbs_data->tuners;
> - od_tuners->powersave_bias = powersave_bias;
> + if (!policy)
> + continue;
>
> cpumask_or(&done, &done, policy->cpus);
> +
> + if (policy->governor != &cpufreq_gov_ondemand)
> + continue;
> +
> + dbs_data = policy->governor_data;
> + od_tuners = dbs_data->tuners;
> + od_tuners->powersave_bias = default_powersave_bias;
> }
> put_online_cpus();
> }
>
That appears to have done the trick. You can add my Tested-by.
rtg
--
Tim Gardner [email protected]
On Tuesday, June 25, 2013 12:41:07 PM Tim Gardner wrote:
> On 06/25/2013 10:19 AM, Jacob Shin wrote:
> > On Tue, Jun 25, 2013 at 12:26:14PM +0530, Viresh Kumar wrote:
> >> On 24 June 2013 22:29, Tim Gardner <[email protected]> wrote:
> >>> This is from Ubuntu Saucy based on 3.10-rc7:
> >>>
> >>> [ 12.911676] BUG: unable to handle kernel NULL pointer dereference at
> >>> 0000000000000070
> >>> [ 12.911691] IP: [<ffffffff8156e572>] od_set_powersave_bias+0x92/0xc0
> >>>
> >>
> >> Can you please look into this bug? It occurred after your
> >> patch... This is the boot log crash we have:
> >
> > Hi,
> >
> > Yes, so sorry about that, it looks like I failed to test with:
> >
> > CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> > CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
> > CONFIG_CPU_FREQ_GOV_ONDEMAND=y
> > CONFIG_X86_AMD_FREQ_SENSITIVITY=m
> >
> > The following patch fixes this, Tim, could you please test ? :
> >
> > ---8<---
> >
> > From 3c727b1f775448599e67c5fb2121d79448e80c90 Mon Sep 17 00:00:00 2001
> > From: Jacob Shin <[email protected]>
> > Date: Tue, 25 Jun 2013 10:40:54 -0500
> > Subject: [PATCH 1/1] cpufreq: fix NULL pointer deference at
> > od_set_powersave_bias()
> >
> > When initializing the default powersave_bias value, we need to first
> > make sure that this policy is running the ondemand governor.
> >
> > Reported-by: Tim Gardner <[email protected]>
> > Signed-off-by: Jacob Shin <[email protected]>
> > ---
> > drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> > index 4b9bb5d..93eb5cb 100644
> > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> > static struct cpufreq_governor cpufreq_gov_ondemand;
> > #endif
> >
> > +static unsigned int default_powersave_bias;
> > +
> > static void ondemand_powersave_bias_init_cpu(int cpu)
> > {
> > struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> > @@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
> >
> > tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
> > tuners->ignore_nice = 0;
> > - tuners->powersave_bias = 0;
> > + tuners->powersave_bias = default_powersave_bias;
> > tuners->io_is_busy = should_io_be_busy();
> >
> > dbs_data->tuners = tuners;
> > @@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> > unsigned int cpu;
> > cpumask_t done;
> >
> > + default_powersave_bias = powersave_bias;
> > cpumask_clear(&done);
> >
> > get_online_cpus();
> > @@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> > continue;
> >
> > policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
> > - dbs_data = policy->governor_data;
> > - od_tuners = dbs_data->tuners;
> > - od_tuners->powersave_bias = powersave_bias;
> > + if (!policy)
> > + continue;
> >
> > cpumask_or(&done, &done, policy->cpus);
> > +
> > + if (policy->governor != &cpufreq_gov_ondemand)
> > + continue;
> > +
> > + dbs_data = policy->governor_data;
> > + od_tuners = dbs_data->tuners;
> > + od_tuners->powersave_bias = default_powersave_bias;
> > }
> > put_online_cpus();
> > }
> >
>
> That appears to have done the trick. You can add my Tested-by.
Queued up as an urgent fix.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On 25 June 2013 21:49, Jacob Shin <[email protected]> wrote:
> Yes, so sorry about that, it looks like I failed to test with:
No problem, it happens :)
> CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
> CONFIG_CPU_FREQ_GOV_ONDEMAND=y
> CONFIG_X86_AMD_FREQ_SENSITIVITY=m
>
> The following patch fixes this, Tim, could you please test ? :
>
> ---8<---
>
> From 3c727b1f775448599e67c5fb2121d79448e80c90 Mon Sep 17 00:00:00 2001
> From: Jacob Shin <[email protected]>
> Date: Tue, 25 Jun 2013 10:40:54 -0500
> Subject: [PATCH 1/1] cpufreq: fix NULL pointer deference at
> od_set_powersave_bias()
>
> When initializing the default powersave_bias value, we need to first
> make sure that this policy is running the ondemand governor.
>
> Reported-by: Tim Gardner <[email protected]>
> Signed-off-by: Jacob Shin <[email protected]>
> ---
> drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 4b9bb5d..93eb5cb 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> static struct cpufreq_governor cpufreq_gov_ondemand;
> #endif
>
> +static unsigned int default_powersave_bias;
> +
> static void ondemand_powersave_bias_init_cpu(int cpu)
> {
> struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> @@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
>
> tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
> tuners->ignore_nice = 0;
> - tuners->powersave_bias = 0;
> + tuners->powersave_bias = default_powersave_bias;
> tuners->io_is_busy = should_io_be_busy();
>
> dbs_data->tuners = tuners;
> @@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> unsigned int cpu;
> cpumask_t done;
>
> + default_powersave_bias = powersave_bias;
Why are the above three changes required? And in case they are, then
they must have been commited separately.
> cpumask_clear(&done);
>
> get_online_cpus();
> @@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> continue;
>
> policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
> - dbs_data = policy->governor_data;
> - od_tuners = dbs_data->tuners;
> - od_tuners->powersave_bias = powersave_bias;
> + if (!policy)
> + continue;
I am not sure if this is enough. What if we had ondemand as the
governor initially, then we changed it to something else. Now also
cur_policy contains a address and isn't zero.
> cpumask_or(&done, &done, policy->cpus);
> +
> + if (policy->governor != &cpufreq_gov_ondemand)
> + continue;
> +
> + dbs_data = policy->governor_data;
> + od_tuners = dbs_data->tuners;
> + od_tuners->powersave_bias = default_powersave_bias;
> }
> put_online_cpus();
> }
> --
> 1.7.9.5
>
>
On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
> On 25 June 2013 21:49, Jacob Shin <[email protected]> wrote:
> > Yes, so sorry about that, it looks like I failed to test with:
>
> No problem, it happens :)
>
> > CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE=y
> > CONFIG_CPU_FREQ_GOV_PERFORMANCE=y
> > CONFIG_CPU_FREQ_GOV_ONDEMAND=y
> > CONFIG_X86_AMD_FREQ_SENSITIVITY=m
> >
> > The following patch fixes this, Tim, could you please test ? :
> >
> > ---8<---
> >
> > From 3c727b1f775448599e67c5fb2121d79448e80c90 Mon Sep 17 00:00:00 2001
> > From: Jacob Shin <[email protected]>
> > Date: Tue, 25 Jun 2013 10:40:54 -0500
> > Subject: [PATCH 1/1] cpufreq: fix NULL pointer deference at
> > od_set_powersave_bias()
> >
> > When initializing the default powersave_bias value, we need to first
> > make sure that this policy is running the ondemand governor.
> >
> > Reported-by: Tim Gardner <[email protected]>
> > Signed-off-by: Jacob Shin <[email protected]>
> > ---
> > drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
> > 1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> > index 4b9bb5d..93eb5cb 100644
> > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> > static struct cpufreq_governor cpufreq_gov_ondemand;
> > #endif
> >
> > +static unsigned int default_powersave_bias;
> > +
> > static void ondemand_powersave_bias_init_cpu(int cpu)
> > {
> > struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> > @@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
> >
> > tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
> > tuners->ignore_nice = 0;
> > - tuners->powersave_bias = 0;
> > + tuners->powersave_bias = default_powersave_bias;
> > tuners->io_is_busy = should_io_be_busy();
> >
> > dbs_data->tuners = tuners;
> > @@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> > unsigned int cpu;
> > cpumask_t done;
> >
> > + default_powersave_bias = powersave_bias;
>
> Why are the above three changes required? And in case they are, then
> they must have been commited separately.
>
> > cpumask_clear(&done);
> >
> > get_online_cpus();
> > @@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
> > continue;
> >
> > policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
> > - dbs_data = policy->governor_data;
> > - od_tuners = dbs_data->tuners;
> > - od_tuners->powersave_bias = powersave_bias;
> > + if (!policy)
> > + continue;
>
> I am not sure if this is enough. What if we had ondemand as the
> governor initially, then we changed it to something else. Now also
> cur_policy contains a address and isn't zero.
Right, so we check below ..
>
> > cpumask_or(&done, &done, policy->cpus);
> > +
> > + if (policy->governor != &cpufreq_gov_ondemand)
> > + continue;
This should catch that case no ?
> > +
> > + dbs_data = policy->governor_data;
> > + od_tuners = dbs_data->tuners;
> > + od_tuners->powersave_bias = default_powersave_bias;
> > }
> > put_online_cpus();
> > }
> > --
> > 1.7.9.5
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 26 June 2013 19:58, Jacob Shin <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
>> I am not sure if this is enough. What if we had ondemand as the
>> governor initially, then we changed it to something else. Now also
>> cur_policy contains a address and isn't zero.
>
> Right, so we check below ..
>
>>
>> > cpumask_or(&done, &done, policy->cpus);
>> > +
>> > + if (policy->governor != &cpufreq_gov_ondemand)
>> > + continue;
>
> This should catch that case no ?
Policy might be freed and reallocated by then. And so doing
policy->governor is dangerous.
On Wed, Jun 26, 2013 at 08:02:29PM +0530, Viresh Kumar wrote:
> On 26 June 2013 19:58, Jacob Shin <[email protected]> wrote:
> > On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
>
> >> I am not sure if this is enough. What if we had ondemand as the
> >> governor initially, then we changed it to something else. Now also
> >> cur_policy contains a address and isn't zero.
> >
> > Right, so we check below ..
> >
> >>
> >> > cpumask_or(&done, &done, policy->cpus);
> >> > +
> >> > + if (policy->governor != &cpufreq_gov_ondemand)
> >> > + continue;
> >
> > This should catch that case no ?
>
> Policy might be freed and reallocated by then. And so doing
> policy->governor is dangerous.
Hm . any hints on how to check for if ondemand is running on this CPU
or not ? I'm not sure what the best way to handle this is ..
Thanks,
On Wed, Jun 26, 2013 at 08:02:29PM +0530, Viresh Kumar wrote:
> On 26 June 2013 19:58, Jacob Shin <[email protected]> wrote:
> > On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
>
> >> I am not sure if this is enough. What if we had ondemand as the
> >> governor initially, then we changed it to something else. Now also
> >> cur_policy contains a address and isn't zero.
I just tested this case with this patch applied, and did not have any
problems.
> >>
> >> > cpumask_or(&done, &done, policy->cpus);
> >> > +
> >> > + if (policy->governor != &cpufreq_gov_ondemand)
> >> > + continue;
> >
> > This should catch that case no ?
>
> Policy might be freed and reallocated by then. And so doing
> policy->governor is dangerous.
Are you worried that after we have passed the above if check, and
before we access ->tuner governor change might occur?
Is there something synonymous to get/put_online_cpus() for cpufreq to
prevent governor change while we update ->tuner values?
Otherwise, should just spinlock?
On 26 June 2013 23:27, Jacob Shin <[email protected]> wrote:
> On Wed, Jun 26, 2013 at 08:02:29PM +0530, Viresh Kumar wrote:
>> On 26 June 2013 19:58, Jacob Shin <[email protected]> wrote:
>> > On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
>>
>> >> I am not sure if this is enough. What if we had ondemand as the
>> >> governor initially, then we changed it to something else. Now also
>> >> cur_policy contains a address and isn't zero.
>
> I just tested this case with this patch applied, and did not have any
> problems.
Try this:
- you need a system with multiple policy groups to test it
- Suppose we have two groups of CPUs: 0 and 1
- Set ondemand as governor for both
- change governor of group 1 to something else (we still have valid policy
struct in Ondemand)
- offline all CPUs from group 1. this will free struct cpufreq_policy
- Online these CPUs back, this will reallocate policy
- Now run this function, the earlier policy struct is already freed and
you are accessing it here.
>> >> > cpumask_or(&done, &done, policy->cpus);
>> >> > +
>> >> > + if (policy->governor != &cpufreq_gov_ondemand)
>> >> > + continue;
>> >
>> > This should catch that case no ?
>>
>> Policy might be freed and reallocated by then. And so doing
>> policy->governor is dangerous.
>
> Are you worried that after we have passed the above if check, and
> before we access ->tuner governor change might occur?
>
> Is there something synonymous to get/put_online_cpus() for cpufreq to
> prevent governor change while we update ->tuner values?
>
> Otherwise, should just spinlock?
No, i wasn't worrying about this but a sequence of events that I told to
you earlier.
Replying to your other mail:
> Hm . any hints on how to check for if ondemand is running on this CPU
> or not ? I'm not sure what the best way to handle this is ..
Make cur_policy zero in cpufreq_governor_dbs() for
CPUFREQ_GOV_STOP notification. This will make sure we use correct
policy pointer.
On Thu, Jun 27, 2013 at 12:32:36PM +0530, Viresh Kumar wrote:
> On 26 June 2013 23:27, Jacob Shin <[email protected]> wrote:
> > On Wed, Jun 26, 2013 at 08:02:29PM +0530, Viresh Kumar wrote:
> >> On 26 June 2013 19:58, Jacob Shin <[email protected]> wrote:
> >> > On Wed, Jun 26, 2013 at 12:18:27PM +0530, Viresh Kumar wrote:
> >>
> >> >> I am not sure if this is enough. What if we had ondemand as the
> >> >> governor initially, then we changed it to something else. Now also
> >> >> cur_policy contains a address and isn't zero.
> >
> > I just tested this case with this patch applied, and did not have any
> > problems.
>
> Try this:
> - you need a system with multiple policy groups to test it
> - Suppose we have two groups of CPUs: 0 and 1
> - Set ondemand as governor for both
> - change governor of group 1 to something else (we still have valid policy
> struct in Ondemand)
> - offline all CPUs from group 1. this will free struct cpufreq_policy
> - Online these CPUs back, this will reallocate policy
> - Now run this function, the earlier policy struct is already freed and
> you are accessing it here.
Ah, I understand now.
>
> >> >> > cpumask_or(&done, &done, policy->cpus);
> >> >> > +
> >> >> > + if (policy->governor != &cpufreq_gov_ondemand)
> >> >> > + continue;
> >> >
> >> > This should catch that case no ?
> >>
> >> Policy might be freed and reallocated by then. And so doing
> >> policy->governor is dangerous.
> >
> > Are you worried that after we have passed the above if check, and
> > before we access ->tuner governor change might occur?
> >
> > Is there something synonymous to get/put_online_cpus() for cpufreq to
> > prevent governor change while we update ->tuner values?
> >
> > Otherwise, should just spinlock?
>
> No, i wasn't worrying about this but a sequence of events that I told to
> you earlier.
>
> Replying to your other mail:
> > Hm . any hints on how to check for if ondemand is running on this CPU
> > or not ? I'm not sure what the best way to handle this is ..
>
> Make cur_policy zero in cpufreq_governor_dbs() for
> CPUFREQ_GOV_STOP notification. This will make sure we use correct
> policy pointer.
Thanks for the tip :-)
Here is V2:
---8<---
>From d99ee318c0f9c415a60e6b0b79605232edbb749c Mon Sep 17 00:00:00 2001
From: Jacob Shin <[email protected]>
Date: Thu, 27 Jun 2013 09:39:48 -0500
Subject: [PATCH V2 1/1] cpufreq: fix NULL pointer deference at
od_set_powersave_bias()
When initializing the default powersave_bias value, we need to first
make sure that this policy is running the ondemand governor.
Reported-by: Tim Gardner <[email protected]>
Signed-off-by: Jacob Shin <[email protected]>
---
drivers/cpufreq/cpufreq_governor.c | 1 +
drivers/cpufreq/cpufreq_ondemand.c | 17 +++++++++++++----
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index dc9b72e..834ad86 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -403,6 +403,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
gov_cancel_work(dbs_data, policy);
mutex_lock(&dbs_data->mutex);
+ cpu_cdbs->cur_policy = NULL;
mutex_destroy(&cpu_cdbs->timer_mutex);
mutex_unlock(&dbs_data->mutex);
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 4b9bb5d..93eb5cb 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -47,6 +47,8 @@ static struct od_ops od_ops;
static struct cpufreq_governor cpufreq_gov_ondemand;
#endif
+static unsigned int default_powersave_bias;
+
static void ondemand_powersave_bias_init_cpu(int cpu)
{
struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
@@ -543,7 +545,7 @@ static int od_init(struct dbs_data *dbs_data)
tuners->sampling_down_factor = DEF_SAMPLING_DOWN_FACTOR;
tuners->ignore_nice = 0;
- tuners->powersave_bias = 0;
+ tuners->powersave_bias = default_powersave_bias;
tuners->io_is_busy = should_io_be_busy();
dbs_data->tuners = tuners;
@@ -585,6 +587,7 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
unsigned int cpu;
cpumask_t done;
+ default_powersave_bias = powersave_bias;
cpumask_clear(&done);
get_online_cpus();
@@ -593,11 +596,17 @@ static void od_set_powersave_bias(unsigned int powersave_bias)
continue;
policy = per_cpu(od_cpu_dbs_info, cpu).cdbs.cur_policy;
- dbs_data = policy->governor_data;
- od_tuners = dbs_data->tuners;
- od_tuners->powersave_bias = powersave_bias;
+ if (!policy)
+ continue;
cpumask_or(&done, &done, policy->cpus);
+
+ if (policy->governor != &cpufreq_gov_ondemand)
+ continue;
+
+ dbs_data = policy->governor_data;
+ od_tuners = dbs_data->tuners;
+ od_tuners->powersave_bias = default_powersave_bias;
}
put_online_cpus();
}
--
1.7.10.4
On 27 June 2013 20:25, Jacob Shin <[email protected]> wrote:
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 4b9bb5d..93eb5cb 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> static struct cpufreq_governor cpufreq_gov_ondemand;
> #endif
>
> +static unsigned int default_powersave_bias;
Because you haven't replied to my earlier comment on this, I
thought you agreed. But it looks you haven't read my first
reply well.
On 27 June 2013 20:39, Viresh Kumar <[email protected]> wrote:
> On 27 June 2013 20:25, Jacob Shin <[email protected]> wrote:
>> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
>> index 4b9bb5d..93eb5cb 100644
>> --- a/drivers/cpufreq/cpufreq_ondemand.c
>> +++ b/drivers/cpufreq/cpufreq_ondemand.c
>> @@ -47,6 +47,8 @@ static struct od_ops od_ops;
>> static struct cpufreq_governor cpufreq_gov_ondemand;
>> #endif
>>
>> +static unsigned int default_powersave_bias;
>
> Because you haven't replied to my earlier comment on this, I
> thought you agreed. But it looks you haven't read my first
> reply well.
BTW, your v1 is already applied and sent to Linus. So,
you need to send only fixup now.
But, you still need to answer why you need
default_powersave_bias? And if it is not a must then should be
removed.
On Thu, Jun 27, 2013 at 08:39:12PM +0530, Viresh Kumar wrote:
> On 27 June 2013 20:25, Jacob Shin <[email protected]> wrote:
> > diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> > index 4b9bb5d..93eb5cb 100644
> > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -47,6 +47,8 @@ static struct od_ops od_ops;
> > static struct cpufreq_governor cpufreq_gov_ondemand;
> > #endif
> >
> > +static unsigned int default_powersave_bias;
>
> Why are the above three changes required? And in case they are, then
> they must have been commited separately.
>
> Because you haven't replied to my earlier comment on this, I
> thought you agreed. But it looks you haven't read my first
> reply well.
Sorry I totally missed it,
I was trying to account for the case when od_set_powersave_bias() is
called while ondemand is not the current governor. And then later
becomes the current governor. So when od_init() is called it will take
the expected default, rather than 0.
On 27 June 2013 20:51, Jacob Shin <[email protected]> wrote:
> Sorry I totally missed it,
>
> I was trying to account for the case when od_set_powersave_bias() is
> called while ondemand is not the current governor. And then later
> becomes the current governor. So when od_init() is called it will take
> the expected default, rather than 0.
Ok. leave it as is.
On Thu, Jun 27, 2013 at 08:50:44PM +0530, Viresh Kumar wrote:
> BTW, your v1 is already applied and sent to Linus. So,
> you need to send only fixup now.
Here is the fixup delta patch, thanks! :
---8<---
>From 133f670aa1dc450fd37ba39081d046bf238c507b Mon Sep 17 00:00:00 2001
From: Jacob Shin <[email protected]>
Date: Thu, 27 Jun 2013 10:44:42 -0500
Subject: [PATCH 1/1] cpufreq: don't leave stale policy pointer in
cdbs->cur_policy
Set ->cur_policy to NULL when stopping a governor. Otherwise, the
->cur_policy pointer may be stale on systems that
have_governor_per_policy when a new policy is allocated due to CPU
hotplug offline/online.
Suggested-by: Viresh Kumar <[email protected]>
Signed-off-by: Jacob Shin <[email protected]>
---
drivers/cpufreq/cpufreq_governor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index dc9b72e..320474e 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -404,6 +404,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
mutex_lock(&dbs_data->mutex);
mutex_destroy(&cpu_cdbs->timer_mutex);
+ cpu_cdbs->cur_policy = NULL;
mutex_unlock(&dbs_data->mutex);
--
1.7.10.4
On 27 June 2013 21:52, Jacob Shin <[email protected]> wrote:
> From 133f670aa1dc450fd37ba39081d046bf238c507b Mon Sep 17 00:00:00 2001
> From: Jacob Shin <[email protected]>
> Date: Thu, 27 Jun 2013 10:44:42 -0500
> Subject: [PATCH 1/1] cpufreq: don't leave stale policy pointer in
> cdbs->cur_policy
>
> Set ->cur_policy to NULL when stopping a governor. Otherwise, the
> ->cur_policy pointer may be stale on systems that
> have_governor_per_policy when a new policy is allocated due to CPU
> hotplug offline/online.
>
> Suggested-by: Viresh Kumar <[email protected]>
> Signed-off-by: Jacob Shin <[email protected]>
> ---
> drivers/cpufreq/cpufreq_governor.c | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Viresh Kumar <[email protected]>