Re: 2.6.28-git8: tg3 doesn't work due to firmware not loading (-git7 is ok)

On Fri, 09 Jan 2009 14:04:22 PST, David Miller said:

From: "Alessandro Suardi" <redacted>
Date: Fri, 9 Jan 2009 18:30:40 +0100

quoted

Clearly the requirement changed in -git8, AFAICS.

Because -git8 is where the firmware requirement got added.

If you build the TG3 driver statically into your kernel,
the only way to get the firmware properly loaded is to
enable that CONFIG_FIRMWARE_IN_KERNEL option.

Maybe this is one of those cases where we actually do want a SELECT?

	select FIRMWARE_IN_KERNEL if TIGON3=y

or similar?

From: Valdis.Kletnieks@vt.edu
Date: Sun, 11 Jan 2009 06:10:58 -0500

On Fri, 09 Jan 2009 14:04:22 PST, David Miller said:

quoted

From: "Alessandro Suardi" <redacted>
Date: Fri, 9 Jan 2009 18:30:40 +0100

quoted

Clearly the requirement changed in -git8, AFAICS.

Because -git8 is where the firmware requirement got added.

If you build the TG3 driver statically into your kernel,
the only way to get the firmware properly loaded is to
enable that CONFIG_FIRMWARE_IN_KERNEL option.

Maybe this is one of those cases where we actually do want a SELECT?

	select FIRMWARE_IN_KERNEL if TIGON3=y

or similar?

I have no idea how this is intended to work, David will
know.

On Sun, 2009-01-11 at 04:08 -0800, David Miller wrote:

From: Valdis.Kletnieks@vt.edu
Date: Sun, 11 Jan 2009 06:10:58 -0500

quoted

On Fri, 09 Jan 2009 14:04:22 PST, David Miller said:

quoted

From: "Alessandro Suardi" <redacted>
Date: Fri, 9 Jan 2009 18:30:40 +0100

quoted

Clearly the requirement changed in -git8, AFAICS.

Because -git8 is where the firmware requirement got added.

If you build the TG3 driver statically into your kernel,
the only way to get the firmware properly loaded is to
enable that CONFIG_FIRMWARE_IN_KERNEL option.

Maybe this is one of those cases where we actually do want a SELECT?

	select FIRMWARE_IN_KERNEL if TIGON3=y

or similar?

I have no idea how this is intended to work, David will know.

Actually, I think the real issue here might be that the tg3 driver is
now behaving _differently_ to how other modern drivers work. It tries to
obtain the firmware once at initialisation time and if that fails it
doesn't register the device.

Other drivers will load the firmware later, at the time the device is
brought up. This means that even if you build the driver into the kernel
without its firmware, it can still request the firmware later, when you
try to start _using_ it. And when the file system is available.

I'll take a look and see if I can remedy that. Then we wouldn't _need_
the FIRMWARE_IN_KERNEL option.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

On Sun, 2009-01-11 at 12:25 +0000, David Woodhouse wrote:

I'll take a look and see if I can remedy that. Then we wouldn't _need_
the FIRMWARE_IN_KERNEL option.

How about this? If it fails to load the firmware from userspace during
the initialisation, it'll try again later in tg3_open(). 

I _think_ that's fine, because we don't do anything else in the early
initialisation which requires the firmware to be loaded.

So if you build with CONFIG_TIGON3=y, CONFIG_FIRMWARE_IN_KERNEL=n, you
should see it fail to load the firmware at boot, but then it should load
it successfully when you bring the device up.

Untested-but-otherwise-Signed-off-by: David Woodhouse [off-list ref]

diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 5e2dbae..f99218c 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c

@@ -7535,11 +7535,45 @@ static int tg3_test_msi(struct tg3 *tp)
 	return err;
 }
 
