Firmware loading problem

DISCUSSION Marcel Holtmann 3 participants 10 messages Jul 22, 2003 – Aug 1, 2003

Share & tools

Marcel Holtmann

author

2003-07-22 14:31 UTC #0 lore raw

Hi,

I installed linux-2.6.0-test1-ac2 and tried to port my driver for the BlueFRITZ! USB Bluetooth dongle to 2.6. This device needs a firmware download and I want to use the new firmware class for getting the firmware file from userspace. After reading the documentation and testing the driver samples I got the results that I expected.

My problem is now that the firmware loader is not working with my firmware file and it seems that this is a problem of the file size, because copying small files through the same interface is working fine. This is the file I want to load:

-rw-r--r-- 1 holtmann staff 418352 Jul 11 12:38 bfubase.frm

I have written my own firmware.agent hotplug script, which looks in general something like this:

echo 1 > $LOADING
cp bfubase.frm $DATA
echo 0 > $LOADING

Loading the above firmware file through this interface results in different behaviours. The results are complete freezes, instant reboots, X server crashes with black screens and sometimes I see an oops about virtual memory, but it goes bye bye too fast to let me do anything useful with it.

Are their any limitations with the sysfs binary file interface?

Regards

Marcel

Manuel Estrada Sainz

2003-07-22 14:40 UTC #1 lore raw

Quoted 22 lines · Marcel Holtmann

Hi,
I installed linux-2.6.0-test1-ac2 and tried to port my driver for the BlueFRITZ! USB Bluetooth dongle to 2.6. This device needs a firmware download and I want to use the new firmware class for getting the firmware file from userspace. After reading the documentation and testing the driver samples I got the results that I expected.
My problem is now that the firmware loader is not working with my firmware file and it seems that this is a problem of the file size, because copying small files through the same interface is working fine. This is the file I want to load:
-rw-r--r-- 1 holtmann staff 418352 Jul 11 12:38 bfubase.frm
I have written my own firmware.agent hotplug script, which looks in general something like this:
echo 1 > $LOADING
cp bfubase.frm $DATA
echo 0 > $LOADING

Quoted 5 lines

Loading the above firmware file through this interface results in different behaviours. The results are complete freezes, instant reboots, X server crashes with black screens and sometimes I see an oops about virtual memory, but it goes bye bye too fast to let me do anything useful with it.

Could you send me a tarball with a sample showing the problem. If possible I would like to do "make test" and have it compile and crash the system appropriately :)

Quoted 1 line

Are their any limitations with the sysfs binary file interface?

Not that I know of. But I am willing to learn :)

Thanks

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Marcel Holtmann

author

2003-07-22 15:23 UTC #2 lore raw

Hi Manuel,

Quoted 26 lines

Quoted 20 lines
I installed linux-2.6.0-test1-ac2 and tried to port my driver for the BlueFRITZ! USB Bluetooth dongle to 2.6. This device needs a firmware download and I want to use the new firmware class for getting the firmware file from userspace. After reading the documentation and testing the driver samples I got the results that I expected.
My problem is now that the firmware loader is not working with my firmware file and it seems that this is a problem of the file size, because copying small files through the same interface is working fine. This is the file I want to load:
-rw-r--r-- 1 holtmann staff 418352 Jul 11 12:38 bfubase.frm
I have written my own firmware.agent hotplug script, which looks in general something like this:
echo 1 > $LOADING
cp bfubase.frm $DATA
echo 0 > $LOADING
Quoted 5 lines
Loading the above firmware file through this interface results in different behaviours. The results are complete freezes, instant reboots, X server crashes with black screens and sometimes I see an oops about virtual memory, but it goes bye bye too fast to let me do anything useful with it.

Quoted 3 lines

Could you send me a tarball with a sample showing the problem. If possible I would like to do "make test" and have it compile and crash the system appropriately :)

I tracked down the problem to request_firmware() or a sysfs problem. With the firmware included in a header file the driver itself works perfect.

Attached is a sample of how I use the request_firmware() and from the documentation it seems correct to me.

Regards

Marcel

/*
*
* Firmware loader test driver
*
* Copyright (C) 2003 Marcel Holtmann <[email protected]> *
*

 *  This program is free software; you can redistribute it and/or modify
 *  it under the terms of the GNU General Public License as published by
 *  the Free Software Foundation; either version 2 of the License, or
 *  (at your option) any later version.
 *
 *  This program is distributed in the hope that it will be useful,
 *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 *  GNU General Public License for more details.
 *
 *  You should have received a copy of the GNU General Public License
 *  along with this program; if not, write to the Free Software
 *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA

*
*/

