request_firmware related problems.

Hi,

Please apply the following patches.

Matthew Wilcox seams busy lately and didn't confirm the pci changes, but I have tested them and it works. He can modify it later with nicer code if he finds it necessary, sysfs binary support has been broken for too much time already being in this stage of development :-/.

- 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.

- request_firmware_own-workqueue.diff:
- In it's current form request_firmware_async() sleeps way too long on the system's shared workqueue, which makes it unresponsive until the firmware load finishes, gets canceled or times out.

Have a nice day

Manuel

--- 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.

@@ -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);

@@ -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;
 	}

@@ -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);
 

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

diff -u -r1.3 drivers/base/firmware_class.c
@@ -22,6 +22,8 @@
 MODULE_LICENSE("GPL");
 
 static int loading_timeout = 10;	/* In seconds */
+static struct workqueue_struct *firmware_wq;
+
 
 struct firmware_priv {
 	char fw_id[FIRMWARE_NAME_MAX];

@@ -467,7 +469,7 @@
 	};
 	INIT_WORK(&fw_work->work, request_firmware_work_func, fw_work);
 
-	schedule_work(&fw_work->work);
+	queue_work(firmware_wq, &fw_work->work);
 	return 0;
 }
 

@@ -485,12 +487,20 @@
 		       __FUNCTION__);
 		class_unregister(&firmware_class);
 	}
+	firmware_wq = create_workqueue("firmware");
+	if (!firmware_wq) {
+		printk(KERN_ERR "%s: create_workqueue failed\n", __FUNCTION__);
+		class_remove_file(&firmware_class, &class_attr_timeout);
+		class_unregister(&firmware_class);
+		error = -EIO;
+	}
 	return error;
 
 }
 static void __exit
 firmware_class_exit(void)
 {
+	destroy_workqueue(firmware_wq);
 	class_remove_file(&firmware_class, &class_attr_timeout);
 	class_unregister(&firmware_class);
 }

Please, send just one patch per email, each one having its own changelog info. There's just no way anyone's patch import tools can handle a single changelog and multiple attachments. It is painful.

Sorry, I'll do it in a few minutes.

I guess so.

OK, I'll wait on this one, and think about it, after all, it just fixes annoying behavior, not real breakness.

Thanks for the quick reply

Manuel

--- 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.

Thanks for mentioning this!

There is indeed an explosion of kernel threads. I feel many of them fall into two categories:

* needs a one-shot kernel thread, often after a timer fires or similar interrupt-ish-context code runs (error handling threads also apply here)

* only needs per-cpu threads if IO(or net) load is significant

And personally, I don't think anyone would scream if schedule_{task,work} just created a new thread -- if needed -- and then did the work. That's what the driver authors _really_ want out of the interface, IMO. They don't want to wait for another keventd task, typically. In other words, a thread pool, that will create new threads beyond the max pool size.

On the other side of the coin, a valid reason for creating kernel threads is when your application is constantly doing stuff in process context. You might as well have dedicated kernel threads for these drivers, because you are assured you will need at least one.

So, in terms of concrete suggestions:

1) if schedule_work is called and no kevent thread is available, create a new one
2) ponder perhaps an implementation that would use generic keventd until a certain load is reached; then, create per-cpu kernel threads just like private workqueue creation occurs now. i.e. switch from shared (keventd) to private at runtime.

As a tangent, I would love a simplified interface that was used in driver code like this:
run_in_process_context(callback_fn, callback_data)
that eliminates the tq/workqueue struct altogether. Combine this simplified interface with suggestion #1 above, and you've got something quite useful, and tough to screw up.

Jeff

3) offer private workqueue interface like we have now -- but one thread only, not NN threads

How about, launching the private workqueue threads on demand?

a) Create thread/s then the first work gets scheduled.
b) When the last pending work is done arm a timer or schedule a delayed special work that kills the thread/s after a while of idleness.
c) When a new work gets added to an otherwise idle workqueue either:
- If we have thread/s we cancel the kill and go on.
- Else we are back at a) we create it/them.

And if instead of creating all threads at once, we create them one by one as work comes in and also kill them one by one as they stay idle, it could be quite smooth.

How does this sound? Am I over-designing the issue?

IMHO this would remove the threads of unused private workqueues without affecting busy workqueues.

About using a single thread instead of NN, wouldn't that impact workqueue users expecting speedy handling of their queues?

Have a nice day

Manuel

--- 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.

4) Have one permanent thread running which kicks off another thread whenever something has been on the queue for more than 5 seconds and reaps threads when the queue is empty for 30 ?