| Message ID | 20190405203558.19160-2-rajatja@google.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
| Series | [v3,1/3] platform/x86: intel_pmc_core: Convert to a platform_driver | expand |
On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > Add a module parameter which when enabled, will check on resume, if the > last S0ix attempt was successful. If not, the driver would warn and provide > helpful debug information (which gets latched during the failed suspend > attempt) to debug the S0ix failure. > > This information is very useful to debug S0ix failures. Specially since > the latched debug information will be lost (over-written) if the system > attempts to go into runtime (or imminent) S0ix again after that failed > suspend attempt. > +#ifdef CONFIG_PM_SLEEP > + > +static bool warn_on_s0ix_failures; > +module_param(warn_on_s0ix_failures, bool, 0644); > +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures"); > + > +static int pmc_core_suspend(struct device *dev) > +{ > + struct pmc_dev *pmcdev = dev_get_drvdata(dev); > + > + /* Save PC10 and S0ix residency for checking later */ > + if (warn_on_s0ix_failures && !pm_suspend_via_firmware() && > + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) && > + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) > + pmcdev->check_counters = true; Perhaps something like pmcdev->check_counters = false; /* User doesn't want to be warned */ if (!warn_on...) return 0; /* We do suspend via firmware */ if (...) return 0; ... ? > + else > + pmcdev->check_counters = false; > + > + return 0; > +} > + > +static inline bool pc10_failed(struct pmc_dev *pmcdev) To be or not to be? :-) Perhaps names of the functions should be pmc_code_is_pc10_failed() and so on > +{ > + u64 pc10_counter; > + > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) && > + pc10_counter == pmcdev->pc10_counter) > + return true; > + else Redundant. > + return false; > +} > + > +static inline bool s0ix_failed(struct pmc_dev *pmcdev) > +{ > + u64 s0ix_counter; > + > + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) && > + s0ix_counter == pmcdev->s0ix_counter) > + return true; > + else Ditto. > + return false; > +} > + > +static int pmc_core_resume(struct device *dev) > +{ > + struct pmc_dev *pmcdev = dev_get_drvdata(dev); > + > + if (!pmcdev->check_counters) > + return 0; > + > + if (pc10_failed(pmcdev)) { > + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n", > + pmcdev->pc10_counter); > + } else if (s0ix_failed(pmcdev)) { > + > + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps; > + const struct pmc_bit_map *map; > + int offset = pmcdev->map->slps0_dbg_offset; > + u32 data; > + > + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n", > + pmcdev->s0ix_counter); > + while (*maps) { > + map = *maps; > + data = pmc_core_reg_read(pmcdev, offset); > + offset += 4; > + while (map->name) { > + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n", > + map->name, > + data & map->bit_mask ? "Yes" : "No"); > + ++map; > + } > + ++maps; > + } Can't we utilize existing print helpers? > + } > + return 0; > +} > + > +#endif
On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > > > Add a module parameter which when enabled, will check on resume, if the > > last S0ix attempt was successful. If not, the driver would warn and provide > > helpful debug information (which gets latched during the failed suspend > > attempt) to debug the S0ix failure. > > > > This information is very useful to debug S0ix failures. Specially since > > the latched debug information will be lost (over-written) if the system > > attempts to go into runtime (or imminent) S0ix again after that failed > > suspend attempt. > > > +#ifdef CONFIG_PM_SLEEP > > + > > +static bool warn_on_s0ix_failures; > > +module_param(warn_on_s0ix_failures, bool, 0644); > > +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures"); > > + > > +static int pmc_core_suspend(struct device *dev) > > +{ > > + struct pmc_dev *pmcdev = dev_get_drvdata(dev); > > + > > + /* Save PC10 and S0ix residency for checking later */ > > > + if (warn_on_s0ix_failures && !pm_suspend_via_firmware() && > > + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) && > > + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) > > + pmcdev->check_counters = true; > > Perhaps something like > > pmcdev->check_counters = false; > /* User doesn't want to be warned */ > if (!warn_on...) > return 0; > /* We do suspend via firmware */ > if (...) > return 0; > ... > > ? I guess what you mean is one conditional per line. Sure, I will do that. > > > + else > > + pmcdev->check_counters = false; > > + > > + return 0; > > +} > > + > > +static inline bool pc10_failed(struct pmc_dev *pmcdev) > > To be or not to be? :-) > Perhaps names of the functions should be > > pmc_code_is_pc10_failed() > > and so on I think the suggestion is to use pmc_core_* as the function names. OK, I will rename the functions to: pmc_core_pc10_failed() and pmc_core_s0ix_failed() > > > +{ > > + u64 pc10_counter; > > + > > + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) && > > + pc10_counter == pmcdev->pc10_counter) > > + return true; > > > + else > > Redundant. OK, I'll remove the "else" part here. > > > + return false; > > +} > > + > > +static inline bool s0ix_failed(struct pmc_dev *pmcdev) > > +{ > > + u64 s0ix_counter; > > + > > + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) && > > + s0ix_counter == pmcdev->s0ix_counter) > > + return true; > > > + else > > Ditto. OK, I'll remove the "else" part here. > > > + return false; > > +} > > + > > +static int pmc_core_resume(struct device *dev) > > +{ > > + struct pmc_dev *pmcdev = dev_get_drvdata(dev); > > + > > + if (!pmcdev->check_counters) > > + return 0; > > + > > + if (pc10_failed(pmcdev)) { > > + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n", > > + pmcdev->pc10_counter); > > + } else if (s0ix_failed(pmcdev)) { > > + > > + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps; > > + const struct pmc_bit_map *map; > > + int offset = pmcdev->map->slps0_dbg_offset; > > + u32 data; > > + > > + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n", > > + pmcdev->s0ix_counter); > > + while (*maps) { > > + map = *maps; > > + data = pmc_core_reg_read(pmcdev, offset); > > + offset += 4; > > + while (map->name) { > > + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n", > > + map->name, > > + data & map->bit_mask ? "Yes" : "No"); > > + ++map; > > + } > > + ++maps; > > + } > > Can't we utilize existing print helpers? I didn't quite see any existing print helpers in this file. I took this code from pmc_core_slps0_dbg_show(), and I think although I can abstract out this code into a static function, the calling code need to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so might be overkill (did not feel that the benefits were worth it). Please let me know if you have any suggestions and will be happy to use them. Thanks, Rajat > > > + } > > + return 0; > > +} > > + > > +#endif > > -- > With Best Regards, > Andy Shevchenko
On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <rajatja@google.com> wrote: > On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > Perhaps something like > > > > pmcdev->check_counters = false; > > /* User doesn't want to be warned */ > > if (!warn_on...) > > return 0; > > /* We do suspend via firmware */ > > if (...) > > return 0; > > ... > > > > ? > > I guess what you mean is one conditional per line. Sure, I will do that. Yes > > > +static inline bool pc10_failed(struct pmc_dev *pmcdev) > > > > To be or not to be? :-) > > Perhaps names of the functions should be > > > > pmc_code_is_pc10_failed() > > > > and so on > > I think the suggestion is to use pmc_core_* as the function names. OK, > I will rename the functions to: > > pmc_core_pc10_failed() > and > pmc_core_s0ix_failed() And verb "to be". See above. > > Can't we utilize existing print helpers? > > I didn't quite see any existing print helpers in this file. I took > this code from pmc_core_slps0_dbg_show(), and I think although I can > abstract out this code into a static function, the calling code need > to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so > might be overkill (did not feel that the benefits were worth it). > Please let me know if you have any suggestions and will be happy to > use them. Instead of adding module parameter and doing these prints, perhaps introduce another debugfs node.
On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <rajatja@google.com> wrote: > > On Mon, Apr 8, 2019 at 10:02 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Apr 5, 2019 at 11:36 PM Rajat Jain <rajatja@google.com> wrote: > > > > Perhaps something like > > > > > > pmcdev->check_counters = false; > > > /* User doesn't want to be warned */ > > > if (!warn_on...) > > > return 0; > > > /* We do suspend via firmware */ > > > if (...) > > > return 0; > > > ... > > > > > > ? > > > > I guess what you mean is one conditional per line. Sure, I will do that. > > Yes > > > > > +static inline bool pc10_failed(struct pmc_dev *pmcdev) > > > > > > To be or not to be? :-) > > > Perhaps names of the functions should be > > > > > > pmc_code_is_pc10_failed() > > > > > > and so on > > > > I think the suggestion is to use pmc_core_* as the function names. OK, > > I will rename the functions to: > > > > pmc_core_pc10_failed() > > and > > pmc_core_s0ix_failed() > > And verb "to be". See above. > > > > Can't we utilize existing print helpers? > > > > I didn't quite see any existing print helpers in this file. I took > > this code from pmc_core_slps0_dbg_show(), and I think although I can > > abstract out this code into a static function, the calling code need > > to use seq_printf(s,...) and dev_warn(dev,...) respectively. - so > > might be overkill (did not feel that the benefits were worth it). > > Please let me know if you have any suggestions and will be happy to > > use them. > > Instead of adding module parameter and doing these prints, perhaps > introduce another debugfs node. Uh, I actually did wanted to print them at the resume time in kernel logs, because I think this is something kernel developers would be responsible for debugging and thus would be great to have this included within the kernel logs. User space tools may differ on different distros and may or may not be looking for S0ix failures, and particularly may not dump this new debugfs attribute. The other thing is that seemingly this could also help in situations where debugfs is not configured? Thanks, Rajat > > -- > With Best Regards, > Andy Shevchenko
On Mon, Apr 8, 2019 at 9:58 PM Rajat Jain <rajatja@google.com> wrote: > On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <rajatja@google.com> wrote: > > Instead of adding module parameter and doing these prints, perhaps > > introduce another debugfs node. > > Uh, I actually did wanted to print them at the resume time in kernel > logs, because I think this is something kernel developers would be > responsible for debugging and thus would be great to have this > included within the kernel logs. User space tools may differ on > different distros and may or may not be looking for S0ix failures, and > particularly may not dump this new debugfs attribute. > > The other thing is that seemingly this could also help in situations > where debugfs is not configured? The keywords above "developers", "debugging", so, the concept of this driver is almost all about debugging, debugfs is not for users or esp. user space tools. So, I don't see any issue here with creating another node and use it for *debugging* whenever it's needed.
On Mon, Apr 8, 2019 at 12:34 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 8, 2019 at 9:58 PM Rajat Jain <rajatja@google.com> wrote: > > On Mon, Apr 8, 2019 at 11:41 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Mon, Apr 8, 2019 at 9:36 PM Rajat Jain <rajatja@google.com> wrote: > > > > Instead of adding module parameter and doing these prints, perhaps > > > introduce another debugfs node. > > > > Uh, I actually did wanted to print them at the resume time in kernel > > logs, because I think this is something kernel developers would be > > responsible for debugging and thus would be great to have this > > included within the kernel logs. User space tools may differ on > > different distros and may or may not be looking for S0ix failures, and > > particularly may not dump this new debugfs attribute. > > > > The other thing is that seemingly this could also help in situations > > where debugfs is not configured? > > The keywords above "developers", "debugging", so, the concept of this > driver is almost all about debugging, debugfs is not for users or esp. > user space tools. > So, I don't see any issue here with creating another node and use it > for *debugging* whenever it's needed. Hi Andy, In the past, we did see random suspend (S0ix) failures after the devices were released to users (S0ix failures reasons can be a lot so was not possible to catch all in lab testing). Rajneesh (@Intel) is aware of the context. In most of these cases, it was very difficult to reproduce the issue in developer environment, so really difficult to arrive at what component was causing those failures without these logs. So I'd still like to make one last request to allow to these debugs in the kernel at resume time, to make this feature very useful (it still doesn't change anything for anyone not enabling the module parameter). However if you still have a strong opinion against this, I'll add it as a new debugfs attribute in the new re-spin of the patch. Thanks & Best Regards, Rajat > > -- > With Best Regards, > Andy Shevchenko
diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c index 331889a57f73..d9561a1c620d 100644 --- a/drivers/platform/x86/intel_pmc_core.c +++ b/drivers/platform/x86/intel_pmc_core.c @@ -20,6 +20,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/platform_device.h> +#include <linux/suspend.h> #include <linux/uaccess.h> #include <asm/cpu_device_id.h> @@ -928,6 +929,90 @@ static int pmc_core_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_SLEEP + +static bool warn_on_s0ix_failures; +module_param(warn_on_s0ix_failures, bool, 0644); +MODULE_PARM_DESC(warn_on_s0ix_failures, "Check and warn for S0ix failures"); + +static int pmc_core_suspend(struct device *dev) +{ + struct pmc_dev *pmcdev = dev_get_drvdata(dev); + + /* Save PC10 and S0ix residency for checking later */ + if (warn_on_s0ix_failures && !pm_suspend_via_firmware() && + !rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) && + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter)) + pmcdev->check_counters = true; + else + pmcdev->check_counters = false; + + return 0; +} + +static inline bool pc10_failed(struct pmc_dev *pmcdev) +{ + u64 pc10_counter; + + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pc10_counter) && + pc10_counter == pmcdev->pc10_counter) + return true; + else + return false; +} + +static inline bool s0ix_failed(struct pmc_dev *pmcdev) +{ + u64 s0ix_counter; + + if (!pmc_core_dev_state_get(pmcdev, &s0ix_counter) && + s0ix_counter == pmcdev->s0ix_counter) + return true; + else + return false; +} + +static int pmc_core_resume(struct device *dev) +{ + struct pmc_dev *pmcdev = dev_get_drvdata(dev); + + if (!pmcdev->check_counters) + return 0; + + if (pc10_failed(pmcdev)) { + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n", + pmcdev->pc10_counter); + } else if (s0ix_failed(pmcdev)) { + + const struct pmc_bit_map **maps = pmcdev->map->slps0_dbg_maps; + const struct pmc_bit_map *map; + int offset = pmcdev->map->slps0_dbg_offset; + u32 data; + + dev_warn(dev, "S0ix entry had failed (S0ix cnt=%llu)\n", + pmcdev->s0ix_counter); + while (*maps) { + map = *maps; + data = pmc_core_reg_read(pmcdev, offset); + offset += 4; + while (map->name) { + dev_warn(dev, "SLP_S0_DBG: %-32s\tState: %s\n", + map->name, + data & map->bit_mask ? "Yes" : "No"); + ++map; + } + ++maps; + } + } + return 0; +} + +#endif + +static const struct dev_pm_ops pmc_core_pm_ops = { + SET_LATE_SYSTEM_SLEEP_PM_OPS(pmc_core_suspend, pmc_core_resume) +}; + static const struct acpi_device_id pmc_core_acpi_ids[] = { {"INT33A1", 0}, /* _HID for Intel Power Engine, _CID PNP0D80*/ { } @@ -938,6 +1023,7 @@ static struct platform_driver pmc_core_driver = { .driver = { .name = "pmc_core", .acpi_match_table = ACPI_PTR(pmc_core_acpi_ids), + .pm = &pmc_core_pm_ops, }, .probe = pmc_core_probe, .remove = pmc_core_remove, diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h index 88d9c0653a5f..fdee5772e532 100644 --- a/drivers/platform/x86/intel_pmc_core.h +++ b/drivers/platform/x86/intel_pmc_core.h @@ -241,6 +241,9 @@ struct pmc_reg_map { * @pmc_xram_read_bit: flag to indicate whether PMC XRAM shadow registers * used to read MPHY PG and PLL status are available * @mutex_lock: mutex to complete one transcation + * @check_counters: On resume, check if counters are getting incremented + * @pc10_counter: PC10 residency counter + * @s0ix_counter: S0ix residency (step adjusted) * * pmc_dev contains info about power management controller device. */ @@ -253,6 +256,10 @@ struct pmc_dev { #endif /* CONFIG_DEBUG_FS */ int pmc_xram_read_bit; struct mutex lock; /* generic mutex lock for PMC Core */ + + bool check_counters; /* Check for counter increments on resume */ + u64 pc10_counter; + u64 s0ix_counter; }; #endif /* PMC_CORE_H */
Add a module parameter which when enabled, will check on resume, if the last S0ix attempt was successful. If not, the driver would warn and provide helpful debug information (which gets latched during the failed suspend attempt) to debug the S0ix failure. This information is very useful to debug S0ix failures. Specially since the latched debug information will be lost (over-written) if the system attempts to go into runtime (or imminent) S0ix again after that failed suspend attempt. Signed-off-by: Rajat Jain <rajatja@google.com> --- v3: No changes v2: Use pm_suspend_via_firmware() to enable the check only for S0ix drivers/platform/x86/intel_pmc_core.c | 86 +++++++++++++++++++++++++++ drivers/platform/x86/intel_pmc_core.h | 7 +++ 2 files changed, 93 insertions(+)