RE: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Kushwaha, Priyalee]

I will resubmit the patch with suggested changes. 

-Original Message-
From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] 
Sent: Saturday, May 27, 2017 3:52 AM
To: Pavel Machek <pa...@ucw.cz>
Cc: Darren Hart <dvh...@infradead.org>; Kushwaha, Priyalee 
<priyalee.kushw...@intel.com>; Chakravarty, Souvik K 
<souvik.k.chakrava...@intel.com>; Andy Shevchenko <a...@infradead.org>; 
linux-kernel@vger.kernel.org; Platform Driver 
<platform-driver-...@vger.kernel.org>; Rafael Wysocki <r...@rjwysocki.net>; 
Brown, Len <len.br...@intel.com>; linux...@vger.kernel.org
Subject: Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while 
load/unload module test

On Sat, May 27, 2017 at 9:54 AM, Pavel Machek <pa...@ucw.cz> wrote:
>> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:

>> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for 
>> +dealing with
>>CONFIG_PM_SLEEP?
>
> Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds 
> like a good idea.

I like the idea, though I would let Rafael to speak up.

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Kushwaha, Priyalee]

I will resubmit the patch with suggested changes. 

-Original Message-
From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] 
Sent: Saturday, May 27, 2017 3:52 AM
To: Pavel Machek 
Cc: Darren Hart ; Kushwaha, Priyalee 
; Chakravarty, Souvik K 
; Andy Shevchenko ; 
linux-kernel@vger.kernel.org; Platform Driver 
; Rafael Wysocki ; 
Brown, Len ; linux...@vger.kernel.org
Subject: Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while 
load/unload module test

On Sat, May 27, 2017 at 9:54 AM, Pavel Machek  wrote:
>> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:

>> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for 
>> +dealing with
>>CONFIG_PM_SLEEP?
>
> Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds 
> like a good idea.

I like the idea, though I would let Rafael to speak up.

--
With Best Regards,
Andy Shevchenko

RE: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Chakravarty, Souvik K]

Thanks for catching this.

BR,
Souvik

> -Original Message-
> From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-
> x86-ow...@vger.kernel.org] On Behalf Of priyalee.kushw...@intel.com
> Sent: Saturday, May 27, 2017 8:48 PM
> To: dvh...@infradead.org; Chakravarty, Souvik K
> ; a...@infradead.org
> Cc: linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org;
> Kushwaha, Priyalee 
> Subject: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while
> load/unload module test
> 
> From: Priyalee Kushwaha 
> 
> This fix oops found while testing load/unload test of
> intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> for PM callbacks, but unregister_pm_notifier was missing from
> module_exit.
> 
>  [ 97.481860] BUG: unable to handle kernel paging request at
> a006f010  [ 97.489742] IP:
> blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.495898] PGD 2e0a067
>  [ 97.495899] PUD 2e0b063
>  [ 97.498737] PMD 179e29067
>  [ 97.501573] PTE 0
> 
>  [ 97.508423] Oops:  1 PREEMPT SMP
>  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl
> gpio_keys dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc
> intel_telemetry_core rtc_cmos efivars x86_pkg_temp_thermal iwlwifi
> snd_hda_codec_hdmi soc_button_array btusb cfg80211 btrtl mei_me
> hci_uart btbcm mei btintel i915 bluetooth intel_pmc_ipc snd_hda_intel
> spi_pxa2xx_platform snd_hda_codec dwc3_pci snd_hda_core tpm_tis
> tpm_tis_core tpm efivarfs  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe
> Not tainted 4.11.0-rc6-intel-dev-bkc #1  [ 97.566950] Hardware name: Intel
> Corp. Joule DVT3/SDS, BIOS GTPP181A.X64.0143.B30.1701132137
> 01/13/2017  [ 97.577518] task: 8801793a21c0 task.stack:
> 8801793f  [ 97.584162] RIP:
> 0010:blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286  [
> 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX:
>   [ 97.604812] RDX: 880179eaf210 RSI:
> a0131000 RDI: 81e3ea20  [ 97.612821] RBP: 8801793f3c68
> R08: 0006 R09: 005c  [ 97.620847] R10:
>  R11: 0006 R12: a0131000  [
> 97.628855] R13:  R14: 880176e35f48 R15:
> 8801793f3ea8  [ 97.636865] FS: 7f7eeba07700()
> GS:88017fc0() knlGS:  [ 97.645948] CS:
> 0010 DS:  ES:  CR0: 80050033  [ 97.652423] CR2:
> a006f010 CR3: 0001775ef000 CR4: 003406f0  [
> 97.660423] Call Trace:
>  [ 97.663166] ? 0xa0031000
>  [ 97.666885] register_pm_notifier+0x18/0x20  [ 97.671581]
> telemetry_debugfs_init+0x92/0x1000
> 
> Signed-off-by: Priyalee Kushwaha 
> ---
>  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c
> b/drivers/platform/x86/intel_telemetry_debugfs.c
> index ef29f18..0f93975 100644
> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
> #endif /* CONFIG_PM_SLEEP */
> 
>   debugfs_conf->telemetry_dbg_dir =
> debugfs_create_dir("telemetry", NULL);
> - if (!debugfs_conf->telemetry_dbg_dir)
> + if (!debugfs_conf->telemetry_dbg_dir) { #ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>   return -ENOMEM;
> + }
> 
>   f = debugfs_create_file("pss_info", S_IFREG | S_IRUGO,
>   debugfs_conf->telemetry_dbg_dir, NULL,
> @@ -1014,6 +1018,9 @@ static int __init telemetry_debugfs_init(void)
>  out:
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
> 
>   return err;
>  }
> @@ -1022,6 +1029,9 @@ static void __exit telemetry_debugfs_exit(void)
> {
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  }
> 
>  late_initcall(telemetry_debugfs_init);
> --
> 2.10.0

RE: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Chakravarty, Souvik K]

