Re: [PATCH v1] platform/x86: intel_pmc_core: promote S0ix failure warn() to WARN()

[David E. Box]

On Thu, 2022-10-27 at 17:40 +0200, Hans de Goede wrote:
> Hi,
> 
> On 10/27/22 17:19, Sven van Ashbrook wrote:
> > The "failure to enter S0ix" warning is critically important for monitoring
> > and debugging power regressions, both in the field and in the test lab.
> > 
> > Promote from lower-case warn() to upper-case WARN() so that it becomes
> > more prominent, and gets picked up as part of existing monitoring
> > infrastructure, which typically focuses on WARN() and ignores warn()
> > type log messages.
> > 
> > Signed-off-by: Sven van Ashbrook <svenva@xxxxxxxxxxxx>
> 
> WARN() is really only intended for internal kernel bugs and not for
> hw misbehaving, so I'm not a fan of the change you are suggesting here.
> 
> Intel folks, do you have an opinion on this ?

I agree that not entering s0ix is a critical failure, but this is a hardware
suspend failure. How we treat this should be akin to how we treat failure to
enter S3 or deeper. S0ix support is indicated by the S0 Low Power Idle bit in
the ACPI FADT table. It's better IMO to create some framework in the suspend or
ACPI core that allows platforms to report whether they have achieved the
hardware state indicated by having this bit set. Rafael, your thoughts?

David

> 
> Regards,
> 
> Hans
> 
> 
> > ---
> > Against v6.1-rc2
> > 
> >  drivers/platform/x86/intel/pmc/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/intel/pmc/core.c
> > b/drivers/platform/x86/intel/pmc/core.c
> > index a1fe1e0dcf4a5..834f0352c0edf 100644
> > --- a/drivers/platform/x86/intel/pmc/core.c
> > +++ b/drivers/platform/x86/intel/pmc/core.c
> > @@ -2125,7 +2125,7 @@ static __maybe_unused int pmc_core_resume(struct
> > device *dev)
> >         }
> >  
> >         /* The real interesting case - S0ix failed - lets ask PMC why. */
> > -       dev_warn(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
> > +       dev_WARN(dev, "CPU did not enter SLP_S0!!! (S0ix cnt=%llu)\n",
> >                  pmcdev->s0ix_counter);
> >         if (pmcdev->map->slps0_dbg_maps)
> >                 pmc_core_slps0_display(pmcdev, dev, NULL);
>