| Message ID | 1334262882-96973-1-git-send-email-tim.gardner@canonical.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----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----- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
-----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----- -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");
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(+)