Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On Fri, Sep 28, 2018 at 02:44:06PM +0530, Bhardwaj, Rajneesh wrote: Resending it since previous message had few HTML contents so it was not delivered to the list. > > > On 26-Sep-18 10:48 PM, Andy Shevchenko wrote: > >On Wed, Sep 26, 2018 at 5:24 PM Bhardwaj, Rajneesh > > wrote: > >>On 26-Sep-18 7:26 PM, Andy Shevchenko wrote: > >>>On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj > >>> wrote: > not be obtained and result in a invalid telemetry_plt_config. > >>>What is telemetry_plt_config? > >>Internal data structure that holds platform config, maintained by the > >>telemetry platform driver. > >You need to spell if for the reader. > > Sure, thanks for the suggestion. I will do it if you agree to my explanation > below and require v2. > > > > This is also applicable to the platforms where the BIOS supports IPC1 > device under debug configurations but IPC1 is disabled by user or the > policy. > > This change allows user to know the reason for not seeing entries under > /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. > +exit: > + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); > >>>Completely useless. > >>> > >>>Device core does it in generic way. > >>If i remove this print then perhaps there is no need of this patch. > >Maybe. > > > > > > > >>Reason to print this is that the platform driver / core driver does not > >>show any error. > >If the code fails and returns 0 — it's a bug in error reporting inside the > >code. > Below are my comments as per previous mail. > The existing Telemetry ecosystem for Atom consists of IPC driver, telem core > driver and telem platform driver but there are no consumers of the APIs > except the ones in telem debugfs driver. Here in this case, not all drivers > that make up the telemetry infra return failure. Here is how the system > looks w/wo IPC1 setting in the BIOS. > > When IPC is present > > root@raj-glk:~# lsmod | grep -i telem > > intel_telemetry_debugfs245760 > > intel_telemetry_pltdrv204800 > > intel_punit_ipc163841 intel_telemetry_pltdrv > > intel_telemetry_core163842 intel_telemetry_debugfs,intel_telemetry_pltdrv > > intel_pmc_ipc204802 intel_telemetry_debugfs,intel_telemetry_pltdrv > > When IPC is missing > > root@raj-glk:~# lsmod | grep -i telem > > intel_telemetry_pltdrv204800 > > intel_punit_ipc163841 intel_telemetry_pltdrv > > intel_telemetry_core163841 intel_telemetry_pltdrv > > intel_pmc_ipc204801 intel_telemetry_pltdrv > > > If we look at the dmesg log, we see "intel_telemetry_core Init". So one > might think that the driver has loaded fine since user may not know that > telemetry is more than just one driver. Such things are often reported by > users of this driver. > So IMHO, keeping this error helps user triage the problem easily. I can > change to pr_info you'd like it. > > > > >>In-fact they are even loaded in module table. OTOH, this > >>debugfs interface fails. This is very confusing to the users if they > >>check the lsmod output so i feel this print might help. > >Again, device core *already has* this and even more (it prints also a > >return code!). > > > -- Best Regards, Rajneesh
Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On Wed, Sep 26, 2018 at 5:24 PM Bhardwaj, Rajneesh wrote: > On 26-Sep-18 7:26 PM, Andy Shevchenko wrote: > > On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj > > wrote: > >> not be obtained and result in a invalid telemetry_plt_config. > > What is telemetry_plt_config? > > Internal data structure that holds platform config, maintained by the > telemetry platform driver. You need to spell if for the reader. > >> This is also applicable to the platforms where the BIOS supports IPC1 > >> device under debug configurations but IPC1 is disabled by user or the > >> policy. > >> > >> This change allows user to know the reason for not seeing entries under > >> /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. > >> +exit: > >> + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); > > Completely useless. > > > > Device core does it in generic way. > > If i remove this print then perhaps there is no need of this patch. Maybe. > Reason to print this is that the platform driver / core driver does not > show any error. If the code fails and returns 0 — it's a bug in error reporting inside the code. > In-fact they are even loaded in module table. OTOH, this > debugfs interface fails. This is very confusing to the users if they > check the lsmod output so i feel this print might help. Again, device core *already has* this and even more (it prints also a return code!). -- With Best Regards, Andy Shevchenko
Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On 26-Sep-18 7:26 PM, Andy Shevchenko wrote: On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: On some Goldmont based systems such as ASRock J3455M the BIOS may not enable the IPC1 device that provides access to the PMC and PUNIT. In such scenarios, the ioss and pss resources from the platform device can IOSS PSS Fine. not be obtained and result in a invalid telemetry_plt_config. What is telemetry_plt_config? Internal data structure that holds platform config, maintained by the telemetry platform driver. This is also applicable to the platforms where the BIOS supports IPC1 device under debug configurations but IPC1 is disabled by user or the policy. This change allows user to know the reason for not seeing entries under /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. Cc: Matt Turner Cc: Len Brown Cc: Souvik Kumar Chakravarty Cc: Kuppuswamy Sathyanarayanan Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 There should be not a blank line. OK. Acked-by: Matt Turner Signed-off-by: Rajneesh Bhardwaj +exit: + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); Completely useless. Device core does it in generic way. If i remove this print then perhaps there is no need of this patch. Reason to print this is that the platform driver / core driver does not show any error. In-fact they are even loaded in module table. OTOH, this debugfs interface fails. This is very confusing to the users if they check the lsmod output so i feel this print might help.
Re: [PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On Mon, Sep 3, 2018 at 9:05 PM Rajneesh Bhardwaj wrote: > > On some Goldmont based systems such as ASRock J3455M the BIOS may not > enable the IPC1 device that provides access to the PMC and PUNIT. In > such scenarios, the ioss and pss resources from the platform device can IOSS PSS > not be obtained and result in a invalid telemetry_plt_config. What is telemetry_plt_config? > This is also applicable to the platforms where the BIOS supports IPC1 > device under debug configurations but IPC1 is disabled by user or the > policy. > > This change allows user to know the reason for not seeing entries under > /sys/kernel/debug/telemetry/* when there is no apparent failure at boot. > > Cc: Matt Turner > Cc: Len Brown > Cc: Souvik Kumar Chakravarty > Cc: Kuppuswamy Sathyanarayanan > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779 > There should be not a blank line. > Acked-by: Matt Turner > Signed-off-by: Rajneesh Bhardwaj > +exit: > + pr_debug(pr_fmt(DRIVER_NAME) " Failed\n"); Completely useless. Device core does it in generic way. -- With Best Regards, Andy Shevchenko
[PATCH 4/4] platform/x86: intel_telemetry: report debugfs failure
On some Goldmont based systems such as ASRock J3455M the BIOS may not
enable the IPC1 device that provides access to the PMC and PUNIT. In
such scenarios, the ioss and pss resources from the platform device can
not be obtained and result in a invalid telemetry_plt_config.
This is also applicable to the platforms where the BIOS supports IPC1
device under debug configurations but IPC1 is disabled by user or the
policy.
This change allows user to know the reason for not seeing entries under
/sys/kernel/debug/telemetry/* when there is no apparent failure at boot.
Cc: Matt Turner
Cc: Len Brown
Cc: Souvik Kumar Chakravarty
Cc: Kuppuswamy Sathyanarayanan
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=198779
Acked-by: Matt Turner
Signed-off-by: Rajneesh Bhardwaj
---
drivers/platform/x86/intel_telemetry_debugfs.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c
b/drivers/platform/x86/intel_telemetry_debugfs.c
index ffd0474b0531..77212e9b22d6 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -951,12 +951,16 @@ static int __init telemetry_debugfs_init(void)
debugfs_conf = (struct telemetry_debugfs_conf *)id->driver_data;
err = telemetry_pltconfig_valid();
- if (err < 0)
- return -ENODEV;
+ if (err < 0) {
+ pr_debug("Invalid pltconfig, ensure IPC1 device is enabled in
BIOS\n");
+ goto exit;
+ }
err = telemetry_debugfs_check_evts();
- if (err < 0)
- return -EINVAL;
+ if (err < 0) {
+ pr_debug("telemetry_debugfs_check_evts failed\n");
+ goto exit;
+ }
register_pm_notifier(&pm_notifier);
@@ -1020,6 +1024,8 @@ static int __init telemetry_debugfs_init(void)
debugfs_conf->telemetry_dbg_dir = NULL;
out_pm:
unregister_pm_notifier(&pm_notifier);
+exit:
+ pr_debug(pr_fmt(DRIVER_NAME) " Failed\n");
return err;
}
--
2.17.1
