Re: Bug#609538: r8169: long delay during resume

Re: Bug#609538: r8169: long delay during resume

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1483 bytes --]

On Mon, 2011-01-10 at 14:14 +0100, Jarek Kamiński wrote:
> Package: linux-2.6
> Version: 2.6.37-1~experimental.1
> Severity: normal
> File: /lib/modules/2.6.37-trunk-amd64/kernel/drivers/net/r8169.ko
> Tags: upstream
> 
> Hi.
> 
> 2.6.37 introduces regression in r8169. During every resume I get ~20
> seconds delay:
> Jan 10 13:57:15 rocket kernel: [36458.257780] ata1.00: configured for UDMA/100
> Jan 10 13:57:15 rocket kernel: [36517.738421] r8169 0000:13:00.0: eth0: unable to apply firmware patch
> Jan 10 13:57:15 rocket kernel: [36517.739859] PM: resume of devices complete after 61177.644 msecs
> Jan 10 13:57:15 rocket kernel: [36517.740258] PM: Finishing wakeup.
> Jan 10 13:57:15 rocket kernel: [36517.740259] Restarting tasks ... done.
> 
> Bisecting leads to commit bca03d5f32c8ee9b5cfa1d32640a63fded6cb3c0
> (r8169: remove the firmware of RTL8111D.). Further debugging showed,
> that firmware.agent is not called at all, I guess that udev is not
> working before "Restarting tasks".

Right, only the kernel is running then.

> Either r8169 tries to load firmware too early, or it should keep it
> loaded in memory for use during resume.

It should.  But an earlier version of this patch was also in Debian's
2.6.36 so it would have had the same problem.

Ben.

> The problem persist no matter if I have firmware-realtek installed, or
> not.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Bug#609538: r8169: long delay during resume

[Jarek Kamiński]

W dniu 11.01.2011 06:49, Ben Hutchings pisze:
> On Mon, 2011-01-10 at 14:14 +0100, Jarek Kamiński wrote:
>> Package: linux-2.6
>> Version: 2.6.37-1~experimental.1

>> 2.6.37 introduces regression in r8169. During every resume I get ~20
>> seconds delay:
>> Jan 10 13:57:15 rocket kernel: [36458.257780] ata1.00: configured for UDMA/100
>> Jan 10 13:57:15 rocket kernel: [36517.738421] r8169 0000:13:00.0: eth0: unable to apply firmware patch
>> Jan 10 13:57:15 rocket kernel: [36517.739859] PM: resume of devices complete after 61177.644 msecs
>> Jan 10 13:57:15 rocket kernel: [36517.740258] PM: Finishing wakeup.
>> Jan 10 13:57:15 rocket kernel: [36517.740259] Restarting tasks ... done.
>>
>> Bisecting leads to commit bca03d5f32c8ee9b5cfa1d32640a63fded6cb3c0
>> (r8169: remove the firmware of RTL8111D.).

>> Either r8169 tries to load firmware too early, or it should keep it
>> loaded in memory for use during resume.
> 
> It should.  But an earlier version of this patch was also in Debian's
> 2.6.36 so it would have had the same problem.

The last 2.6.36 I've tried was 2.6.36-1~experimental.1, I've then
passsed and returned to 2.6.32 for unrelated problems. I think it wasn't
affected, but I can re-check it and/or test later 2.6.36 versions if it
may help.

Sorry if my information was misleading.


-- 
pozdr(); // Jarek

Re: Bug#609538: r8169: long delay during resume

[Francois Romieu]

Jarek Kamiński <jarek@vilo.eu.org> :
[...]
> The last 2.6.36 I've tried was 2.6.36-1~experimental.1, I've then
> passsed and returned to 2.6.32 for unrelated problems. I think it wasn't
> affected, but I can re-check it and/or test later 2.6.36 versions if it
> may help.

The patch below against 2.6.37-git + davem's pending fixes (see
http://marc.info/?l=linux-netdev&m=129448910825754) should help.

Can you give it a try ?

r8169: keep firmware in memory.

The firmware agent is not available during resume.

It will help with http://bugs.debian.org/609538 but it will not avoid
the 60 seconds delay when there is no firmware.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
Cc: Hayes <hayeswang@realtek.com>
Cc: Ben Hutchings <benh@debian.org>
---
 drivers/net/r8169.c |   43 +++++++++++++++++++++++++++++++------------
 1 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index dd758cd..7e2f01c 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -554,6 +554,8 @@ struct rtl8169_private {
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
 	u32 saved_wolopts;
+
+	const struct firmware *fw;
 };
 
 MODULE_AUTHOR("Realtek and the Linux r8169 crew <netdev@vger.kernel.org>");
@@ -1668,6 +1670,29 @@ rtl_phy_write_fw(struct rtl8169_private *tp, const struct firmware *fw)
 	}
 }
 
