Net driver fixes related to power management

PATCH In review Rafael J. Wysocki 3 participants 15 messages Feb 10, 2011 – Feb 11, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Rafael J. Wysocki

author

2011-02-10 16:56 UTC #0 lore raw

cover Net driver fixes related to power management

Hi,

The following series of patches fix minor issues related to power management in the tg3, alt1 and atl1c drivers.

[1/3] - tg3: Don't use device_init_wakeup() (PCI does that) [2/3] - atl1c: Don't use device_init_wakeup() (ditto) [3/3] - atl1: Don't use legacy PCI power management.

Please consider for applying.

Thanks,
Rafael

Rafael J. Wysocki

author

2011-02-10 16:53 UTC #1 lore raw

PATCH 1/3 tg3: Avoid setting power.can_wakeup for devices that cannot wake up

From: Rafael J. Wysocki <[email protected]>

The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/tg3.c +4 -2

Index: linux-2.6/drivers/net/tg3.c
===================================================================
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)

Rafael J. Wysocki

author

2011-02-10 16:56 UTC #2 lore raw

PATCH 1/3 tg3: Avoid setting power.can_wakeup for devices that cannot wake up

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/net/tg3.c +4 -2

Index: linux-2.6/drivers/net/tg3.c
===================================================================
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)

Matt Carlson

2011-02-10 20:48 UTC #3 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

Quoted 19 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410

Quoted 10 lines

drivers/net/tg3.c +4 -2

@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);

I did this because I couldn't see where 'can_wakeup' gets set. I don't see a call to device_init_wakeup() that would be relevant to tg3 devices. I do see a couple calls to device_set_wakeup_capable() in acpi/glue.c and acpi/scan.c. Is that the place?

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413

Quoted 3 lines

drivers/net/tg3.c +0 -0

 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)

This is something I was always curious about too. The TG3_FLAG_WOL_CAP tracks whether or not the device can handle WOL. Is it safe to do away with this flag and lean on the 'can_wakeup' flag instead? My concern is that some other part of the system might enable that flag after tg3_get_invariants() has run. If that happens and the device isn't really WOL capable bad things can occur.

Matt Carlson

2011-02-10 20:49 UTC #4 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

In reply to Rafael J. Wysocki

Quoted 19 lines · Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410

Quoted 10 lines

drivers/net/tg3.c +4 -2

@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413

Quoted 3 lines

drivers/net/tg3.c +0 -0

 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)

Rafael J. Wysocki

author

2011-02-10 21:08 UTC #5 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

Quoted 27 lines · Matt Carlson

Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================
drivers/net/tg3.c +2 -2
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,

Quoted 8 lines

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410
Quoted 3 lines
drivers/net/tg3.c +2 -0
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
I did this because I couldn't see where 'can_wakeup' gets set. I don't see a call to device_init_wakeup() that would be relevant to tg3 devices. I do see a couple calls to device_set_wakeup_capable() in acpi/glue.c and acpi/scan.c. Is that the place?

No, it's pci_pm_init() or platform_pci_wakeup_init() and they both use device_set_wakeup_capable() rather tha device_init_wakeup(), which is just a combination of device_set_wakeup_capable() and device_set_wakeup_enable() anyway.

And there's no why reason PCI drivers should use device_pm_init() at all.

Quoted 7 lines

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413
Quoted 3 lines
drivers/net/tg3.c +0 -0
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)
This is something I was always curious about too. The TG3_FLAG_WOL_CAP tracks whether or not the device can handle WOL. Is it safe to do away with this flag and lean on the 'can_wakeup' flag instead?

I don't think so. power.can_wakeup only tracks the capability to generate wakeup signals from the PCI perspective and it is set by PCI if the device appears to be able to generate wakeup signals.

So, I think the driver should work as in the $subject patch - reset the power.can_wakeup flag if TG3_FLAG_WOL_CAP is unset and don't touch it otherwise.

Thanks,
Rafael

Rafael J. Wysocki

author

2011-02-10 21:09 UTC #6 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

In reply to Matt Carlson

Quoted 27 lines · Matt Carlson

Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================
drivers/net/tg3.c +2 -2
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,

Quoted 8 lines

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410
Quoted 3 lines
drivers/net/tg3.c +2 -0
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
I did this because I couldn't see where 'can_wakeup' gets set. I don't see a call to device_init_wakeup() that would be relevant to tg3 devices. I do see a couple calls to device_set_wakeup_capable() in acpi/glue.c and acpi/scan.c. Is that the place?

