[PATCH 0/6] firmware: dynamic firmware id allocation

[PATCH 0/6] firmware: dynamic firmware id allocation

[Samuel Ortiz]

Hi Greg,

This 6 patches patch set allows for a removal of the firmware name length
restriction from the firmware class API.
Currently we're limited to 30 characters for firmware names because of a
statically allocated 30 bytes string in the firmware class code. We also want
to remove the FIRMWARE_NAME_MAX definition to prevent future drivers from
referencing it.

Patch #1 removes that restriction by dynamically allocating the firmware id.
Patches #2,3,4,5 remove all FIRMWARE_NAME_MAX references from the kernel code.
Patch #6 gets rid of the FIRMWARE_NAME_MAX definition, and should be applied
only once all previous patches from this patch set are applied.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 1/6] firmware: allocate firmware id dynamically

[Samuel Ortiz]

[-- Attachment #1: firmware_class.patch --]
[-- Type: text/plain, Size: 2199 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>

The firmware loader has a statically allocated 30 bytes long string for the
firmware id (a.k.a. the firmware file name). There is no reason why we couldnt
allocate it dynamically, and avoid having restrictions on the firmware names
lengths.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Cc: Marcel Holtmann <holtmann@linux.intel.com>
Cc: Zhu Yi <yi.zhu@intel.com>,
Cc: John Linville <linville@tuxdriver.com>

---
 drivers/base/firmware_class.c |   16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Index: iwm-2.6/drivers/base/firmware_class.c
===================================================================
--- iwm-2.6.orig/drivers/base/firmware_class.c	2009-05-26 16:30:41.000000000 +0200
+++ iwm-2.6/drivers/base/firmware_class.c	2009-05-26 16:37:20.000000000 +0200
@@ -40,7 +40,7 @@ static int loading_timeout = 60;	/* In s
 static DEFINE_MUTEX(fw_lock);
 
 struct firmware_priv {
-	char fw_id[FIRMWARE_NAME_MAX];
+	char *fw_id;
 	struct completion completion;
 	struct bin_attribute attr_data;
 	struct firmware *fw;
@@ -278,6 +278,7 @@ static void fw_dev_release(struct device
 {
 	struct firmware_priv *fw_priv = dev_get_drvdata(dev);
 
+	kfree(fw_priv->fw_id);
 	kfree(fw_priv);
 	kfree(dev);
 
@@ -309,7 +310,13 @@ static int fw_register_device(struct dev
 
 	init_completion(&fw_priv->completion);
 	fw_priv->attr_data = firmware_attr_data_tmpl;
-	strlcpy(fw_priv->fw_id, fw_name, FIRMWARE_NAME_MAX);
+	fw_priv->fw_id = kstrdup(fw_name, GFP_KERNEL);
+	if (!fw_priv->fw_id) {
+		dev_err(device, "%s: Firmware name allocation failed\n",
+			__func__);
+		retval = -ENOMEM;
+		goto error_kfree;
+	}
 
 	fw_priv->timeout.function = firmware_class_timeout;
 	fw_priv->timeout.data = (u_long) fw_priv;
@@ -323,11 +330,14 @@ static int fw_register_device(struct dev
 	retval = device_register(f_dev);
 	if (retval) {
 		dev_err(device, "%s: device_register failed\n", __func__);
-		goto error_kfree;
+		goto error_kfree_fw_id;
 	}
 	*dev_p = f_dev;
 	return 0;
 
+error_kfree_fw_id:
+	kfree(fw_priv->fw_id);
+
 error_kfree:
 	kfree(fw_priv);
 	kfree(f_dev);

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 2/6] atm/ueagle-atm: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

[-- Attachment #1: firmware-ueagle.patch --]
[-- Type: text/plain, Size: 1988 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>
To: Damien Bergamini <damien.bergamini@free.fr>

We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
firmware name length restriction.
This patch replaces the shared FIRMWARE_NAME_MAX definition with a ueagle
local one.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

---
 drivers/usb/atm/ueagle-atm.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: iwm-2.6/drivers/usb/atm/ueagle-atm.c
===================================================================
--- iwm-2.6.orig/drivers/usb/atm/ueagle-atm.c	2009-05-26 17:11:36.000000000 +0200
+++ iwm-2.6/drivers/usb/atm/ueagle-atm.c	2009-05-26 17:11:44.000000000 +0200
@@ -306,6 +306,7 @@ enum {
 #define FW_GET_BYTE(p)	*((__u8 *) (p))
 
 #define FW_DIR "ueagle-atm/"
+#define UEA_FW_NAME_MAX 30
 #define NB_MODEM 4
 
 #define BULK_TIMEOUT 300
@@ -1564,9 +1565,9 @@ static void cmvs_file_name(struct uea_so
 		file = cmv_file[sc->modem_index];
 
 	strcpy(cmv_name, FW_DIR);
-	strlcat(cmv_name, file, FIRMWARE_NAME_MAX);
+	strlcat(cmv_name, file, UEA_FW_NAME_MAX);
 	if (ver == 2)
-		strlcat(cmv_name, ".v2", FIRMWARE_NAME_MAX);
+		strlcat(cmv_name, ".v2", UEA_FW_NAME_MAX);
 }
 
 static int request_cmvs_old(struct uea_softc *sc,
@@ -1574,7 +1575,7 @@ static int request_cmvs_old(struct uea_s
 {
 	int ret, size;
 	u8 *data;
-	char cmv_name[FIRMWARE_NAME_MAX]; /* 30 bytes stack variable */
+	char cmv_name[UEA_FW_NAME_MAX]; /* 30 bytes stack variable */
 
 	cmvs_file_name(sc, cmv_name, 1);
 	ret = request_firmware(fw, cmv_name, &sc->usb_dev->dev);
@@ -1608,7 +1609,7 @@ static int request_cmvs(struct uea_softc
 	int ret, size;
 	u32 crc;
 	u8 *data;
-	char cmv_name[FIRMWARE_NAME_MAX]; /* 30 bytes stack variable */
+	char cmv_name[UEA_FW_NAME_MAX]; /* 30 bytes stack variable */
 
 	cmvs_file_name(sc, cmv_name, 2);
 	ret = request_firmware(fw, cmv_name, &sc->usb_dev->dev);

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 3/6] tuners/xc2028: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

[-- Attachment #1: firmware-xc202.patch --]
[-- Type: text/plain, Size: 1193 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>
To: Michel Ludwig <michel.ludwig@gmail.com>

We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
firmware name length restriction.
This patch gets rid of the xc2028 FIRMWARE_NAME_MAX reference.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
---
 drivers/media/common/tuners/tuner-xc2028.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: iwm-2.6/drivers/media/common/tuners/tuner-xc2028.c
===================================================================
--- iwm-2.6.orig/drivers/media/common/tuners/tuner-xc2028.c	2009-05-26 17:15:54.000000000 +0200
+++ iwm-2.6/drivers/media/common/tuners/tuner-xc2028.c	2009-05-26 17:19:01.000000000 +0200
@@ -48,7 +48,7 @@ MODULE_PARM_DESC(audio_std,
 	"NICAM/A\n"
 	"NICAM/B\n");
 
-static char firmware_name[FIRMWARE_NAME_MAX];
+static char firmware_name[30];
 module_param_string(firmware_name, firmware_name, sizeof(firmware_name), 0);
 MODULE_PARM_DESC(firmware_name, "Firmware file name. Allows overriding the "
 				"default firmware name\n");

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 4/6] dvb/dvb-usb: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

[-- Attachment #1: firmware-dvb-usb.patch --]
[-- Type: text/plain, Size: 1092 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>
To: Mauro Carvalho Chehab <mchehab@infradead.org>

We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
firmware name length restriction.
This patch changes the dvb_usb_device_properties firmware field accordingly.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

---
 drivers/media/dvb/dvb-usb/dvb-usb.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h
===================================================================
--- iwm-2.6.orig/drivers/media/dvb/dvb-usb/dvb-usb.h	2009-05-26 17:24:36.000000000 +0200
+++ iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h	2009-05-26 17:25:19.000000000 +0200
@@ -196,7 +196,7 @@ struct dvb_usb_device_properties {
 #define CYPRESS_FX2     3
 	int        usb_ctrl;
 	int        (*download_firmware) (struct usb_device *, const struct firmware *);
-	const char firmware[FIRMWARE_NAME_MAX];
+	const char *firmware;
 	int        no_reconnect;
 
 	int size_of_priv;

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 5/6] pcmcia/ds: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

[-- Attachment #1: firmware-ds.patch --]
[-- Type: text/plain, Size: 1593 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>
To: Dominik Brodowski <linux@dominikbrodowski.net>

We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
firmware name length restriction.
With the FIRMWARE_NAME_MAX removal, the ds.c reference becomes useless as we
dont need to check for the firmware name length anymore.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

---
 drivers/pcmcia/ds.c |   12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

Index: iwm-2.6/drivers/pcmcia/ds.c
===================================================================
--- iwm-2.6.orig/drivers/pcmcia/ds.c	2009-05-26 17:32:21.000000000 +0200
+++ iwm-2.6/drivers/pcmcia/ds.c	2009-05-26 17:33:07.000000000 +0200
@@ -828,7 +828,6 @@ static int pcmcia_load_firmware(struct p
 {
 	struct pcmcia_socket *s = dev->socket;
 	const struct firmware *fw;
-	char path[FIRMWARE_NAME_MAX];
 	int ret = -ENOMEM;
 	int no_funcs;
 	int old_funcs;
@@ -839,16 +838,7 @@ static int pcmcia_load_firmware(struct p
 
 	ds_dev_dbg(1, &dev->dev, "trying to load CIS file %s\n", filename);
 
-	if (strlen(filename) > (FIRMWARE_NAME_MAX - 1)) {
-		dev_printk(KERN_WARNING, &dev->dev,
-			   "pcmcia: CIS filename is too long [%s]\n",
-			   filename);
-		return -EINVAL;
-	}
-
-	snprintf(path, sizeof(path), "%s", filename);
-
-	if (request_firmware(&fw, path, &dev->dev) == 0) {
+	if (request_firmware(&fw, filename, &dev->dev) == 0) {
 		if (fw->size >= CISTPL_MAX_CIS_SIZE) {
 			ret = -EINVAL;
 			dev_printk(KERN_ERR, &dev->dev,

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH 6/6] firmware: FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

[-- Attachment #1: firmware-no_max.patch --]
[-- Type: text/plain, Size: 843 bytes --]

From: Samuel Ortiz <sameo@linux.intel.com>

As we're allocating the firmware name dynamically, we no longer need this
definition.
This patch must be applied only after the 5 previous patches from this pacth
set have been applied.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

---
 include/linux/firmware.h |    1 -
 1 file changed, 1 deletion(-)

Index: iwm-2.6/include/linux/firmware.h
===================================================================
--- iwm-2.6.orig/include/linux/firmware.h	2009-05-26 17:44:30.000000000 +0200
+++ iwm-2.6/include/linux/firmware.h	2009-05-26 17:44:34.000000000 +0200
@@ -5,7 +5,6 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 
-#define FIRMWARE_NAME_MAX 30 
 #define FW_ACTION_NOHOTPLUG 0
 #define FW_ACTION_HOTPLUG 1
 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 4/6] dvb/dvb-usb: prepare for FIRMWARE_NAME_MAX removal

[Michael Krufky]

On Tue, May 26, 2009 at 1:40 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> From: Samuel Ortiz <sameo@linux.intel.com>
> To: Mauro Carvalho Chehab <mchehab@infradead.org>
>
> We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
> firmware name length restriction.
> This patch changes the dvb_usb_device_properties firmware field accordingly.
>
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
>
> ---
>  drivers/media/dvb/dvb-usb/dvb-usb.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Index: iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h
> ===================================================================
> --- iwm-2.6.orig/drivers/media/dvb/dvb-usb/dvb-usb.h    2009-05-26 17:24:36.000000000 +0200
> +++ iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h 2009-05-26 17:25:19.000000000 +0200
> @@ -196,7 +196,7 @@ struct dvb_usb_device_properties {
>  #define CYPRESS_FX2     3
>        int        usb_ctrl;
>        int        (*download_firmware) (struct usb_device *, const struct firmware *);
> -       const char firmware[FIRMWARE_NAME_MAX];
> +       const char *firmware;
>        int        no_reconnect;
>
>        int size_of_priv;
>
> --
> Intel Open Source Technology Centre
> http://oss.intel.com/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Samuel,

Your patch makes the following change:

-       const char firmware[FIRMWARE_NAME_MAX];
+       const char *firmware;

Before your change, struct dvb_usb_device_properties actually contains
memory allocated for the firmware filename.  After your change, this
is nothing but a pointer.

This will cause an OOPS.

Regards,

Mike Krufky

Re: [PATCH 4/6] dvb/dvb-usb: prepare for FIRMWARE_NAME_MAX removal

[Samuel Ortiz]

On Tue, May 26, 2009 at 02:32:45PM -0400, Michael Krufky wrote:
> On Tue, May 26, 2009 at 1:40 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> > From: Samuel Ortiz <sameo@linux.intel.com>
> > To: Mauro Carvalho Chehab <mchehab@infradead.org>
> >
> > We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
> > firmware name length restriction.
> > This patch changes the dvb_usb_device_properties firmware field accordingly.
> >
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> >
> > ---
> >  drivers/media/dvb/dvb-usb/dvb-usb.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > Index: iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h
> > ===================================================================
> > --- iwm-2.6.orig/drivers/media/dvb/dvb-usb/dvb-usb.h    2009-05-26 17:24:36.000000000 +0200
> > +++ iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h 2009-05-26 17:25:19.000000000 +0200
> > @@ -196,7 +196,7 @@ struct dvb_usb_device_properties {
> >  #define CYPRESS_FX2     3
> >        int        usb_ctrl;
> >        int        (*download_firmware) (struct usb_device *, const struct firmware *);
> > -       const char firmware[FIRMWARE_NAME_MAX];
> > +       const char *firmware;
> >        int        no_reconnect;
> >
> >        int size_of_priv;
> >
> > --
> > Intel Open Source Technology Centre
> > http://oss.intel.com/
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> 
> Samuel,
> 
> Your patch makes the following change:
> 
> -       const char firmware[FIRMWARE_NAME_MAX];
> +       const char *firmware;
> 
> Before your change, struct dvb_usb_device_properties actually contains
> memory allocated for the firmware filename.  After your change, this
> is nothing but a pointer.
> 
> This will cause an OOPS.
No, not if it's correctly initialized, as it seems to be for all the
dvb_usb_device_properties users right now.
Typically, you'd initialize your dvb_usb_device_properties like this:

static struct dvb_usb_device_properties a800_properties = {
        .caps = DVB_USB_IS_AN_I2C_ADAPTER,

        .usb_ctrl = CYPRESS_FX2,
        .firmware = "dvb-usb-avertv-a800-02.fw",
[...]

And that's fine.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 4/6] dvb/dvb-usb: prepare for FIRMWARE_NAME_MAX removal

[Michael Krufky]

On Tue, May 26, 2009 at 2:42 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
> On Tue, May 26, 2009 at 02:32:45PM -0400, Michael Krufky wrote:
>> On Tue, May 26, 2009 at 1:40 PM, Samuel Ortiz <sameo@linux.intel.com> wrote:
>> > From: Samuel Ortiz <sameo@linux.intel.com>
>> > To: Mauro Carvalho Chehab <mchehab@infradead.org>
>> >
>> > We're going to remove the FIRMWARE_NAME_MAX definition in order to avoid any
>> > firmware name length restriction.
>> > This patch changes the dvb_usb_device_properties firmware field accordingly.
>> >
>> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
>> >
>> > ---
>> >  drivers/media/dvb/dvb-usb/dvb-usb.h |    2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > Index: iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h
>> > ===================================================================
>> > --- iwm-2.6.orig/drivers/media/dvb/dvb-usb/dvb-usb.h    2009-05-26 17:24:36.000000000 +0200
>> > +++ iwm-2.6/drivers/media/dvb/dvb-usb/dvb-usb.h 2009-05-26 17:25:19.000000000 +0200
>> > @@ -196,7 +196,7 @@ struct dvb_usb_device_properties {
>> >  #define CYPRESS_FX2     3
>> >        int        usb_ctrl;
>> >        int        (*download_firmware) (struct usb_device *, const struct firmware *);
>> > -       const char firmware[FIRMWARE_NAME_MAX];
>> > +       const char *firmware;
>> >        int        no_reconnect;
>> >
>> >        int size_of_priv;
>> >
>> > --
>> > Intel Open Source Technology Centre
>> > http://oss.intel.com/
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> > Please read the FAQ at  http://www.tux.org/lkml/
>> >
>>
>> Samuel,
>>
>> Your patch makes the following change:
>>
>> -       const char firmware[FIRMWARE_NAME_MAX];
>> +       const char *firmware;
>>
>> Before your change, struct dvb_usb_device_properties actually contains
>> memory allocated for the firmware filename.  After your change, this
>> is nothing but a pointer.
>>
>> This will cause an OOPS.
> No, not if it's correctly initialized, as it seems to be for all the
> dvb_usb_device_properties users right now.
> Typically, you'd initialize your dvb_usb_device_properties like this:
>
> static struct dvb_usb_device_properties a800_properties = {
>        .caps = DVB_USB_IS_AN_I2C_ADAPTER,
>
>        .usb_ctrl = CYPRESS_FX2,
>        .firmware = "dvb-usb-avertv-a800-02.fw",
> [...]
>
> And that's fine.

You're right -- there is nothing wrong with the change -- my bad.

I traced though the code after posting that last mail.  It looked
risky when I just looked at the patch, but in the end this is actually
cleaner and much better.

Sorry for the noise.

Acked /
Reviewed-by: Michael Krufky <mkrufky@kernellabs.com>

Re: [PATCH 0/6] firmware: dynamic firmware id allocation

[John W. Linville]

On Tue, May 26, 2009 at 07:40:12PM +0200, Samuel Ortiz wrote:

> This 6 patches patch set allows for a removal of the firmware name length
> restriction from the firmware class API.
> Currently we're limited to 30 characters for firmware names because of a
> statically allocated 30 bytes string in the firmware class code. We also want
> to remove the FIRMWARE_NAME_MAX definition to prevent future drivers from
> referencing it.
> 
> Patch #1 removes that restriction by dynamically allocating the firmware id.
> Patches #2,3,4,5 remove all FIRMWARE_NAME_MAX references from the kernel code.
> Patch #6 gets rid of the FIRMWARE_NAME_MAX definition, and should be applied
> only once all previous patches from this patch set are applied.

You probably want to include "libertas: adapt for dynamic firmware
id allocation" in this series... :-)

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.