+static int tg3_request_firmware(struct tg3 *tp)
+{
+	const __be32 *fw_data;
+
+	if (request_firmware(&tp->fw, tp->fw_needed, &tp->pdev->dev)) {
+		printk(KERN_ERR "tg3: Failed to load firmware \"%s\"\n",
+		       tp->fw_needed);
+		return -ENOENT;
+	}
+
+	fw_data = (void *)tp->fw->data;
+
+	/* Firmware blob starts with version numbers, followed by
+	   start address and _full_ length including BSS sections
+	   (which must be longer than the actual data, of course */
+
+	tp->fw_len = be32_to_cpu(fw_data[2]);	/* includes bss */
+	if (tp->fw_len < (tp->fw->size - 12)) {
+		printk(KERN_ERR "tg3: bogus length %d in \"%s\"\n",
+		       tp->fw_len, tp->fw_needed);
+		return -EINVAL;
+	}
+
+	/* We no longer need firmware; we have it. */
+	tp->fw_needed = NULL;
+	return 0;
+}
+
 static int tg3_open(struct net_device *dev)
 {
 	struct tg3 *tp = netdev_priv(dev);
 	int err;
 
+	if (tp->fw_needed) {
+		err = tg3_request_firmware(tp);
+		if (err)
+			return err;
+	}
+
 	netif_carrier_off(tp->dev);
 
 	err = tg3_set_power_state(tp, PCI_D0);

@@ -12934,7 +12968,6 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	struct net_device *dev;
 	struct tg3 *tp;
 	int err, pm_cap;
-	const char *fw_name = NULL;
 	char str[40];
 	u64 dma_mask, persist_dma_mask;
 

@@ -13091,7 +13124,7 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	tg3_init_bufmgr_config(tp);
 
 	if (tp->pci_chip_rev_id == CHIPREV_ID_5701_A0)
-		fw_name = FIRMWARE_TG3;
+		tp->fw_needed = FIRMWARE_TG3;
 
 	if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
 		tp->tg3_flags2 |= TG3_FLG2_TSO_CAPABLE;

@@ -13107,34 +13140,19 @@ static int __devinit tg3_init_one(struct pci_dev *pdev,
 	}
 	if (tp->tg3_flags2 & TG3_FLG2_TSO_CAPABLE) {
 		if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705)
-			fw_name = FIRMWARE_TG3TSO5;
+			tp->fw_needed = FIRMWARE_TG3TSO5;
 		else
-			fw_name = FIRMWARE_TG3TSO;
+			tp->fw_needed = FIRMWARE_TG3TSO;
 	}
 
-	if (fw_name) {
-		const __be32 *fw_data;
-
-		err = request_firmware(&tp->fw, fw_name, &tp->pdev->dev);
-		if (err) {
-			printk(KERN_ERR "tg3: Failed to load firmware \"%s\"\n",
-			       fw_name);
-			goto err_out_iounmap;
-		}
-
-		fw_data = (void *)tp->fw->data;
-
-		/* Firmware blob starts with version numbers, followed by
-		   start address and _full_ length including BSS sections
-		   (which must be longer than the actual data, of course */
-
-		tp->fw_len = be32_to_cpu(fw_data[2]);	/* includes bss */
-		if (tp->fw_len < (tp->fw->size - 12)) {
-			printk(KERN_ERR "tg3: bogus length %d in \"%s\"\n",
-			       tp->fw_len, fw_name);
-			err = -EINVAL;
+	if (tp->fw_needed) {
+		err = tg3_request_firmware(tp);
+		/* Failure to load firmware at this stage is not fatal; we'll
+		   try again in tg3_open(). So if you have the driver built
+		   into the kernel, you can still get the firmware loaded
+		   after userspace is running, when the device comes up. */
+		if (err != -ENOENT)
 			goto err_out_fw;
-		}
 	}
 
 	/* TSO is on by default on chips that support hardware TSO.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

On Sun, Jan 11, 2009 at 1:59 PM, David Woodhouse [off-list ref] wrote:

Patches cleanly but doesn't build in 2.6.29-rc1:

  CC      drivers/leds/led-core.o
  CC      drivers/leds/led-class.o
  LD      drivers/leds/built-in.o
  CC      drivers/net/tg3.o
drivers/net/tg3.c: In function 'tg3_request_firmware':
drivers/net/tg3.c:7542: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:7544: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:7557: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:7562: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c: In function 'tg3_open':
drivers/net/tg3.c:7571: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c: In function 'tg3_init_one':
drivers/net/tg3.c:13127: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:13143: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:13145: error: 'struct tg3' has no member named 'fw_needed'
drivers/net/tg3.c:13148: error: 'struct tg3' has no member named 'fw_needed'
make[2]: *** [drivers/net/tg3.o] Error 1
make[1]: *** [drivers/net] Error 2
make: *** [drivers] Error 2
[asuardi@sandman linux-2.6.29-rc1]$


Indeed, struct tg3 in tg3.h doesn't have fw_needed here...


--alessandro

 "Sun keeps rising in the west / I keep on waking fully confused"

   (The Replacements, "Within Your Reach")

On Sun, 2009-01-11 at 17:42 +0100, Alessandro Suardi wrote:

Patches cleanly but doesn't build in 2.6.29-rc1:

Pants; sorry, I omitted this bit...

diff --git a/drivers/net/tg3.h b/drivers/net/tg3.h
index ae5da60..508def3 100644
--- a/drivers/net/tg3.h
+++ b/drivers/net/tg3.h

@@ -2764,6 +2764,7 @@ struct tg3 {
 	struct ethtool_coalesce		coal;
 
 	/* firmware info */
+	const char			*fw_needed;
 	const struct firmware		*fw;
 	u32				fw_len; /* includes BSS */
 };