#include <linux/config.h>
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>

#include <linux/device.h>
#include <linux/firmware.h>

#include <linux/usb.h>

static struct usb_device_id fwldtest_table[] = {
	{ USB_DEVICE(0x057c, 0x2200) },

	{ }	/* Terminating entry */
};

MODULE_DEVICE_TABLE(usb, fwldtest_table);

static int fwldtest_probe(struct usb_interface *iface, const struct usb_device_id *id)

{
	const struct firmware *firmware;
	struct usb_device *udev = interface_to_usbdev(iface);

	if (request_firmware(&firmware, "firmware.bin", &udev->dev) < 0) {
		printk(KERN_ERR "Firmware request failed\n");
		goto error;
	}

	printk(KERN_INFO "firmware data %p size %d\n", firmware->data, firmware->size);

error:

	release_firmware(firmware);

	return -EIO;
}

static void fwldtest_disconnect(struct usb_interface *iface) {
}

static struct usb_driver fwldtest_driver = {
.owner = THIS_MODULE,
.name = "fwldtest",
.probe = fwldtest_probe,
.disconnect = fwldtest_disconnect,
.id_table = fwldtest_table,
};

static int __init fwldtest_init(void)

{
	return usb_register(&fwldtest_driver);
}

static void __exit fwldtest_cleanup(void)

{
	usb_deregister(&fwldtest_driver);
}

module_init(fwldtest_init);
module_exit(fwldtest_cleanup);

MODULE_AUTHOR("Marcel Holtmann <[email protected]>"); MODULE_DESCRIPTION("Firmware loader test driver"); MODULE_LICENSE("GPL");

Manuel Estrada Sainz

2003-07-26 08:50 UTC #3 lore raw

Quoted 28 lines · Marcel Holtmann

Hi Manuel,
Quoted 26 lines
Quoted 25 lines
I installed linux-2.6.0-test1-ac2 and tried to port my driver for the BlueFRITZ! USB Bluetooth dongle to 2.6. This device needs a firmware download and I want to use the new firmware class for getting the firmware file from userspace. After reading the documentation and testing the driver samples I got the results that I expected.
My problem is now that the firmware loader is not working with my firmware file and it seems that this is a problem of the file size, because copying small files through the same interface is working fine. This is the file I want to load:
-rw-r--r-- 1 holtmann staff 418352 Jul 11 12:38 bfubase.frm
I have written my own firmware.agent hotplug script, which looks in general something like this:
echo 1 > $LOADING
cp bfubase.frm $DATA
echo 0 > $LOADING
Loading the above firmware file through this interface results in different behaviours. The results are complete freezes, instant reboots, X server crashes with black screens and sometimes I see an oops about virtual memory, but it goes bye bye too fast to let me do anything useful with it.

Quoted 7 lines

Quoted 3 lines
Could you send me a tarball with a sample showing the problem. If possible I would like to do "make test" and have it compile and crash the system appropriately :)
I tracked down the problem to request_firmware() or a sysfs problem. With the firmware included in a header file the driver itself works perfect.

You are right, the problem was in sysfs, attached goes a patch that WorksForMe (tm), please test and report.

Quoted 2 lines

Attached is a sample of how I use the request_firmware() and from the documentation it seems correct to me.

Not what I was asking for, but it seams OK.

About the patch:
- undo recent change, made in the believe that "buffer" was the size of the whole file, it is just PAGE_SIZE in size.
- Since files are allowed to have unknown sizes, by setting their size to 0, we can't preallocate a buffer of their size on open.

- don't use any offset when handling buffer
- simplifies code
- since it is temporary storage it doesn't really matter

- undo relevant changes to request_firmware() code.

- hopefully adapt drivers/pci/pci-sysfs.c to this changes
- Please double check, I didn't look very carefully on this.

Have a nice day

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Index: drivers/base/firmware_class.c
===================================================================
RCS file: /home/cvs/linux-2.5/drivers/base/firmware_class.c,v retrieving revision 1.3

base/firmware_class.c +2 -2

diff -u -r1.3 firmware_class.c
@@ -149,7 +149,7 @@
 	if (offset + count > fw->size)
 		count = fw->size - offset;
 
-	memcpy(buffer + offset, fw->data + offset, count);
+	memcpy(buffer, fw->data + offset, count);
 	return count;
 }
 static int

@@ -198,7 +198,7 @@
 	if (retval)
 		return retval;
 
-	memcpy(fw->data + offset, buffer + offset, count);
+	memcpy(fw->data + offset, buffer, count);
 
 	fw->size = max_t(size_t, offset + count, fw->size);

