On Thu, Apr 11, 2019 at 3:38 AM Rajat Jain <rajatja@xxxxxxxxxx> 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.
> +static int pmc_core_suspend(struct device *dev)
> +{
> + struct pmc_dev *pmcdev = dev_get_drvdata(dev);
> +
> + pmcdev->check_counters = false;
> +
> + /* No warnings on S0ix failures */
> + if (!warn_on_s0ix_failures)
> + return 0;
> +
> + /* Check if the syspend will actually use S0ix */
> + if (pm_suspend_via_firmware())
> + return 0;
> +
> + /* Save PC10 and S0ix residency for checking later */
> + if (!rdmsrl_safe(MSR_PKG_C10_RESIDENCY, &pmcdev->pc10_counter) &&
> + !pmc_core_dev_state_get(pmcdev, &pmcdev->s0ix_counter))
Split it.
> + pmcdev->check_counters = true;
> +
> + return 0;
> +}
> +
> +static inline bool pmc_core_is_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;
Split this as well.
> +
> + return false;
> +}
> +
> +static inline bool pmc_core_is_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;
And this.
> +
> + 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 (pmc_core_is_pc10_failed(pmcdev)) {
> + dev_info(dev, "PC10 entry had failed (PC10 cnt=0x%llx)\n",
> + pmcdev->pc10_counter);
> + } else if (pmc_core_is_s0ix_failed(pmcdev)) {
> +
Redundant.
> + 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;
map++;
> + }
> + ++maps;
maps++;
> + }
This is quite noisy. You need to print only what is important. I don't
think polluting dmesg with piles of these kind of messages is a good
idea.
Also, it is more likely should be done on debug level (except may be
one or two messages with really important information).
> + }
> + return 0;
> +}
> +
> +#endif
--
With Best Regards,
Andy Shevchenko