-- 
dwmw2

On Sun, Jan 11, 2009 at 5:53 PM, David Woodhouse [off-list ref] wrote:

This one works, thank you.

However, with the combination of in-kernel tg3 and
 CONFIG_IN_KERNEL_FIRMWARE=n there is at least a 30" delay
 during boot while waiting for the firmware request (which will fail).

Such wait didn't use to be there, and stuff worked. Could the wait
 be removed, to behave as it used to - as well ?

Thanks again, ciao,

--alessandro

 "Sun keeps rising in the west / I keep on waking fully confused"

   (The Replacements, "Within Your Reach")

On Sun, 2009-01-11 at 20:24 +0100, Alessandro Suardi wrote:

However, with the combination of in-kernel tg3 and
 CONFIG_IN_KERNEL_FIRMWARE=n there is at least a 30" delay
 during boot while waiting for the firmware request (which will fail).

Hm, ideally it should fail fast if the root file system isn't yet
mounted. I thought we'd seen and fixed a 'long delay' problem already.

I'll take another look.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 12:24:58 +0000

Other drivers will load the firmware later, at the time the device is
brought up. This means that even if you build the driver into the kernel
without its firmware, it can still request the firmware later, when you
try to start _using_ it. And when the file system is available.

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

To me, device probe is in fact the place to fail firmware discovery
for networking devices.  Because such a failure can mean you can't
mount your root filesystem.

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 12:59:59 +0000

So if you build with CONFIG_TIGON3=y, CONFIG_FIRMWARE_IN_KERNEL=n, you
should see it fail to load the firmware at boot, but then it should load
it successfully when you bring the device up.

And you won't be able to mount an NFS root filesystem.

On Sun, 2009-01-11 at 13:39 -0800, David Miller wrote:

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

The option to build arbitrary firmware into your kernel is never going
to be pulled. You can do nfsroot on your myri10ge today, without initrd,
even though the firmware is distributed separately from the kernel. That
doesn't prevent you from building it _in_ to your kernel, if you choose
to.

-- 
dwmw2

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 22:02:27 +0000

On Sun, 2009-01-11 at 13:39 -0800, David Miller wrote:

quoted

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

The option to build arbitrary firmware into your kernel is never going
to be pulled. You can do nfsroot on your myri10ge today, without initrd,
even though the firmware is distributed separately from the kernel. That
doesn't prevent you from building it _in_ to your kernel, if you choose
to.

So what used to work out of the box by typing make now
will have all kinds of strange depencies, right?

This is a regression, no matter how you spin it, in my
book.

On Sun, 2009-01-11 at 13:39 -0800, David Miller wrote:

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

