[PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[Tim Gardner]

Cc: Jon Mason <jdmason@kudzu.us>
Cc: netdev@vger.kernel.org
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/net/ethernet/neterion/vxge/vxge-main.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c b/drivers/net/ethernet/neterion/vxge/vxge-main.c
index 51387c3..dcef72d 100644
--- a/drivers/net/ethernet/neterion/vxge/vxge-main.c
+++ b/drivers/net/ethernet/neterion/vxge/vxge-main.c
@@ -4856,3 +4856,5 @@ vxge_closer(void)
 }
 module_init(vxge_starter);
 module_exit(vxge_closer);
+MODULE_FIRMWARE("vxge/X3fw-pxe.ncf");
+MODULE_FIRMWARE("vxge/X3fw.ncf");
-- 
1.7.9.5

Re: [PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[Ben Hutchings]

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

On Thu, 2012-04-12 at 14:34 -0600, Tim Gardner wrote:
> Cc: Jon Mason <jdmason@kudzu.us>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
> ---
>  drivers/net/ethernet/neterion/vxge/vxge-main.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c b/drivers/net/ethernet/neterion/vxge/vxge-main.c
> index 51387c3..dcef72d 100644
> --- a/drivers/net/ethernet/neterion/vxge/vxge-main.c
> +++ b/drivers/net/ethernet/neterion/vxge/vxge-main.c
> @@ -4856,3 +4856,5 @@ vxge_closer(void)
>  }
>  module_init(vxge_starter);
>  module_exit(vxge_closer);
> +MODULE_FIRMWARE("vxge/X3fw-pxe.ncf");
> +MODULE_FIRMWARE("vxge/X3fw.ncf");

I don't agree; these firmware files are updates for the flash and only
need to be loaded once.

Also: this driver's behaviour of automatically updating flash without
any confirmation seems quite dangerous.  The driver also isn't usable
after it performs such an update:

	printk(KERN_NOTICE "Upgrade of firmware successful!  Adapter must be "
	       "hard reset before using, thus requiring a system reboot or a "
	       "hotplug event.\n");

So what is the point of integrating firmware update into the driver at
all?

Ben.

-- 
Ben Hutchings
It is easier to change the specification to fit the program than vice versa.

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