Thanks for catching this.

BR,
Souvik

> -Original Message-
> From: platform-driver-x86-ow...@vger.kernel.org [mailto:platform-driver-
> x86-ow...@vger.kernel.org] On Behalf Of priyalee.kushw...@intel.com
> Sent: Saturday, May 27, 2017 8:48 PM
> To: dvh...@infradead.org; Chakravarty, Souvik K
> ; a...@infradead.org
> Cc: linux-kernel@vger.kernel.org; platform-driver-...@vger.kernel.org;
> Kushwaha, Priyalee 
> Subject: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while
> load/unload module test
> 
> From: Priyalee Kushwaha 
> 
> This fix oops found while testing load/unload test of
> intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> for PM callbacks, but unregister_pm_notifier was missing from
> module_exit.
> 
>  [ 97.481860] BUG: unable to handle kernel paging request at
> a006f010  [ 97.489742] IP:
> blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.495898] PGD 2e0a067
>  [ 97.495899] PUD 2e0b063
>  [ 97.498737] PMD 179e29067
>  [ 97.501573] PTE 0
> 
>  [ 97.508423] Oops:  1 PREEMPT SMP
>  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl
> gpio_keys dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc
> intel_telemetry_core rtc_cmos efivars x86_pkg_temp_thermal iwlwifi
> snd_hda_codec_hdmi soc_button_array btusb cfg80211 btrtl mei_me
> hci_uart btbcm mei btintel i915 bluetooth intel_pmc_ipc snd_hda_intel
> spi_pxa2xx_platform snd_hda_codec dwc3_pci snd_hda_core tpm_tis
> tpm_tis_core tpm efivarfs  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe
> Not tainted 4.11.0-rc6-intel-dev-bkc #1  [ 97.566950] Hardware name: Intel
> Corp. Joule DVT3/SDS, BIOS GTPP181A.X64.0143.B30.1701132137
> 01/13/2017  [ 97.577518] task: 8801793a21c0 task.stack:
> 8801793f  [ 97.584162] RIP:
> 0010:blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286  [
> 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX:
>   [ 97.604812] RDX: 880179eaf210 RSI:
> a0131000 RDI: 81e3ea20  [ 97.612821] RBP: 8801793f3c68
> R08: 0006 R09: 005c  [ 97.620847] R10:
>  R11: 0006 R12: a0131000  [
> 97.628855] R13:  R14: 880176e35f48 R15:
> 8801793f3ea8  [ 97.636865] FS: 7f7eeba07700()
> GS:88017fc0() knlGS:  [ 97.645948] CS:
> 0010 DS:  ES:  CR0: 80050033  [ 97.652423] CR2:
> a006f010 CR3: 0001775ef000 CR4: 003406f0  [
> 97.660423] Call Trace:
>  [ 97.663166] ? 0xa0031000
>  [ 97.666885] register_pm_notifier+0x18/0x20  [ 97.671581]
> telemetry_debugfs_init+0x92/0x1000
> 
> Signed-off-by: Priyalee Kushwaha 
> ---
>  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c
> b/drivers/platform/x86/intel_telemetry_debugfs.c
> index ef29f18..0f93975 100644
> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
> #endif /* CONFIG_PM_SLEEP */
> 
>   debugfs_conf->telemetry_dbg_dir =
> debugfs_create_dir("telemetry", NULL);
> - if (!debugfs_conf->telemetry_dbg_dir)
> + if (!debugfs_conf->telemetry_dbg_dir) { #ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>   return -ENOMEM;
> + }
> 
>   f = debugfs_create_file("pss_info", S_IFREG | S_IRUGO,
>   debugfs_conf->telemetry_dbg_dir, NULL,
> @@ -1014,6 +1018,9 @@ static int __init telemetry_debugfs_init(void)
>  out:
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
> 
>   return err;
>  }
> @@ -1022,6 +1029,9 @@ static void __exit telemetry_debugfs_exit(void)
> {
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  }
> 
>  late_initcall(telemetry_debugfs_init);
> --
> 2.10.0

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Andy Shevchenko]

