Re: [PATCH 1/1] x86/platform/intel-mid: Retrofit pci_platform_pm_ops ->get_power hook

[Lukas Wunner <lukas@wunner.de>]

Hi Andy,

thanks a lot for testing the patch!

On Fri, Oct 07, 2016 at 11:55:34PM +0300, Andy Shevchenko wrote:
> On Thu, 2016-10-06 at 08:24 +0200, Lukas Wunner wrote:
> > +pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> > +{
> > +	struct mid_pwr *pwr = midpwr;
> > +	int id, reg, bit;
> > +	u32 power;
> > +
> > +	if (!pwr || !pwr->available)
> > +		return PCI_UNKNOWN;
> 
> I'm not sure it's a right value. In arch/x86/pci/intel_mid_pci.c we
> assign D3hot to PMCSR for all devices. I dunno how to proceed here,
> though your case works for me.

Generally returning PCI_UNKNOWN from the ->get_state hook is fine
if the platform fails to determine the state.  The ACPI equivalent
acpi_pci_get_power_state() does this as well.

> 
> > +
> > +	id = intel_mid_pwr_get_lss_id(pdev);
> > +	if (id < 0)
> > +		return PCI_UNKNOWN;
> 
> Similar here, not all PCI devices are using PWRMU (or P-Unit, which
> support is absent) and might be AON, or be controllable via PMCSR only.

Hm, then mid_pci_power_manageable() is broken, you let it return true
unconditionally.  I'm not familiar at all with Intel MID devices, do
you have a way to clearly identify if a PCI device is power managed
by the PWRMU versus PMCSR?  My guess is that at the very least,
intel_mid_pwr_get_lss_id() needs to be called and false needs to
be returned by mid_pci_power_manageable() if the return value is
negative.

As said it's not a problem for the single existing caller of
mid_pci_get_power_state(), which is pci_target_state(), since it
only honors the return value if it's PCI_D3cold. Otherwise it
defers to the PMCSR.  The rationale is that reading the PMCSR is
usually the most reliable way to determine the power state, but
D3cold cannot be determined by reading the PMCSR.

What you say sounds to me like you need to amend the callbacks
in mid_pci_platform_pm to differentiate between PWRMU-manageable
versus non-PWRMU-manageable devices, but that's not specific to
the single ->get_state callback added by this commit and can thus
go into a separate commit.

> 
> > +
> > +	reg = (id * LSS_PWS_BITS) / 32;
> > +	bit = (id * LSS_PWS_BITS) % 32;
> > +	power = mid_pwr_get_state(pwr, reg);
> > +	return (power >> bit) & 3;
> 
> Don't add sparse warnings:
> 
>         return (__force pci_power_t)((power >> bit) & 3);

Okay, I'll fix this in v2.

Thanks again,

Lukas