Re: [PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[David Miller]

From: Ben Hutchings <ben@decadent.org.uk>
Date: Sun, 15 Apr 2012 14:56:55 +0100

> Also: this driver's behaviour of automatically updating flash without
> any confirmation seems quite dangerous.  The driver also isn't usable
> after it performs such an update:
> 
> 	printk(KERN_NOTICE "Upgrade of firmware successful!  Adapter must be "
> 	       "hard reset before using, thus requiring a system reboot or a "
> 	       "hotplug event.\n");
> 
> So what is the point of integrating firmware update into the driver at
> all?

This is beyond terrible.

Re: [PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[Tim Gardner]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 04/15/2012 07:56 AM, Ben Hutchings wrote:
> On Thu, 2012-04-12 at 14:34 -0600, Tim Gardner wrote:
>> Cc: Jon Mason <jdmason@kudzu.us> Cc: netdev@vger.kernel.org 
>> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 
>> drivers/net/ethernet/neterion/vxge/vxge-main.c |    2 ++ 1 file
>> changed, 2 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c
>> b/drivers/net/ethernet/neterion/vxge/vxge-main.c index
>> 51387c3..dcef72d 100644 ---
>> a/drivers/net/ethernet/neterion/vxge/vxge-main.c +++
>> b/drivers/net/ethernet/neterion/vxge/vxge-main.c @@ -4856,3
>> +4856,5 @@ vxge_closer(void) } module_init(vxge_starter); 
>> module_exit(vxge_closer); +MODULE_FIRMWARE("vxge/X3fw-pxe.ncf"); 
>> +MODULE_FIRMWARE("vxge/X3fw.ncf");
> 
> I don't agree; these firmware files are updates for the flash and
> only need to be loaded once.
> 
> Also: this driver's behaviour of automatically updating flash
> without any confirmation seems quite dangerous.  The driver also
> isn't usable after it performs such an update:
> 
> printk(KERN_NOTICE "Upgrade of firmware successful!  Adapter must
> be " "hard reset before using, thus requiring a system reboot or a
> " "hotplug event.\n");
> 
> So what is the point of integrating firmware update into the driver
> at all?
> 
> Ben.
> 

I guess I'm confused about use of the MODULE_FIRMWARE() macro. I
thought it merely described the names of the firmware files that were
actually used by the driver and had no run-time impact. Regardless of
whether firmware files are used on every load, why _not_ describe them
to modinfo ?

I'm auditing the Ubuntu linux-firmware package to reduce size by
removing obsolete firmware files. Along the way I'm also trying to
update the drivers that have caught my attention in their use of
MODULE_FIRMWARE.

rtg
- -- 
Tim Gardner tim.gardner@canonical.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCgAGBQJPjA7cAAoJED12yEX6FEfKqWEQAJHHf/g0yuJg5eJ7XSdCJWbV
6xs5NNj3Wwo2bN578PnwK28grBFT6atDg6Y0KkcMgZ/NCc7Q8GN/7yJaK+VcW6wQ
395JNYf67bFx+6B+MDVj2qPHa/2EJjYGjZlxMzPPIKUqYOzHt18A779Tb5DLWelj
B1DAJJcTDVF1jyAEB/4zCDq1R39jARWGDzC11OqrQqEmBqbE2z5CgLeDECR0uDsg
axyIW4Mc+nSF1SrrmvdtXfHzDPN+wpXVoGTjb83iqBLWSkKo8QYQDQLnc67mZgAa
lT+ZdFIfAY8vE/PmfokX+xkCc7Dk1B36fIuwWEIRM4QUgFp0skXHUyr8n3xDRLiD
+Kcb3IMIIprzlPi7zpEwB0ulubyjKdh8+dCwlHZVLmRt/QgXUyLCQJG6vqg6WlBO
T53xZ24JcwmdSASYDMTxWmEc3ERq33b1uKPfrUGTLENdyt4F5yU1KT0HXmkJ8Chq
/wQLX9fAC3janMKJP4fdQvox/WBAihZ4wIBNUKnCYl01XXCDvy0FnOtxk3ZGPHzv
g1tS8U2pJUuktX74U1p4ltrKQXhW3z4Oro5BdLTqNunlXDqmT0kiBVkLbJmDNzwK
mL7tlcx8Nn28WRYUM+MW7J1C0+tVRaVMtF8dW1ICduhzzPy5KSarI23SFlExoQ5s
Kn56ELI/wV/ajx+z2pTk
=mobO
-----END PGP SIGNATURE-----

Re: [PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[Ben Hutchings]

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

On Mon, 2012-04-16 at 06:21 -0600, Tim Gardner wrote:
> On 04/15/2012 07:56 AM, Ben Hutchings wrote:
> > On Thu, 2012-04-12 at 14:34 -0600, Tim Gardner wrote:
> >> Cc: Jon Mason <jdmason@kudzu.us> Cc: netdev@vger.kernel.org 
> >> Signed-off-by: Tim Gardner <tim.gardner@canonical.com> --- 
> >> drivers/net/ethernet/neterion/vxge/vxge-main.c |    2 ++ 1 file
> >> changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/net/ethernet/neterion/vxge/vxge-main.c
> >> b/drivers/net/ethernet/neterion/vxge/vxge-main.c index
> >> 51387c3..dcef72d 100644 ---
> >> a/drivers/net/ethernet/neterion/vxge/vxge-main.c +++
> >> b/drivers/net/ethernet/neterion/vxge/vxge-main.c @@ -4856,3
> >> +4856,5 @@ vxge_closer(void) } module_init(vxge_starter); 
> >> module_exit(vxge_closer); +MODULE_FIRMWARE("vxge/X3fw-pxe.ncf"); 
> >> +MODULE_FIRMWARE("vxge/X3fw.ncf");
> > 
> > I don't agree; these firmware files are updates for the flash and
> > only need to be loaded once.
> > 
> > Also: this driver's behaviour of automatically updating flash
> > without any confirmation seems quite dangerous.  The driver also
> > isn't usable after it performs such an update:
> > 
> > printk(KERN_NOTICE "Upgrade of firmware successful!  Adapter must
> > be " "hard reset before using, thus requiring a system reboot or a
> > " "hotplug event.\n");
> > 
> > So what is the point of integrating firmware update into the driver
> > at all?
> > 
> > Ben.
> > 
> 
> I guess I'm confused about use of the MODULE_FIRMWARE() macro. I
> thought it merely described the names of the firmware files that were
> actually used by the driver and had no run-time impact. Regardless of
> whether firmware files are used on every load, why _not_ describe them
> to modinfo ?
[...]

Ah, that's a good question.  Quoting my own interpretation from
<1257629601.15927.361.camel@localhost>:

> Drivers that must load 'firmware' into the devices they drive should 
> declare the names of the files they will request, using the
> MODULE_FIRMWARE() macro.  This enables other tools to discover these
> dependencies statically, and warn the user if firmware files are
> missing.

In Debian we use this to decide which files need to be copied into an
initramfs.  You use that too unless you've changed this feature of
initramfs-tools.  We warn when building an initramfs and during a major
kernel upgrade if it looks like a driver will be used and the
corresponding firmware isn't installed.

I also have an (unfinished) patch that will use this information for
CONFIG_FIRMWARE_IN_KERNEL.

In this case the firmware files are used to upgrade old flash, but since
the vendor has closed down there aren't going to be any further updates.
So the likelihood of the files actually being needed by the driver is
very small.

Ben.

-- 
Ben Hutchings
This sentence contradicts itself - no actually it doesn't.

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

Re: [PATCH net-next] net: vxge: Add MODULE_FIRMWARE

[Tim Gardner]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 04/16/2012 08:29 AM, Ben Hutchings wrote:
>> 
>> I guess I'm confused about use of the MODULE_FIRMWARE() macro. I 
>> thought it merely described the names of the firmware files that
>> were actually used by the driver and had no run-time impact.
>> Regardless of whether firmware files are used on every load, why
>> _not_ describe them to modinfo ?
> [...]
> 
> Ah, that's a good question.  Quoting my own interpretation from 
> <1257629601.15927.361.camel@localhost>:
> 
>> Drivers that must load 'firmware' into the devices they drive
>> should declare the names of the files they will request, using
>> the MODULE_FIRMWARE() macro.  This enables other tools to
>> discover these dependencies statically, and warn the user if
>> firmware files are missing.
> 
> In Debian we use this to decide which files need to be copied into
> an initramfs.  You use that too unless you've changed this feature
> of initramfs-tools.  We warn when building an initramfs and during
> a major kernel upgrade if it looks like a driver will be used and
> the corresponding firmware isn't installed.
> 

Yep, we've merged "initramfs-tools (0.99) unstable" into precise. I'll
assume it behaves as described. The Ubuntu server installer will
certainly annoy you if firmware is missing. Its likely the LiveCD will
as well.

> I also have an (unfinished) patch that will use this information
> for CONFIG_FIRMWARE_IN_KERNEL.
> 
> In this case the firmware files are used to upgrade old flash, but
> since the vendor has closed down there aren't going to be any
> further updates. So the likelihood of the files actually being
> needed by the driver is very small.
> 
> Ben.
> 

I didn't realize the vendor is defunct, so I suddenly don't care as much.

rtg
- -- 
Tim Gardner tim.gardner@canonical.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBCgAGBQJPjDIXAAoJED12yEX6FEfKRE4P/j7zqPOeom4wwocrn4zQrU/6
BW5BTUpea9pnC7dgIp/Vl/HgO+5WrdApBrcONpfJeHE+1bBM1gx6NANLu2qPx3BG
EqMUI+I7bAbwFqUQnDs1wgqfat5CKTGfmf8lwjTzbmRVMlPRMfCGZWlfSL/Pi9Tn
5R8M4TxbspOkI0+iRmcjlOiRVAYuTglaiQD9r0cqyRQeJ+jQ5RK/uwCNyTKHh3eB
LmeXj7RU/gbUiXZkqh6HEtT9QqhOL2BYHboVjHnKGcdvLylqaLNiwcWKLOrQ4bxy
q3GuLtEmAb7fV5stONhyoqrDBfvWGNtbOSmqW4yXIeKOT3S4/UOs+LrrIXItgLgD
qpBW9xBNjT+K+DkTh8iTpCF39igyMG+PvPJ3bjxEznPZoYU5TLJLPrw6YDUnfe54
lFUMdnAVHZmgdF9uUrakNacwPXkdTd3t4F+Y+sPLxaHYyOC/+hrXnkj9KLxlVjYr
RP82iHhINcNgEl5Rqj98Kp0kw8vsVBuDm5i5LvaRLlah7PNWJ/tSbFXwG+qC0VZK
BkVSatVyLgWDOJAN4NYgPME9/IAV+Mmlu7ZOIN/8oyRiMaK4HPsDDSLYhwthmXN3
r9ViAJTdGCBf+FTvLS8H3YXbueFuLNoH0nfV4oPB77jrYWesEPEqJlKPqefJAVAG
NZshSCqSm2x4C+2UXuhp
=BYdE
-----END PGP SIGNATURE-----