On Sat, May 27, 2017 at 9:54 AM, Pavel Machek  wrote:
>> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:

>> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
>>CONFIG_PM_SLEEP?
>
> Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds
> like a good idea.

I like the idea, though I would let Rafael to speak up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Andy Shevchenko]

On Sat, May 27, 2017 at 9:54 AM, Pavel Machek  wrote:
>> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:

>> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
>>CONFIG_PM_SLEEP?
>
> Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds
> like a good idea.

I like the idea, though I would let Rafael to speak up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Pavel Machek]

Hi!

> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:
> > From: Priyalee Kushwaha 
> > 
> > This fix oops found while testing load/unload test of
> > intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> > for PM callbacks, but unregister_pm_notifier was missing from
> > module_exit.
> > 
> >  [ 97.481860] BUG: unable to handle kernel paging request at 
> > a006f010
> >  [ 97.489742] IP: blocking_notifier_chain_register+0x3a/0xa0
> >  [ 97.495898] PGD 2e0a067
> >  [ 97.495899] PUD 2e0b063
> >  [ 97.498737] PMD 179e29067
> >  [ 97.501573] PTE 0
> > 
> >  [ 97.508423] Oops:  1 PREEMPT SMP
> >  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl 
> > gpio_keys dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc 
> > intel_telemetry_core rtc_cmos efivars x86_pkg_temp_thermal iwlwifi 
> > snd_hda_codec_hdmi soc_button_array btusb cfg80211 btrtl mei_me hci_uart 
> > btbcm mei btintel i915 bluetooth intel_pmc_ipc snd_hda_intel 
> > spi_pxa2xx_platform snd_hda_codec dwc3_pci snd_hda_core tpm_tis 
> > tpm_tis_core tpm efivarfs
> >  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe Not tainted 
> > 4.11.0-rc6-intel-dev-bkc #1
> >  [ 97.566950] Hardware name: Intel Corp. Joule DVT3/SDS, BIOS 
> > GTPP181A.X64.0143.B30.1701132137 01/13/2017
> >  [ 97.577518] task: 8801793a21c0 task.stack: 8801793f
> >  [ 97.584162] RIP: 0010:blocking_notifier_chain_register+0x3a/0xa0
> >  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286
> >  [ 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX: 
> > 
> >  [ 97.604812] RDX: 880179eaf210 RSI: a0131000 RDI: 
> > 81e3ea20
> >  [ 97.612821] RBP: 8801793f3c68 R08: 0006 R09: 
> > 005c
> >  [ 97.620847] R10:  R11: 0006 R12: 
> > a0131000
> >  [ 97.628855] R13:  R14: 880176e35f48 R15: 
> > 8801793f3ea8
> >  [ 97.636865] FS: 7f7eeba07700() GS:88017fc0() 
> > knlGS:
> >  [ 97.645948] CS: 0010 DS:  ES:  CR0: 80050033
> >  [ 97.652423] CR2: a006f010 CR3: 0001775ef000 CR4: 
> > 003406f0
> >  [ 97.660423] Call Trace:
> >  [ 97.663166] ? 0xa0031000
> >  [ 97.666885] register_pm_notifier+0x18/0x20
> >  [ 97.671581] telemetry_debugfs_init+0x92/0x1000
> > 
> > Signed-off-by: Priyalee Kushwaha 
> 
> Hi Priyalee,
> 
> Thanks for catching this, we should get a fix into the RC cycle. but I think 
> we
> can make some small changes that will improve legibility here.
> 
> > ---
> >  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c 
> > b/drivers/platform/x86/intel_telemetry_debugfs.c
> > index ef29f18..0f93975 100644
> > --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> > +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> > @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > debugfs_conf->telemetry_dbg_dir = debugfs_create_dir("telemetry", NULL);
> > -   if (!debugfs_conf->telemetry_dbg_dir)
> > +   if (!debugfs_conf->telemetry_dbg_dir) {
> > +#ifdef CONFIG_PM_SLEEP
> > +   unregister_pm_notifier(_notifier);
> > +#endif /* CONFIG_PM_SLEEP */
> > return -ENOMEM;
> > +   }
> 
> As a general rule, we try to avoid peppering code with #ifdef blocks, and 
> prefer
> to create no-op functions, or similar. CONFIG_PM_SLEEP unfortunately doesn't
> have such no-op functions.
> 
> Rather than add the CONFIG_PM_SLEEP block above, please convert the above to 
> use
> an err= and goto statement, and create the appropriate labels below.
> 
> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
>CONFIG_PM_SLEEP?

Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds
like a good idea.
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Pavel Machek]