And there's no why reason PCI drivers should use device_pm_init() at all.

Quoted 7 lines

drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413
Quoted 3 lines
drivers/net/tg3.c +0 -0
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)
This is something I was always curious about too. The TG3_FLAG_WOL_CAP tracks whether or not the device can handle WOL. Is it safe to do away with this flag and lean on the 'can_wakeup' flag instead?

I don't think so. power.can_wakeup only tracks the capability to generate wakeup signals from the PCI perspective and it is set by PCI if the device appears to be able to generate wakeup signals.

So, I think the driver should work as in the $subject patch - reset the power.can_wakeup flag if TG3_FLAG_WOL_CAP is unset and don't touch it otherwise.

Thanks,
Rafael

Matt Carlson

Acked-by

2011-02-11 00:00 UTC #7 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

Quoted 37 lines · Rafael J. Wysocki

Quoted 27 lines · Matt Carlson
Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================
drivers/net/tg3.c +2 -2
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
Quoted 8 lines
drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410
Quoted 3 lines
drivers/net/tg3.c +2 -0
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
I did this because I couldn't see where 'can_wakeup' gets set. I don't see a call to device_init_wakeup() that would be relevant to tg3 devices. I do see a couple calls to device_set_wakeup_capable() in acpi/glue.c and acpi/scan.c. Is that the place?

Quoted 6 lines

No, it's pci_pm_init() or platform_pci_wakeup_init() and they both use device_set_wakeup_capable() rather tha device_init_wakeup(), which is just a combination of device_set_wakeup_capable() and device_set_wakeup_enable() anyway.
And there's no why reason PCI drivers should use device_pm_init() at all.

O.K.

Acked-by Matt Carlson <[email protected]>

Quoted 8 lines

Quoted 7 lines
drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413
Quoted 3 lines
drivers/net/tg3.c +0 -0
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)
This is something I was always curious about too. The TG3_FLAG_WOL_CAP tracks whether or not the device can handle WOL. Is it safe to do away with this flag and lean on the 'can_wakeup' flag instead?

Quoted 7 lines

I don't think so. power.can_wakeup only tracks the capability to generate wakeup signals from the PCI perspective and it is set by PCI if the device appears to be able to generate wakeup signals.
So, I think the driver should work as in the $subject patch - reset the power.can_wakeup flag if TG3_FLAG_WOL_CAP is unset and don't touch it otherwise.

That makes sense. Thanks.

Matt Carlson

Acked-by

2011-02-11 00:00 UTC #8 lore raw

Re: [PATCH 1/3] tg3: Avoid setting power.can_wakeup for devices that cannot wake up

In reply to Rafael J. Wysocki

Quoted 37 lines · Rafael J. Wysocki

Quoted 27 lines · Matt Carlson
Quoted 26 lines · Rafael J. Wysocki
From: Rafael J. Wysocki <[email protected]>
The tg3 driver uses device_init_wakeup() in such a way that the device's power.can_wakeup flag may be set even though the PCI subsystem cleared it before, in which case the device cannot wake up the system from sleep states. Modify the driver to only change the power.can_wakeup flag if the device is not capable of generating wakeup signals.
Signed-off-by Rafael J. Wysocki <[email protected]>
---
 drivers/net/tg3.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/net/tg3.c
===================================================================
drivers/net/tg3.c +2 -2
@@ -12403,9 +12403,11 @@ static void __devinit tg3_get_eeprom_hw_
 			tp->tg3_flags3 |= TG3_FLG3_RGMII_EXT_IBND_TX_EN;
 	}
 done:
-	device_init_wakeup(&tp->pdev->dev, tp->tg3_flags & TG3_FLAG_WOL_CAP);
-	device_set_wakeup_enable(&tp->pdev->dev,
+	if (tp->tg3_flags & TG3_FLAG_WOL_CAP)
+		device_set_wakeup_enable(&tp->pdev->dev,
Quoted 8 lines
drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12410
Quoted 3 lines
drivers/net/tg3.c +2 -0
 				 tp->tg3_flags & TG3_FLAG_WOL_ENABLE);
+	else
+		device_set_wakeup_capable(&tp->pdev->dev, false);
I did this because I couldn't see where 'can_wakeup' gets set. I don't see a call to device_init_wakeup() that would be relevant to tg3 devices. I do see a couple calls to device_set_wakeup_capable() in acpi/glue.c and acpi/scan.c. Is that the place?

Quoted 6 lines

No, it's pci_pm_init() or platform_pci_wakeup_init() and they both use device_set_wakeup_capable() rather tha device_init_wakeup(), which is just a combination of device_set_wakeup_capable() and device_set_wakeup_enable() anyway.
And there's no why reason PCI drivers should use device_pm_init() at all.

O.K.

Acked-by Matt Carlson <[email protected]>

Quoted 8 lines

Quoted 7 lines
drivers/net/tg3.c static void __devinit tg3_get_eeprom_hw_ · line 12413
Quoted 3 lines
drivers/net/tg3.c +0 -0
 }
 
 static int __devinit tg3_issue_otp_command(struct tg3 *tp, u32 cmd)