+static void rtl_release_firmware(struct rtl8169_private *tp)
+{
+	release_firmware(tp->fw);
+	tp->fw = NULL;
+}
+
+static int rtl_apply_firmware(struct rtl8169_private *tp, const char *fw_name)
+{
+	const struct firmware **fw = &tp->fw;
+	int rc = !*fw;
+
+	if (rc) {
+		rc = request_firmware(fw, fw_name, &tp->pci_dev->dev);
+		if (rc < 0)
+			goto out;
+	}
+
+	/* TODO: release firmware once rtl_phy_write_fw signals failures. */
+	rtl_phy_write_fw(tp, *fw);
+out:
+	return rc;
+}
+
 static void rtl8169s_hw_phy_config(struct rtl8169_private *tp)
 {
 	static const struct phy_reg phy_reg_init[] = {
@@ -2041,7 +2066,6 @@ static void rtl8168d_1_hw_phy_config(struct rtl8169_private *tp)
 		{ 0x0d, 0xf880 }
 	};
 	void __iomem *ioaddr = tp->mmio_addr;
-	const struct firmware *fw;
 
 	rtl_writephy_batch(tp, phy_reg_init_0, ARRAY_SIZE(phy_reg_init_0));
 
@@ -2105,11 +2129,8 @@ static void rtl8168d_1_hw_phy_config(struct rtl8169_private *tp)
 
 	rtl_writephy(tp, 0x1f, 0x0005);
 	rtl_writephy(tp, 0x05, 0x001b);
-	if (rtl_readphy(tp, 0x06) == 0xbf00 &&
-	    request_firmware(&fw, FIRMWARE_8168D_1, &tp->pci_dev->dev) == 0) {
-		rtl_phy_write_fw(tp, fw);
-		release_firmware(fw);
-	} else {
+	if ((rtl_readphy(tp, 0x06) != 0xbf00) ||
+	    (rtl_apply_firmware(tp, FIRMWARE_8168D_1) < 0)) {
 		netif_warn(tp, probe, tp->dev, "unable to apply firmware patch\n");
 	}
 
@@ -2159,7 +2180,6 @@ static void rtl8168d_2_hw_phy_config(struct rtl8169_private *tp)
 		{ 0x0d, 0xf880 }
 	};
 	void __iomem *ioaddr = tp->mmio_addr;
-	const struct firmware *fw;
 
 	rtl_writephy_batch(tp, phy_reg_init_0, ARRAY_SIZE(phy_reg_init_0));
 
@@ -2214,11 +2234,8 @@ static void rtl8168d_2_hw_phy_config(struct rtl8169_private *tp)
 
 	rtl_writephy(tp, 0x1f, 0x0005);
 	rtl_writephy(tp, 0x05, 0x001b);
-	if (rtl_readphy(tp, 0x06) == 0xb300 &&
-	    request_firmware(&fw, FIRMWARE_8168D_2, &tp->pci_dev->dev) == 0) {
-		rtl_phy_write_fw(tp, fw);
-		release_firmware(fw);
-	} else {
+	if ((rtl_readphy(tp, 0x06) != 0xb300) ||
+	    (rtl_apply_firmware(tp, FIRMWARE_8168D_2) < 0)) {
 		netif_warn(tp, probe, tp->dev, "unable to apply firmware patch\n");
 	}
 
@@ -4662,6 +4679,8 @@ static int rtl8169_close(struct net_device *dev)
 	tp->TxDescArray = NULL;
 	tp->RxDescArray = NULL;
 
+	rtl_release_firmware(tp);
+
 	pm_runtime_put_sync(&pdev->dev);
 
 	return 0;
-- 
1.7.3.4

Re: Bug#609538: r8169: long delay during resume

[Jarek Kamiński]

W dniu 11.01.2011 14:25, Francois Romieu pisze:
> Jarek Kamiński <jarek@vilo.eu.org> :
> [...]
>> The last 2.6.36 I've tried was 2.6.36-1~experimental.1, I've then
>> passsed and returned to 2.6.32 for unrelated problems. I think it wasn't
>> affected, but I can re-check it and/or test later 2.6.36 versions if it
>> may help.
> 
> The patch below against 2.6.37-git + davem's pending fixes (see
> http://marc.info/?l=linux-netdev&m=129448910825754) should help.
> 
> Can you give it a try ?
> 
> r8169: keep firmware in memory.

This patch applied against current git tree fixes problem (with firmware
installed). However, cherry-picking
eee3a96c6368f47df8df5bd4ed1843600652b337 (r8169: delay phy init until
device opens.) from net-2.6.git and applying "r8169: keep firmware in
memory." still results with
#v+
Jan 11 19:05:18 rocket kernel: [  673.176439] ata1.00: configured for
UDMA/100
Jan 11 19:05:18 rocket kernel: [  731.639054] r8169 0000:13:00.0: eth0:
unable to apply firmware patch
Jan 11 19:05:18 rocket kernel: [  731.640486] PM: resume of devices
complete after 60228.033 msecs
#v-
(I don't fully understand why)

"r8169: delay phy init until device opens." alone also doesn't do the trick.

-- 
pozdr(); // Jarek

Re: Bug#609538: r8169: long delay during resume

[Francois Romieu]

Jarek Kamiński <jarek@vilo.eu.org> :
[failure]

/me slaps head

Please check that it works if you add the patch below and I'll
merge both for a proper submission.

r8169: I am a clown.

Modifying rtl8169_reset_task() to tolerate firmware changes between
close() and open() but this is not high-priority.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>

diff --git a/drivers/net/r8169.c b/drivers/net/r8169.c
index 7e2f01c..57fa6bd 100644
--- a/drivers/net/r8169.c
+++ b/drivers/net/r8169.c
@@ -3119,6 +3119,8 @@ static void __devexit rtl8169_remove_one(struct pci_dev *pdev)
 
 	cancel_delayed_work_sync(&tp->task);
 
+	rtl_release_firmware(tp);
+
 	unregister_netdev(dev);
 
 	if (pci_dev_run_wake(pdev))
@@ -4679,8 +4681,6 @@ static int rtl8169_close(struct net_device *dev)
 	tp->TxDescArray = NULL;
 	tp->RxDescArray = NULL;
 
-	rtl_release_firmware(tp);
-
 	pm_runtime_put_sync(&pdev->dev);
 
 	return 0;

> "r8169: delay phy init until device opens." alone also doesn't do the trick.

It was the expected behavior.

-- 
Ueimor

Re: Bug#609538: r8169: long delay during resume

[Daniel J Blueman]

When resuming from suspended with 2.6.38-rc1 with my RTL8168d/8111d
hardware, I see significant delays [1].

The only firmware I see in /lib/firmware is RTL8192E, thus it looks
like this is due to it being not present.

I see that Francois' patch is present in 2.6.38-rc1; is there a way to
avoid this delay, or is this likely in request_firmware?

Thanks,
  Daniel

--- [1]

[21786.797521] sdhci-pci 0000:09:00.0: Will use DMA mode even though
HW doesn't fully claim to support it.
[21786.797530] sdhci-pci 0000:09:00.0: setting latency timer to 64
[21847.280237] r8169 0000:0b:00.0: eth0: unable to apply firmware patch
[21847.283688] PM: resume of devices complete after 61090.398 msecs
[21848.934980] Synaptics Touchpad, model: 1, fw: 7.2, id: 0x1c0b1,
caps: 0xd04733/0xa40000/0xa0000
[21848.980306] input: SynPS/2 Synaptics TouchPad as
/devices/platform/i8042/serio1/input/input12
[21850.886820] PM: Finishing wakeup.
[21850.888808] Restarting tasks ... done.
[21851.384291] r8169 0000:0b:00.0: eth0: unable to apply firmware patch
[21851.386090] r8169 0000:0b:00.0: eth0: link down
[21853.324784] EXT4-fs (sda1): re-mounted. Opts: commit=0
-- 
Daniel J Blueman

Re: Bug#609538: r8169: long delay during resume

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Thu, 2011-01-20 at 10:04 +0700, Daniel J Blueman wrote:
> When resuming from suspended with 2.6.38-rc1 with my RTL8168d/8111d
> hardware, I see significant delays [1].
> 
> The only firmware I see in /lib/firmware is RTL8192E, thus it looks
> like this is due to it being not present.

If you're using Debian, the necessary blob is in the firmware-realtek
package.

> I see that Francois' patch is present in 2.6.38-rc1; is there a way to
> avoid this delay, or is this likely in request_firmware?
[...]

It's a known problem with calls to request_firmware() when userland is
not running (early initialisation or resume from sleep).  It may be
fixable.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Bug#609538: r8169: long delay during resume

[Jarek Kamiński]

On Thu, Jan 20, 2011 at 03:20:03AM +0000, Ben Hutchings wrote:
> On Thu, 2011-01-20 at 10:04 +0700, Daniel J Blueman wrote:
>> I see that Francois' patch is present in 2.6.38-rc1; is there a way to
>> avoid this delay, or is this likely in request_firmware?

> It's a known problem with calls to request_firmware() when userland is
> not running (early initialisation or resume from sleep).  It may be
> fixable.

It was partially fixed in commit
f1e02ed109df5f99abf942b8ccc99960cb09dd38 in linux-2.6.git (r8169: keep
firmware in memory.). Sorry for not reporting it here, I clicked "reply"
instead of "group reply" late in the night.

If this commit could be included in Debian kernel, the bug could be
closed.

J.