Hi!

> On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:
> > From: Priyalee Kushwaha 
> > 
> > This fix oops found while testing load/unload test of
> > intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> > for PM callbacks, but unregister_pm_notifier was missing from
> > module_exit.
> > 
> >  [ 97.481860] BUG: unable to handle kernel paging request at 
> > a006f010
> >  [ 97.489742] IP: blocking_notifier_chain_register+0x3a/0xa0
> >  [ 97.495898] PGD 2e0a067
> >  [ 97.495899] PUD 2e0b063
> >  [ 97.498737] PMD 179e29067
> >  [ 97.501573] PTE 0
> > 
> >  [ 97.508423] Oops:  1 PREEMPT SMP
> >  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl 
> > gpio_keys dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc 
> > intel_telemetry_core rtc_cmos efivars x86_pkg_temp_thermal iwlwifi 
> > snd_hda_codec_hdmi soc_button_array btusb cfg80211 btrtl mei_me hci_uart 
> > btbcm mei btintel i915 bluetooth intel_pmc_ipc snd_hda_intel 
> > spi_pxa2xx_platform snd_hda_codec dwc3_pci snd_hda_core tpm_tis 
> > tpm_tis_core tpm efivarfs
> >  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe Not tainted 
> > 4.11.0-rc6-intel-dev-bkc #1
> >  [ 97.566950] Hardware name: Intel Corp. Joule DVT3/SDS, BIOS 
> > GTPP181A.X64.0143.B30.1701132137 01/13/2017
> >  [ 97.577518] task: 8801793a21c0 task.stack: 8801793f
> >  [ 97.584162] RIP: 0010:blocking_notifier_chain_register+0x3a/0xa0
> >  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286
> >  [ 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX: 
> > 
> >  [ 97.604812] RDX: 880179eaf210 RSI: a0131000 RDI: 
> > 81e3ea20
> >  [ 97.612821] RBP: 8801793f3c68 R08: 0006 R09: 
> > 005c
> >  [ 97.620847] R10:  R11: 0006 R12: 
> > a0131000
> >  [ 97.628855] R13:  R14: 880176e35f48 R15: 
> > 8801793f3ea8
> >  [ 97.636865] FS: 7f7eeba07700() GS:88017fc0() 
> > knlGS:
> >  [ 97.645948] CS: 0010 DS:  ES:  CR0: 80050033
> >  [ 97.652423] CR2: a006f010 CR3: 0001775ef000 CR4: 
> > 003406f0
> >  [ 97.660423] Call Trace:
> >  [ 97.663166] ? 0xa0031000
> >  [ 97.666885] register_pm_notifier+0x18/0x20
> >  [ 97.671581] telemetry_debugfs_init+0x92/0x1000
> > 
> > Signed-off-by: Priyalee Kushwaha 
> 
> Hi Priyalee,
> 
> Thanks for catching this, we should get a fix into the RC cycle. but I think 
> we
> can make some small changes that will improve legibility here.
> 
> > ---
> >  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c 
> > b/drivers/platform/x86/intel_telemetry_debugfs.c
> > index ef29f18..0f93975 100644
> > --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> > +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> > @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
> >  #endif /* CONFIG_PM_SLEEP */
> >  
> > debugfs_conf->telemetry_dbg_dir = debugfs_create_dir("telemetry", NULL);
> > -   if (!debugfs_conf->telemetry_dbg_dir)
> > +   if (!debugfs_conf->telemetry_dbg_dir) {
> > +#ifdef CONFIG_PM_SLEEP
> > +   unregister_pm_notifier(_notifier);
> > +#endif /* CONFIG_PM_SLEEP */
> > return -ENOMEM;
> > +   }
> 
> As a general rule, we try to avoid peppering code with #ifdef blocks, and 
> prefer
> to create no-op functions, or similar. CONFIG_PM_SLEEP unfortunately doesn't
> have such no-op functions.
> 
> Rather than add the CONFIG_PM_SLEEP block above, please convert the above to 
> use
> an err= and goto statement, and create the appropriate labels below.
> 
> +Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
>CONFIG_PM_SLEEP?

Yeah, empty "unregister_pm_notifier" in !CONFIG_PM_SLEEP case sounds
like a good idea.
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Darren Hart]