This is something I was always curious about too. The TG3_FLAG_WOL_CAP tracks whether or not the device can handle WOL. Is it safe to do away with this flag and lean on the 'can_wakeup' flag instead?

Quoted 7 lines

I don't think so. power.can_wakeup only tracks the capability to generate wakeup signals from the PCI perspective and it is set by PCI if the device appears to be able to generate wakeup signals.
So, I think the driver should work as in the $subject patch - reset the power.can_wakeup flag if TG3_FLAG_WOL_CAP is unset and don't touch it otherwise.

That makes sense. Thanks.

Rafael J. Wysocki

author

2011-02-10 16:54 UTC #9 lore raw

PATCH 2/3 atl1c: Do not call device_init_wakeup() in atl1c_probe()

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The atl1c driver shouldn't call device_init_wakeup() in its probe routine with the second argument equal to 1, because for PCI devices the wakeup capability setting is initialized as appropriate by the PCI subsystem. Remove the potentially harmful call.

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/atl1c/atl1c_main.c |    1 -
 1 file changed, 1 deletion(-)

drivers/net/atl1c/atl1c_main.c +0 -1

Index: linux-2.6/drivers/net/atl1c/atl1c_main.c
===================================================================
@@ -2718,7 +2718,6 @@ static int __devinit atl1c_probe(struct
 		goto err_reset;
 	}
 
-	device_init_wakeup(&pdev->dev, 1);
 	/* reset the controller to
 	 * put the device in a known good starting state */
 	err = atl1c_phy_init(&adapter->hw);

Rafael J. Wysocki

author

2011-02-10 16:56 UTC #10 lore raw

PATCH 2/3 atl1c: Do not call device_init_wakeup() in atl1c_probe()

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/atl1c/atl1c_main.c |    1 -
 1 file changed, 1 deletion(-)

drivers/net/atl1c/atl1c_main.c +0 -1

Index: linux-2.6/drivers/net/atl1c/atl1c_main.c
===================================================================
@@ -2718,7 +2718,6 @@ static int __devinit atl1c_probe(struct
 		goto err_reset;
 	}
 
-	device_init_wakeup(&pdev->dev, 1);
 	/* reset the controller to
 	 * put the device in a known good starting state */
 	err = atl1c_phy_init(&adapter->hw);

Rafael J. Wysocki

author

2011-02-10 16:55 UTC #11 lore raw

PATCH 3/3 atl1: Do not use legacy PCI power management

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

The atl1 driver uses the legacy PCI power management, so it has to do some PCI-specific things in its ->suspend() and ->resume() callbacks, which isn't necessary and should better be done by the PCI subsystem-level power management code.

Convert atl1 to the new PCI power management framework and make it let the PCI subsystem take care of all the PCI-specific aspects of device handling during system power transitions.

Tested-by Thomas Fjellstrom <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/atlx/atl1.c |   77 +++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 46 deletions(-)

drivers/net/atlx/atl1.c +31 ~~-46~~

