Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle constraints for debug only

[Rafael J. Wysocki]

On Friday, August 11, 2017 6:18:50 PM CEST Srinivas Pandruvada wrote:
> On Fri, 2017-08-11 at 14:43 +0000, Mario.Limonciello@xxxxxxxx wrote:
> > > 
> > > -----Original Message-----
> > > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@xxxxxxxxxxxxx
> > > om]
> > > Sent: Thursday, August 10, 2017 5:54 PM
> > > To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>; rjw@rjwysocki.
> > > net;
> > > lenb@xxxxxxxxxx
> > > Cc: linux-pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> > > acpi@xxxxxxxxxxxxxxx; lukas@xxxxxxxxx
> > > Subject: Re: [PATCH v2 2/2] ACPI / Sleep: Check low power idle
> > > constraints for
> > > debug only
> > > 
> > > On Thu, 2017-08-10 at 22:07 +0000, Mario.Limonciello@xxxxxxxx
> > > wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > [...]
> > > 
> > > > 
> > > > > 
> > > > > +
> > > > > +		ret = acpi_device_get_power(adev, &state);
> > > > > +		if (!ret)
> > > > > +			pr_debug("LPI: %s required min power
> > > > > state
> > > > > %d, current
> > > > > power state %d, real power state %d\n",
> > > > > +				 lpi_constraints_table[i].name
> > > > > ,
> > > > > +				 lpi_constraints_table[i].min_
> > > > > dsta
> > > > > te,
> > > > > +				 adev->power.state, state);
> > > > Isn't this superfluous to be showing the state returned from
> > > > acpi_device_get_power and
> > > > also probing directly at the state? You can't just rely on the
> > > > information you got
> > > > back from apci_device_get_power?
> > > They can be different as one is real power state and the other is
> > > what
> > > was set.
> > > For example on Dell 9365 it shows
> > > 
> > > [ 1924.393653] LPI: \_SB.PCI0.XHC required min power state 3,
> > > current
> > > power state 3, real power state 255
> > > 
> > Isn't 255 ACPI_STATE_UNKNOWN?  That makes it seem like it
> > is a logic problem in acpi_device_get_power (or somewhere down the
> > chain)
> > doesn't it?
> There is no _PSC for XHC device. So it will return unknown. This is an
> optional object, so I think that dumping the status is fine, but
> matching with output of acpi_device_get_power() as it relies on _PSC is
> not correct for the constraint.

Right.  Moreover, acpi_device_get_power() is basically mostly intended for
initialization, so the constraint should be matched against power.state,
as that's the current ACPI power state of the device as far as the kernel
can say.

Thanks,
Rafael