On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:
> From: Priyalee Kushwaha 
> 
> This fix oops found while testing load/unload test of
> intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> for PM callbacks, but unregister_pm_notifier was missing from
> module_exit.
> 
>  [ 97.481860] BUG: unable to handle kernel paging request at a006f010
>  [ 97.489742] IP: blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.495898] PGD 2e0a067
>  [ 97.495899] PUD 2e0b063
>  [ 97.498737] PMD 179e29067
>  [ 97.501573] PTE 0
> 
>  [ 97.508423] Oops:  1 PREEMPT SMP
>  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl gpio_keys 
> dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc intel_telemetry_core 
> rtc_cmos efivars x86_pkg_temp_thermal iwlwifi snd_hda_codec_hdmi 
> soc_button_array btusb cfg80211 btrtl mei_me hci_uart btbcm mei btintel i915 
> bluetooth intel_pmc_ipc snd_hda_intel spi_pxa2xx_platform snd_hda_codec 
> dwc3_pci snd_hda_core tpm_tis tpm_tis_core tpm efivarfs
>  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe Not tainted 
> 4.11.0-rc6-intel-dev-bkc #1
>  [ 97.566950] Hardware name: Intel Corp. Joule DVT3/SDS, BIOS 
> GTPP181A.X64.0143.B30.1701132137 01/13/2017
>  [ 97.577518] task: 8801793a21c0 task.stack: 8801793f
>  [ 97.584162] RIP: 0010:blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286
>  [ 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX: 
> 
>  [ 97.604812] RDX: 880179eaf210 RSI: a0131000 RDI: 
> 81e3ea20
>  [ 97.612821] RBP: 8801793f3c68 R08: 0006 R09: 
> 005c
>  [ 97.620847] R10:  R11: 0006 R12: 
> a0131000
>  [ 97.628855] R13:  R14: 880176e35f48 R15: 
> 8801793f3ea8
>  [ 97.636865] FS: 7f7eeba07700() GS:88017fc0() 
> knlGS:
>  [ 97.645948] CS: 0010 DS:  ES:  CR0: 80050033
>  [ 97.652423] CR2: a006f010 CR3: 0001775ef000 CR4: 
> 003406f0
>  [ 97.660423] Call Trace:
>  [ 97.663166] ? 0xa0031000
>  [ 97.666885] register_pm_notifier+0x18/0x20
>  [ 97.671581] telemetry_debugfs_init+0x92/0x1000
> 
> Signed-off-by: Priyalee Kushwaha 

Hi Priyalee,

Thanks for catching this, we should get a fix into the RC cycle. but I think we
can make some small changes that will improve legibility here.

> ---
>  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c 
> b/drivers/platform/x86/intel_telemetry_debugfs.c
> index ef29f18..0f93975 100644
> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
>  #endif /* CONFIG_PM_SLEEP */
>  
>   debugfs_conf->telemetry_dbg_dir = debugfs_create_dir("telemetry", NULL);
> - if (!debugfs_conf->telemetry_dbg_dir)
> + if (!debugfs_conf->telemetry_dbg_dir) {
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>   return -ENOMEM;
> + }

As a general rule, we try to avoid peppering code with #ifdef blocks, and prefer
to create no-op functions, or similar. CONFIG_PM_SLEEP unfortunately doesn't
have such no-op functions.

Rather than add the CONFIG_PM_SLEEP block above, please convert the above to use
an err= and goto statement, and create the appropriate labels below.

+Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
   CONFIG_PM_SLEEP?

>  
>   f = debugfs_create_file("pss_info", S_IFREG | S_IRUGO,
>   debugfs_conf->telemetry_dbg_dir, NULL,
> @@ -1014,6 +1018,9 @@ static int __init telemetry_debugfs_init(void)
>  out:
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;

  e.g.

   out_pm:

> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  
>   return err;
>  }
> @@ -1022,6 +1029,9 @@ static void __exit telemetry_debugfs_exit(void)
>  {
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  }
>  
>  late_initcall(telemetry_debugfs_init);
> -- 
> 2.10.0
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v1 1/1] intel_telemetry_debugfs: fix oops found while load/unload module test

