[v11] Fix wakeup problems on some AMD platforms

[PATCH v11 8/9] PCI: Split PME state selection into a local static function

Posted by Mario Limonciello 2 years, 6 months ago

When a device is not power manageable by the platform, and not already
in a low power state pci_target_state() will find the deepest state
that PME is supported and use this to select the wakeup state.

Simplify this logic and split it out to a local function. No intended
functional changes.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v10->v11:
 * New patch split from existing patch in v10
---
 drivers/pci/pci.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0c..693f4ca90452b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2660,6 +2660,20 @@ int pci_wake_from_d3(struct pci_dev *dev, bool enable)
 }
 EXPORT_SYMBOL(pci_wake_from_d3);
 
+/*
+ * Find the deepest state from which the device can generate
+ * PME#.
+ */
+static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
+{
+	pci_power_t state = PCI_D3hot;
+
+	while (state && !(dev->pme_support & (1 << state)))
+		state--;
+
+	return state;
+}
+
 /**
  * pci_target_state - find an appropriate low power state for a given PCI dev
  * @dev: PCI device
@@ -2701,21 +2715,8 @@ static pci_power_t pci_target_state(struct pci_dev *dev, bool wakeup)
 	else if (!dev->pm_cap)
 		return PCI_D0;
 
-	if (wakeup && dev->pme_support) {
-		pci_power_t state = PCI_D3hot;
-
-		/*
-		 * Find the deepest state from which the device can generate
-		 * PME#.
-		 */
-		while (state && !(dev->pme_support & (1 << state)))
-			state--;
-
-		if (state)
-			return state;
-		else if (dev->pme_support & 1)
-			return PCI_D0;
-	}
+	if (wakeup && dev->pme_support)
+		return pci_get_wake_pme_state(dev);
 
 	return PCI_D3hot;
 }
-- 
2.34.1

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

Posted by Andy Shevchenko 2 years, 6 months ago

On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
> When a device is not power manageable by the platform, and not already
> in a low power state pci_target_state() will find the deepest state
> that PME is supported and use this to select the wakeup state.
> 
> Simplify this logic and split it out to a local function. No intended
> functional changes.

...

> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
> +{
> +	pci_power_t state = PCI_D3hot;
> +
> +	while (state && !(dev->pme_support & (1 << state)))
> +		state--;
> +
> +	return state;

Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).

Basically it's something like

	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));

(but double check and test the logic).

> +}

...

Yeah, I see that is the existing code, perhaps amend it first?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

Posted by Limonciello, Mario 2 years, 6 months ago


On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
> On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
>> When a device is not power manageable by the platform, and not already
>> in a low power state pci_target_state() will find the deepest state
>> that PME is supported and use this to select the wakeup state.
>>
>> Simplify this logic and split it out to a local function. No intended
>> functional changes.
> 
> ...
> 
>> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>> +{
>> +	pci_power_t state = PCI_D3hot;
>> +
>> +	while (state && !(dev->pme_support & (1 << state)))
>> +		state--;
>> +
>> +	return state;
> 
> Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
> 
> Basically it's something like
> 
> 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
> 
> (but double check and test the logic).
> 
>> +}
> 
> ...
> 
> Yeah, I see that is the existing code, perhaps amend it first?
> 

Are you sure?  I actually double checked the sparse output using this 
command before I sent it.

make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o

I didn't see anything related to the line numbers for this function.

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

Posted by Andy Shevchenko 2 years, 6 months ago

On Thu, Aug 10, 2023 at 11:29:49AM -0500, Limonciello, Mario wrote:
> On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
> > On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:

...

> > > +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
> > > +{
> > > +	pci_power_t state = PCI_D3hot;
> > > +
> > > +	while (state && !(dev->pme_support & (1 << state)))
> > > +		state--;
> > > +
> > > +	return state;
> > 
> > Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
> > 
> > Basically it's something like
> > 
> > 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
> > 
> > (but double check and test the logic).
> > 
> > > +}
> > 
> > ...
> > 
> > Yeah, I see that is the existing code, perhaps amend it first?
> > 
> 
> Are you sure?

Yes.

Just the original code

drivers/pci/pci.c:2711:60: warning: restricted pci_power_t degrades to integer
drivers/pci/pci.c:2712:30: warning: restricted pci_power_t degrades to integer

                /*
                 * Find the deepest state from which the device can generate
                 * PME#.
                 */
2711 ==>         while (state && !(dev->pme_support & (1 << state)))
2712 ==>                state--;

How is yours different?

> I actually double checked the sparse output using this
> command before I sent it.
> 
> make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o
> 
> I didn't see anything related to the line numbers for this function.

Just in case...

$ gcc --version
gcc (Debian 12.3.0-5) 12.3.0

$ sparse --version
0.6.4 (Debian: 0.6.4-3)

$ make --version
GNU Make 4.3
Built for x86_64-pc-linux-gnu

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v11 8/9] PCI: Split PME state selection into a local static function

Posted by Limonciello, Mario 2 years, 6 months ago


On 8/11/2023 3:55 AM, Andy Shevchenko wrote:
> On Thu, Aug 10, 2023 at 11:29:49AM -0500, Limonciello, Mario wrote:
>> On 8/10/2023 11:21 AM, Andy Shevchenko wrote:
>>> On Wed, Aug 09, 2023 at 01:54:52PM -0500, Mario Limonciello wrote:
> 
> ...
> 
>>>> +static inline pci_power_t pci_get_wake_pme_state(struct pci_dev *dev)
>>>> +{
>>>> +	pci_power_t state = PCI_D3hot;
>>>> +
>>>> +	while (state && !(dev->pme_support & (1 << state)))
>>>> +		state--;
>>>> +
>>>> +	return state;
>>>
>>> Sparse won't be happy about this code (with CF=-D__CHECK_ENDIAN__).
>>>
>>> Basically it's something like
>>>
>>> 	return (__force pci_power_t)fls(dev->pme_support & GENMASK(PCI_D3hot, 0));
>>>
>>> (but double check and test the logic).
>>>
>>>> +}
>>>
>>> ...
>>>
>>> Yeah, I see that is the existing code, perhaps amend it first?
>>>
>>
>> Are you sure?
> 
> Yes.
> 
> Just the original code
> 
> drivers/pci/pci.c:2711:60: warning: restricted pci_power_t degrades to integer
> drivers/pci/pci.c:2712:30: warning: restricted pci_power_t degrades to integer
> 
>                  /*
>                   * Find the deepest state from which the device can generate
>                   * PME#.
>                   */
> 2711 ==>         while (state && !(dev->pme_support & (1 << state)))
> 2712 ==>                state--;
> 
> How is yours different?
> 
>> I actually double checked the sparse output using this
>> command before I sent it.
>>
>> make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' drivers/pci/pci.o
>>
>> I didn't see anything related to the line numbers for this function.
> 
> Just in case...
> 
> $ gcc --version
> gcc (Debian 12.3.0-5) 12.3.0
> 
> $ sparse --version
> 0.6.4 (Debian: 0.6.4-3)
> 
> $ make --version
> GNU Make 4.3
> Built for x86_64-pc-linux-gnu
> 

Oh, I see why I wasn't observing it now.  Because it's inline code it's 
processed separately and isn't with the rest of the grouped output from 
2416 through 2922.

drivers/pci/pci.c:2679:52: sparse: warning: restricted pci_power_t 
degrades to integer
drivers/pci/pci.c:2680:22: sparse: warning: restricted pci_power_t 
degrades to integer