Index: drivers/pci/pci-sysfs.c
===================================================================
RCS file: /home/cvs/linux-2.5/drivers/pci/pci-sysfs.c,v retrieving revision 1.6

pci/pci-sysfs.c +12 ~~-12~~

diff -u -r1.6 pci-sysfs.c
@@ -87,7 +87,7 @@
 	while (off & 3) {
 		unsigned char val;
 		pci_read_config_byte(dev, off, &val);
-		buf[off] = val;
+		buf[0] = val;
 		off++;
 		if (--size == 0)
 			break;

@@ -96,10 +96,10 @@
 	while (size > 3) {
 		unsigned int val;
 		pci_read_config_dword(dev, off, &val);
-		buf[off] = val & 0xff;
-		buf[off + 1] = (val >> 8) & 0xff;
-		buf[off + 2] = (val >> 16) & 0xff;
-		buf[off + 3] = (val >> 24) & 0xff;
+		buf[0] = val & 0xff;
+		buf[1] = (val >> 8) & 0xff;
+		buf[2] = (val >> 16) & 0xff;
+		buf[3] = (val >> 24) & 0xff;
 		off += 4;
 		size -= 4;
 	}

@@ -107,7 +107,7 @@
 	while (size > 0) {
 		unsigned char val;
 		pci_read_config_byte(dev, off, &val);
-		buf[off] = val;
+		buf[0] = val;
 		off++;
 		--size;
 	}

@@ -129,24 +129,24 @@
 	}
 
 	while (off & 3) {
-		pci_write_config_byte(dev, off, buf[off]);
+		pci_write_config_byte(dev, off, buf[0]);
 		off++;
 		if (--size == 0)
 			break;
 	}
 
 	while (size > 3) {
-		unsigned int val = buf[off];
-		val |= (unsigned int) buf[off + 1] << 8;
-		val |= (unsigned int) buf[off + 2] << 16;
-		val |= (unsigned int) buf[off + 3] << 24;
+		unsigned int val = buf[0];
+		val |= (unsigned int) buf[1] << 8;
+		val |= (unsigned int) buf[2] << 16;
+		val |= (unsigned int) buf[3] << 24;
 		pci_write_config_dword(dev, off, val);
 		off += 4;
 		size -= 4;
 	}
 
 	while (size > 0) {
-		pci_write_config_byte(dev, off, buf[off]);
+		pci_write_config_byte(dev, off, buf[0]);
 		off++;
 		--size;
 	}

Index: fs/sysfs/bin.c
===================================================================
RCS file: /home/cvs/linux-2.5/fs/sysfs/bin.c,v
retrieving revision 1.9

sysfs/bin.c +2 -2

diff -u -r1.9 bin.c
@@ -47,7 +47,7 @@
 		return ret;
 	count = ret;
 
-	if (copy_to_user(userbuf, buffer + offs, count) != 0)
+	if (copy_to_user(userbuf, buffer, count) != 0)
 		return -EINVAL;
 
 	pr_debug("offs = %lld, *off = %lld, count = %zd\n", offs, *off, count);

@@ -83,7 +83,7 @@
 			count = size - offs;
 	}
 
-	if (copy_from_user(buffer + offs, userbuf, count))
+	if (copy_from_user(buffer, userbuf, count))
 		return -EFAULT;
 
 	count = flush_write(dentry, buffer, offs, count);

Marcel Holtmann

author

2003-07-26 10:59 UTC #4 lore raw

Hi Manuel,

Quoted 6 lines

Quoted 3 lines
I tracked down the problem to request_firmware() or a sysfs problem. With the firmware included in a header file the driver itself works perfect.
You are right, the problem was in sysfs, attached goes a patch that WorksForMe (tm), please test and report.

the firmware loading of my driver is now working, thanks. If someone has double checked the pci-sysfs.c change, please forward this patch to Linus.

What did Marcelo say about including your backport into 2.4?

Quoted 4 lines

Quoted 2 lines
Attached is a sample of how I use the request_firmware() and from the documentation it seems correct to me.
Not what I was asking for, but it seams OK.

I know, but I don't have such an easy package you requested.

Regards

Marcel

Manuel Estrada Sainz

2003-07-26 11:11 UTC #5 lore raw

Quoted 6 lines · Marcel Holtmann

Hi Manuel,
Quoted 4 lines
Quoted 3 lines
I tracked down the problem to request_firmware() or a sysfs problem. With the firmware included in a header file the driver itself works perfect.