[Darren Hart]

On Sat, May 27, 2017 at 08:17:39AM -0700, priyalee.kushw...@intel.com wrote:
> From: Priyalee Kushwaha 
> 
> This fix oops found while testing load/unload test of
> intel_telemetry_debugfs module. Module_init uses register_pm_notifier
> for PM callbacks, but unregister_pm_notifier was missing from
> module_exit.
> 
>  [ 97.481860] BUG: unable to handle kernel paging request at a006f010
>  [ 97.489742] IP: blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.495898] PGD 2e0a067
>  [ 97.495899] PUD 2e0b063
>  [ 97.498737] PMD 179e29067
>  [ 97.501573] PTE 0
> 
>  [ 97.508423] Oops:  1 PREEMPT SMP
>  [ 97.512724] Modules linked in: intel_telemetry_debugfs intel_rapl gpio_keys 
> dwc3 udc_core intel_telemetry_pltdrv intel_punit_ipc intel_telemetry_core 
> rtc_cmos efivars x86_pkg_temp_thermal iwlwifi snd_hda_codec_hdmi 
> soc_button_array btusb cfg80211 btrtl mei_me hci_uart btbcm mei btintel i915 
> bluetooth intel_pmc_ipc snd_hda_intel spi_pxa2xx_platform snd_hda_codec 
> dwc3_pci snd_hda_core tpm_tis tpm_tis_core tpm efivarfs
>  [ 97.558453] CPU: 0 PID: 889 Comm: modprobe Not tainted 
> 4.11.0-rc6-intel-dev-bkc #1
>  [ 97.566950] Hardware name: Intel Corp. Joule DVT3/SDS, BIOS 
> GTPP181A.X64.0143.B30.1701132137 01/13/2017
>  [ 97.577518] task: 8801793a21c0 task.stack: 8801793f
>  [ 97.584162] RIP: 0010:blocking_notifier_chain_register+0x3a/0xa0
>  [ 97.590903] RSP: 0018:8801793f3c58 EFLAGS: 00010286
>  [ 97.596802] RAX: a006f000 RBX: 81e3ea20 RCX: 
> 
>  [ 97.604812] RDX: 880179eaf210 RSI: a0131000 RDI: 
> 81e3ea20
>  [ 97.612821] RBP: 8801793f3c68 R08: 0006 R09: 
> 005c
>  [ 97.620847] R10:  R11: 0006 R12: 
> a0131000
>  [ 97.628855] R13:  R14: 880176e35f48 R15: 
> 8801793f3ea8
>  [ 97.636865] FS: 7f7eeba07700() GS:88017fc0() 
> knlGS:
>  [ 97.645948] CS: 0010 DS:  ES:  CR0: 80050033
>  [ 97.652423] CR2: a006f010 CR3: 0001775ef000 CR4: 
> 003406f0
>  [ 97.660423] Call Trace:
>  [ 97.663166] ? 0xa0031000
>  [ 97.666885] register_pm_notifier+0x18/0x20
>  [ 97.671581] telemetry_debugfs_init+0x92/0x1000
> 
> Signed-off-by: Priyalee Kushwaha 

