diff mbox series

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

Message ID 20230809185453.40916-9-mario.limonciello@amd.com
State New
Headers show
Series Fix wakeup problems on some AMD platforms | expand

Commit Message

Mario Limonciello Aug. 9, 2023, 6:54 p.m. UTC
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(-)

Comments

Mario Limonciello Aug. 11, 2023, 12:40 p.m. UTC | #1
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
diff mbox series

Patch

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;
 }