Quoted 6 lines

Quoted 2 lines
You are right, the problem was in sysfs, attached goes a patch that WorksForMe (tm), please test and report.
the firmware loading of my driver is now working, thanks. If someone has double checked the pci-sysfs.c change, please forward this patch to Linus.

I'll wait for Greg and/or Matthew to say something about it.

Quoted 1 line

What did Marcelo say about including your backport into 2.4?

Nothing, he didn't even bother to answer :-(. Maybe someone more relevant than myself could dig on the issue.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Attached is a sample of how I use the request_firmware() and from the documentation it seems correct to me.
Not what I was asking for, but it seams OK.
I know, but I don't have such an easy package you requested.

Never mind, it is fixed now.

Regards

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Matthew Wilcox

2003-07-28 02:01 UTC #6 lore raw

In reply to Manuel Estrada Sainz

Quoted 3 lines · Manuel Estrada Sainz

- hopefully adapt drivers/pci/pci-sysfs.c to this changes
- Please double check, I didn't look very carefully on this.

Definitely wrong. I was going to undo this change since I realised how it doesn't work for you; but the change you made to the PCI code is wrong. It ends up copying everything to offset 0 from the buf address. I wish Pat had cc'd me when making the change to the interface originally ;-(

I'll whip up a patch in a few minutes.

Signature · 3 lines

"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Manuel Estrada Sainz

2003-07-28 00:20 UTC #7 lore raw

Quoted 8 lines · Matthew Wilcox

Quoted 3 lines · Manuel Estrada Sainz
- hopefully adapt drivers/pci/pci-sysfs.c to this changes
- Please double check, I didn't look very carefully on this.
Definitely wrong. I was going to undo this change since I realised how it doesn't work for you; but the change you made to the PCI code is wrong. It ends up copying everything to offset 0 from the buf address.

Exactly, and that is what sysfs code expects (with the rest of the patch), the buffer is just temporary storage, it doesn't really matter what offset you use as long as you don't write further than buffer+PAGE_SIZE and both sides of the issue agree.

Quoted 4 lines

I wish Pat had cc'd me when making the change to the interface originally ;-(
I'll whip up a patch in a few minutes.

Great.

Have a nice day

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Manuel Estrada Sainz

2003-08-01 15:05 UTC #8 lore raw

Quoted 6 lines · Manuel Estrada Sainz

Quoted 5 lines · Matthew Wilcox
Quoted 3 lines · Manuel Estrada Sainz
- hopefully adapt drivers/pci/pci-sysfs.c to this changes
- Please double check, I didn't look very carefully on this.

Quoted 8 lines

Quoted 3 lines
Definitely wrong. I was going to undo this change since I realised how it doesn't work for you; but the change you made to the PCI code is wrong. It ends up copying everything to offset 0 from the buf address.
Exactly, and that is what sysfs code expects (with the rest of the patch), the buffer is just temporary storage, it doesn't really matter what offset you use as long as you don't write further than buffer+PAGE_SIZE and both sides of the issue agree.

My fault, I was severely misguided here, Matthew is of course write. Now that I understand the issue a little deeper I'll try send a correct patch to get the issue done with.

Have a nice day

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

Manuel Estrada Sainz

2003-08-01 18:51 UTC #9 lore raw

Quoted 11 lines · Manuel Estrada Sainz

Quoted 10 lines · Manuel Estrada Sainz
Quoted 8 lines · Matthew Wilcox
Quoted 3 lines · Manuel Estrada Sainz
- hopefully adapt drivers/pci/pci-sysfs.c to this changes
- Please double check, I didn't look very carefully on this.
Definitely wrong. I was going to undo this change since I realised how it doesn't work for you; but the change you made to the PCI code is wrong. It ends up copying everything to offset 0 from the buf address.

Quoted 8 lines

Quoted 4 lines
Exactly, and that is what sysfs code expects (with the rest of the patch), the buffer is just temporary storage, it doesn't really matter what offset you use as long as you don't write further than buffer+PAGE_SIZE and both sides of the issue agree.
My fault, I was severely misguided here, Matthew is of course write. Now that I understand the issue a little deeper I'll try send a correct patch to get the issue done with.

OK, this time I have tested the PCI changes and it works:

the patches:

- sysfs-bin-unbreak-2-main.diff:
- undo recent change, made in the believe that "buffer" was the size of the whole file, it is just PAGE_SIZE in size. This was causing kernel memory corruption.

- Since files are allowed to have unknown sizes, by setting their size to 0, we can't preallocate a buffer of their size on open.

- sysfs-bin-unbreak-2-request_firmware.diff:
- Adapt to the above sysfs change.

- sysfs-bin-unbreak-2-pci.diff:
- hopefully adapt drivers/pci/pci-sysfs.c to this changes.
- Matthew can probably make it look prettier, but for now it works.

Have a nice day

Manuel

Signature · 6 lines

--- Manuel Estrada Sainz <[email protected]>
                         <[email protected]>
			 <[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.

sysfs/bin.c +2 -2

@@ -47,7 +47,7 @@
 		return ret;
 	count = ret;
 
-	if (copy_to_user(userbuf, buffer + offs, count) != 0)
+	if (copy_to_user(userbuf, buffer, count) != 0)
 		return -EINVAL;
 
 	pr_debug("offs = %lld, *off = %lld, count = %zd\n", offs, *off, count);

@@ -83,7 +83,7 @@
 			count = size - offs;
 	}
 
-	if (copy_from_user(buffer + offs, userbuf, count))
+	if (copy_from_user(buffer, userbuf, count))
 		return -EFAULT;
 
 	count = flush_write(dentry, buffer, offs, count);

pci/pci-sysfs.c +14 ~~-12~~

@@ -67,6 +67,7 @@
 {
 	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
 	unsigned int size = 64;
+	loff_t init_off = off;
 
 	/* Several chips lock up trying to read undefined config space */
 	if (capable(CAP_SYS_ADMIN)) {

@@ -87,7 +88,7 @@
 	while (off & 3) {
 		unsigned char val;
 		pci_read_config_byte(dev, off, &val);
-		buf[off] = val;
+		buf[off - init_off] = val;
 		off++;
 		if (--size == 0)
 			break;

@@ -96,10 +97,10 @@
 	while (size > 3) {
 		unsigned int val;
 		pci_read_config_dword(dev, off, &val);
-		buf[off] = val & 0xff;
-		buf[off + 1] = (val >> 8) & 0xff;
-		buf[off + 2] = (val >> 16) & 0xff;
-		buf[off + 3] = (val >> 24) & 0xff;
+		buf[off - init_off] = val & 0xff;
+		buf[off - init_off + 1] = (val >> 8) & 0xff;
+		buf[off - init_off + 2] = (val >> 16) & 0xff;
+		buf[off - init_off + 3] = (val >> 24) & 0xff;
 		off += 4;
 		size -= 4;
 	}

@@ -107,7 +108,7 @@
 	while (size > 0) {
 		unsigned char val;
 		pci_read_config_byte(dev, off, &val);
-		buf[off] = val;
+		buf[off - init_off] = val;
 		off++;
 		--size;
 	}

@@ -120,6 +121,7 @@
 {
 	struct pci_dev *dev = to_pci_dev(container_of(kobj,struct device,kobj));
 	unsigned int size = count;
+	loff_t init_off = off;
 
 	if (off > 256)
 		return 0;

@@ -129,24 +131,24 @@
 	}
 
 	while (off & 3) {
-		pci_write_config_byte(dev, off, buf[off]);
+		pci_write_config_byte(dev, off, buf[off - init_off]);
 		off++;
 		if (--size == 0)
 			break;
 	}
 
 	while (size > 3) {
-		unsigned int val = buf[off];
-		val |= (unsigned int) buf[off + 1] << 8;
-		val |= (unsigned int) buf[off + 2] << 16;
-		val |= (unsigned int) buf[off + 3] << 24;
+		unsigned int val = buf[off - init_off];
+		val |= (unsigned int) buf[off - init_off + 1] << 8;
+		val |= (unsigned int) buf[off - init_off + 2] << 16;
+		val |= (unsigned int) buf[off - init_off + 3] << 24;
 		pci_write_config_dword(dev, off, val);
 		off += 4;
 		size -= 4;
 	}
 
 	while (size > 0) {
-		pci_write_config_byte(dev, off, buf[off]);
+		pci_write_config_byte(dev, off, buf[off - init_off]);
 		off++;
 		--size;
 	}

base/firmware_class.c +2 -2

@@ -151,7 +151,7 @@
 	if (offset + count > fw->size)
 		count = fw->size - offset;
 
-	memcpy(buffer + offset, fw->data + offset, count);
+	memcpy(buffer, fw->data + offset, count);
 	return count;
 }
 static int

@@ -200,7 +200,7 @@
 	if (retval)
 		return retval;
 
-	memcpy(fw->data + offset, buffer + offset, count);
+	memcpy(fw->data + offset, buffer, count);
 
 	fw->size = max_t(size_t, offset + count, fw->size);