Index: linux-2.6/drivers/net/atlx/atl1.c
===================================================================
@@ -950,6 +950,7 @@ static int __devinit atl1_sw_init(struct
 	hw->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
 	adapter->wol = 0;
+	device_set_wakeup_enable(&adapter->pdev->dev, false);
 	adapter->rx_buffer_len = (hw->max_frame_size + 7) & ~7;
 	adapter->ict = 50000;		/* 100ms */
 	adapter->link_speed = SPEED_0;	/* hardware init */

@@ -2735,15 +2736,15 @@ static int atl1_close(struct net_device
 }
 
 #ifdef CONFIG_PM
-static int atl1_suspend(struct pci_dev *pdev, pm_message_t state)
+static int atl1_suspend(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct atl1_adapter *adapter = netdev_priv(netdev);
 	struct atl1_hw *hw = &adapter->hw;
 	u32 ctrl = 0;
 	u32 wufc = adapter->wol;
 	u32 val;
-	int retval;
 	u16 speed;
 	u16 duplex;

@@ -2751,17 +2752,15 @@ static int atl1_suspend(struct pci_dev *
 	if (netif_running(netdev))
 		atl1_down(adapter);
 
-	retval = pci_save_state(pdev);
-	if (retval)
-		return retval;
-
 	atl1_read_phy_reg(hw, MII_BMSR, (u16 *) & ctrl);
 	atl1_read_phy_reg(hw, MII_BMSR, (u16 *) & ctrl);
 	val = ctrl & BMSR_LSTATUS;
 	if (val)
 		wufc &= ~ATLX_WUFC_LNKC;
+	if (!wufc)
+		goto disable_wol;
 
-	if (val && wufc) {
+	if (val) {
 		val = atl1_get_speed_and_duplex(hw, &speed, &duplex);
 		if (val) {
 			if (netif_msg_ifdown(adapter))

@@ -2798,23 +2797,18 @@ static int atl1_suspend(struct pci_dev *
 		ctrl |= PCIE_PHYMISC_FORCE_RCV_DET;
 		iowrite32(ctrl, hw->hw_addr + REG_PCIE_PHYMISC);
 		ioread32(hw->hw_addr + REG_PCIE_PHYMISC);
-
-		pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
-		goto exit;
-	}
-
-	if (!val && wufc) {
+	} else {
 		ctrl |= (WOL_LINK_CHG_EN | WOL_LINK_CHG_PME_EN);
 		iowrite32(ctrl, hw->hw_addr + REG_WOL_CTRL);
 		ioread32(hw->hw_addr + REG_WOL_CTRL);
 		iowrite32(0, hw->hw_addr + REG_MAC_CTRL);
 		ioread32(hw->hw_addr + REG_MAC_CTRL);
 		hw->phy_configured = false;
-		pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
-		goto exit;
 	}
 
-disable_wol:
+	return 0;
+
+ disable_wol:
 	iowrite32(0, hw->hw_addr + REG_WOL_CTRL);
 	ioread32(hw->hw_addr + REG_WOL_CTRL);
 	ctrl = ioread32(hw->hw_addr + REG_PCIE_PHYMISC);

@@ -2822,37 +2816,17 @@ disable_wol:
 	iowrite32(ctrl, hw->hw_addr + REG_PCIE_PHYMISC);
 	ioread32(hw->hw_addr + REG_PCIE_PHYMISC);
 	hw->phy_configured = false;
-	pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
-exit:
-	if (netif_running(netdev))
-		pci_disable_msi(adapter->pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 
 	return 0;
 }
 
-static int atl1_resume(struct pci_dev *pdev)
+static int atl1_resume(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct atl1_adapter *adapter = netdev_priv(netdev);
-	u32 err;
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-
-	err = pci_enable_device(pdev);
-	if (err) {
-		if (netif_msg_ifup(adapter))
-			dev_printk(KERN_DEBUG, &pdev->dev,
-				"error enabling pci device\n");
-		return err;
-	}
-
-	pci_set_master(pdev);
 	iowrite32(0, adapter->hw.hw_addr + REG_WOL_CTRL);
-	pci_enable_wake(pdev, PCI_D3hot, 0);
-	pci_enable_wake(pdev, PCI_D3cold, 0);
 
 	atl1_reset_hw(&adapter->hw);

@@ -2864,16 +2838,25 @@ static int atl1_resume(struct pci_dev *p
 
 	return 0;
 }
+
+static SIMPLE_DEV_PM_OPS(atl1_pm_ops, atl1_suspend, atl1_resume);
+#define ATL1_PM_OPS	(&atl1_pm_ops)
+
 #else
-#define atl1_suspend NULL
-#define atl1_resume NULL
+
+static int atl1_suspend(struct device *dev) { return 0; }
+
+#define ATL1_PM_OPS	NULL
 #endif
 
 static void atl1_shutdown(struct pci_dev *pdev)
 {
-#ifdef CONFIG_PM
-	atl1_suspend(pdev, PMSG_SUSPEND);
-#endif
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct atl1_adapter *adapter = netdev_priv(netdev);
+
+	atl1_suspend(&pdev->dev);
+	pci_wake_from_d3(pdev, adapter->wol);
+	pci_set_power_state(pdev, PCI_D3hot);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

@@ -3117,9 +3100,8 @@ static struct pci_driver atl1_driver = {
 	.id_table = atl1_pci_tbl,
 	.probe = atl1_probe,
 	.remove = __devexit_p(atl1_remove),
-	.suspend = atl1_suspend,
-	.resume = atl1_resume,
-	.shutdown = atl1_shutdown
+	.shutdown = atl1_shutdown,
+	.driver.pm = ATL1_PM_OPS,
 };
 
 /*

@@ -3409,6 +3391,9 @@ static int atl1_set_wol(struct net_devic
 	adapter->wol = 0;
 	if (wol->wolopts & WAKE_MAGIC)
 		adapter->wol |= ATLX_WUFC_MAG;
+
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }

Rafael J. Wysocki

author

2011-02-10 16:56 UTC #12 lore raw

PATCH 3/3 atl1: Do not use legacy PCI power management

In reply to Rafael J. Wysocki

From: Rafael J. Wysocki <[email protected]>

Convert atl1 to the new PCI power management framework and make it let the PCI subsystem take care of all the PCI-specific aspects of device handling during system power transitions.

Tested-by Thomas Fjellstrom <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/net/atlx/atl1.c |   77 +++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 46 deletions(-)

drivers/net/atlx/atl1.c +31 ~~-46~~

Index: linux-2.6/drivers/net/atlx/atl1.c
===================================================================
@@ -950,6 +950,7 @@ static int __devinit atl1_sw_init(struct
 	hw->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
 	adapter->wol = 0;
+	device_set_wakeup_enable(&adapter->pdev->dev, false);
 	adapter->rx_buffer_len = (hw->max_frame_size + 7) & ~7;
 	adapter->ict = 50000;		/* 100ms */
 	adapter->link_speed = SPEED_0;	/* hardware init */

@@ -2735,15 +2736,15 @@ static int atl1_close(struct net_device
 }
 
 #ifdef CONFIG_PM
-static int atl1_suspend(struct pci_dev *pdev, pm_message_t state)
+static int atl1_suspend(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct atl1_adapter *adapter = netdev_priv(netdev);
 	struct atl1_hw *hw = &adapter->hw;
 	u32 ctrl = 0;
 	u32 wufc = adapter->wol;
 	u32 val;
-	int retval;
 	u16 speed;
 	u16 duplex;

@@ -2751,17 +2752,15 @@ static int atl1_suspend(struct pci_dev *
 	if (netif_running(netdev))
 		atl1_down(adapter);
 
-	retval = pci_save_state(pdev);
-	if (retval)
-		return retval;
-
 	atl1_read_phy_reg(hw, MII_BMSR, (u16 *) & ctrl);
 	atl1_read_phy_reg(hw, MII_BMSR, (u16 *) & ctrl);
 	val = ctrl & BMSR_LSTATUS;
 	if (val)
 		wufc &= ~ATLX_WUFC_LNKC;
+	if (!wufc)
+		goto disable_wol;
 
-	if (val && wufc) {
+	if (val) {
 		val = atl1_get_speed_and_duplex(hw, &speed, &duplex);
 		if (val) {
 			if (netif_msg_ifdown(adapter))

@@ -2798,23 +2797,18 @@ static int atl1_suspend(struct pci_dev *
 		ctrl |= PCIE_PHYMISC_FORCE_RCV_DET;
 		iowrite32(ctrl, hw->hw_addr + REG_PCIE_PHYMISC);
 		ioread32(hw->hw_addr + REG_PCIE_PHYMISC);
-
-		pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
-		goto exit;
-	}
-
-	if (!val && wufc) {
+	} else {
 		ctrl |= (WOL_LINK_CHG_EN | WOL_LINK_CHG_PME_EN);
 		iowrite32(ctrl, hw->hw_addr + REG_WOL_CTRL);
 		ioread32(hw->hw_addr + REG_WOL_CTRL);
 		iowrite32(0, hw->hw_addr + REG_MAC_CTRL);
 		ioread32(hw->hw_addr + REG_MAC_CTRL);
 		hw->phy_configured = false;
-		pci_enable_wake(pdev, pci_choose_state(pdev, state), 1);
-		goto exit;
 	}
 
-disable_wol:
+	return 0;
+
+ disable_wol:
 	iowrite32(0, hw->hw_addr + REG_WOL_CTRL);
 	ioread32(hw->hw_addr + REG_WOL_CTRL);
 	ctrl = ioread32(hw->hw_addr + REG_PCIE_PHYMISC);

@@ -2822,37 +2816,17 @@ disable_wol:
 	iowrite32(ctrl, hw->hw_addr + REG_PCIE_PHYMISC);
 	ioread32(hw->hw_addr + REG_PCIE_PHYMISC);
 	hw->phy_configured = false;
-	pci_enable_wake(pdev, pci_choose_state(pdev, state), 0);
-exit:
-	if (netif_running(netdev))
-		pci_disable_msi(adapter->pdev);
-	pci_disable_device(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
 
 	return 0;
 }
 
-static int atl1_resume(struct pci_dev *pdev)
+static int atl1_resume(struct device *dev)
 {
+	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct atl1_adapter *adapter = netdev_priv(netdev);
-	u32 err;
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-
-	err = pci_enable_device(pdev);
-	if (err) {
-		if (netif_msg_ifup(adapter))
-			dev_printk(KERN_DEBUG, &pdev->dev,
-				"error enabling pci device\n");
-		return err;
-	}
-
-	pci_set_master(pdev);
 	iowrite32(0, adapter->hw.hw_addr + REG_WOL_CTRL);
-	pci_enable_wake(pdev, PCI_D3hot, 0);
-	pci_enable_wake(pdev, PCI_D3cold, 0);
 
 	atl1_reset_hw(&adapter->hw);

@@ -2864,16 +2838,25 @@ static int atl1_resume(struct pci_dev *p
 
 	return 0;
 }
+
+static SIMPLE_DEV_PM_OPS(atl1_pm_ops, atl1_suspend, atl1_resume);
+#define ATL1_PM_OPS	(&atl1_pm_ops)
+
 #else
-#define atl1_suspend NULL
-#define atl1_resume NULL
+
+static int atl1_suspend(struct device *dev) { return 0; }
+
+#define ATL1_PM_OPS	NULL
 #endif
 
 static void atl1_shutdown(struct pci_dev *pdev)
 {
-#ifdef CONFIG_PM
-	atl1_suspend(pdev, PMSG_SUSPEND);
-#endif
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct atl1_adapter *adapter = netdev_priv(netdev);
+
+	atl1_suspend(&pdev->dev);
+	pci_wake_from_d3(pdev, adapter->wol);
+	pci_set_power_state(pdev, PCI_D3hot);
 }
 
 #ifdef CONFIG_NET_POLL_CONTROLLER

@@ -3117,9 +3100,8 @@ static struct pci_driver atl1_driver = {
 	.id_table = atl1_pci_tbl,
 	.probe = atl1_probe,
 	.remove = __devexit_p(atl1_remove),
-	.suspend = atl1_suspend,
-	.resume = atl1_resume,
-	.shutdown = atl1_shutdown
+	.shutdown = atl1_shutdown,
+	.driver.pm = ATL1_PM_OPS,
 };
 
 /*

@@ -3409,6 +3391,9 @@ static int atl1_set_wol(struct net_devic
 	adapter->wol = 0;
 	if (wol->wolopts & WAKE_MAGIC)
 		adapter->wol |= ATLX_WUFC_MAG;
+
+	device_set_wakeup_enable(&adapter->pdev->dev, adapter->wol);
+
 	return 0;
 }

David Miller

maintainer

2011-02-11 19:38 UTC #13 lore raw

In reply to Rafael J. Wysocki

From: "Rafael J. Wysocki" <[email protected]>
Date: Thu, 10 Feb 2011 17:51:56 +0100

Quoted 8 lines

The following series of patches fix minor issues related to power management in the tg3, alt1 and atl1c drivers.
[1/3] - tg3: Don't use device_init_wakeup() (PCI does that) [2/3] - atl1c: Don't use device_init_wakeup() (ditto) [3/3] - atl1: Don't use legacy PCI power management.
Please consider for applying.

All applied to net-next-2.6, thank you.

David Miller

maintainer

2011-02-11 19:39 UTC #14 lore raw

In reply to Rafael J. Wysocki

From: "Rafael J. Wysocki" <[email protected]>
Date: Thu, 10 Feb 2011 17:51:56 +0100

Quoted 8 lines

The following series of patches fix minor issues related to power management in the tg3, alt1 and atl1c drivers.
[1/3] - tg3: Don't use device_init_wakeup() (PCI does that) [2/3] - atl1c: Don't use device_init_wakeup() (ditto) [3/3] - atl1: Don't use legacy PCI power management.
Please consider for applying.

All applied to net-next-2.6, thank you.