Of course, we've been talking for ages about pulling the dhcp and
nfsroot hacks from the kernel and requiring an initrd for that _anyway_,
but that's a different issue...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 22:15:54 +0000

On Sun, 2009-01-11 at 13:39 -0800, David Miller wrote:

quoted

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

Of course, we've been talking for ages about pulling the dhcp and
nfsroot hacks from the kernel and requiring an initrd for that _anyway_,
but that's a different issue...

It's not there now, so in the mean time....

On Sun, 2009-01-11 at 14:05 -0800, David Miller wrote:

So what used to work out of the box by typing make now
will have all kinds of strange depencies, right?

This is a regression, no matter how you spin it, in my
book.

I know you think that, but I disagree. Yes, it's slightly less
convenient for a small handful of older drivers, but in the general case
it's a massive improvement -- because now we can have a single canonical
location to collect _all_ the firmware that is redistributable, for use
with the kernel. And we've already started adding a whole bunch of new
firmwares to that repository.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

On Sun, Jan 11, 2009 at 10:41:38PM +0000, David Woodhouse wrote:

On Sun, 2009-01-11 at 14:05 -0800, David Miller wrote:

quoted

So what used to work out of the box by typing make now
will have all kinds of strange depencies, right?

This is a regression, no matter how you spin it, in my
book.

I know you think that, but I disagree. Yes, it's slightly less
convenient for a small handful of older drivers,

We're speaking about the driver for the on-board NIC which is present on
almost 100% of the servers built since the last 5 years. There are an
awful lot of users of this driver playing with PXE, nfsroot and other
non-legacy boot methods. So "small handful" or not, we must ensure not
to annoy a lot of users with strange bugs and awkward configurations.

but in the general case
it's a massive improvement -- because now we can have a single canonical
location to collect _all_ the firmware that is redistributable, for use
with the kernel. And we've already started adding a whole bunch of new
firmwares to that repository.

In the past, the firmware distributed with the driver was always the
one known to work with that version of the driver. Now you'll have to
know what version of the firmware to use with what kernel version,
this is adding a new dimension to troubleshooting complexity.

I still think that these firmware hacks are a solution looking for
a problem to solve. We had something which has worked well for 15
years and now it's easy to see a machine hang at boot time for 60
seconds because a firmware file is missing or invalid.

Regression IMO, I agree with Davem.

Willy

On Sun, Jan 11, 2009 at 01:39:29PM -0800, David Miller wrote:

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 12:24:58 +0000

quoted

Other drivers will load the firmware later, at the time the device is
brought up. This means that even if you build the driver into the kernel
without its firmware, it can still request the firmware later, when you
try to start _using_ it. And when the file system is available.

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

To me, device probe is in fact the place to fail firmware discovery
for networking devices.  Because such a failure can mean you can't
mount your root filesystem.

David, is this the direction you want to take the fix?  I'm sitting on
a patch that elaborates on David Woodhouse's work which moves the
request_firmware call to tg3_open().  Before I posted it for comment,
I wanted to make sure the patch is moving in the right direction.

From: "Matt Carlson" <redacted>
Date: Tue, 13 Jan 2009 10:39:48 -0800

On Sun, Jan 11, 2009 at 01:39:29PM -0800, David Miller wrote:

quoted

From: David Woodhouse <dwmw2@infradead.org>
Date: Sun, 11 Jan 2009 12:24:58 +0000

quoted

Other drivers will load the firmware later, at the time the device is
brought up. This means that even if you build the driver into the kernel
without its firmware, it can still request the firmware later, when you
try to start _using_ it. And when the file system is available.

For example, for mounting an NFS root using that device.... Oh, will I
need an initramfs for that once you pull the firmware-in-kernel
option?

To me, device probe is in fact the place to fail firmware discovery
for networking devices.  Because such a failure can mean you can't
mount your root filesystem.

David, is this the direction you want to take the fix?  I'm sitting on
a patch that elaborates on David Woodhouse's work which moves the
request_firmware call to tg3_open().  Before I posted it for comment,
I wanted to make sure the patch is moving in the right direction.

You can post it, sure.

But it doesn't actually fix the nfsroot case, that will still be
broken.