Hi Priyalee,

Thanks for catching this, we should get a fix into the RC cycle. but I think we
can make some small changes that will improve legibility here.

> ---
>  drivers/platform/x86/intel_telemetry_debugfs.c | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c 
> b/drivers/platform/x86/intel_telemetry_debugfs.c
> index ef29f18..0f93975 100644
> --- a/drivers/platform/x86/intel_telemetry_debugfs.c
> +++ b/drivers/platform/x86/intel_telemetry_debugfs.c
> @@ -966,8 +966,12 @@ static int __init telemetry_debugfs_init(void)
>  #endif /* CONFIG_PM_SLEEP */
>  
>   debugfs_conf->telemetry_dbg_dir = debugfs_create_dir("telemetry", NULL);
> - if (!debugfs_conf->telemetry_dbg_dir)
> + if (!debugfs_conf->telemetry_dbg_dir) {
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>   return -ENOMEM;
> + }

As a general rule, we try to avoid peppering code with #ifdef blocks, and prefer
to create no-op functions, or similar. CONFIG_PM_SLEEP unfortunately doesn't
have such no-op functions.

Rather than add the CONFIG_PM_SLEEP block above, please convert the above to use
an err= and goto statement, and create the appropriate labels below.

+Rafael, Len, Pavel, linux-pm: Is there a preferred approach for dealing with
   CONFIG_PM_SLEEP?

>  
>   f = debugfs_create_file("pss_info", S_IFREG | S_IRUGO,
>   debugfs_conf->telemetry_dbg_dir, NULL,
> @@ -1014,6 +1018,9 @@ static int __init telemetry_debugfs_init(void)
>  out:
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;

  e.g.

   out_pm:

> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  
>   return err;
>  }
> @@ -1022,6 +1029,9 @@ static void __exit telemetry_debugfs_exit(void)
>  {
>   debugfs_remove_recursive(debugfs_conf->telemetry_dbg_dir);
>   debugfs_conf->telemetry_dbg_dir = NULL;
> +#ifdef CONFIG_PM_SLEEP
> + unregister_pm_notifier(_notifier);
> +#endif /* CONFIG_PM_SLEEP */
>  }
>  
>  late_initcall(telemetry_debugfs_init);
> -- 
> 2.10.0
> 
> 

-- 
Darren Hart
VMware Open Source Technology Center