[PATCH] [2.6.0-test3] request_firmware related problems.

[PATCH] [2.6.0-test3] request_firmware related problems.

[Manuel Estrada Sainz]

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

 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 <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
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.

[-- Attachment #2: sysfs-bin-unbreak-2-main.diff --]
[-- Type: text/plain, Size: 565 bytes --]

--- fs/sysfs/bin.c	4 Jul 2003 02:21:18 -0000	1.9
+++ fs/sysfs/bin.c	1 Aug 2003 14:26:45 -0000
@@ -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);

[-- Attachment #3: sysfs-bin-unbreak-2-pci.diff --]
[-- Type: text/plain, Size: 2190 bytes --]

--- drivers/pci/pci-sysfs.c	4 Jul 2003 02:21:18 -0000	1.6
+++ drivers/pci/pci-sysfs.c	1 Aug 2003 14:26:43 -0000
@@ -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;
 	}

[-- Attachment #4: sysfs-bin-unbreak-2-request_firmware.diff --]
[-- Type: text/plain, Size: 543 bytes --]

--- drivers/base/firmware_class.c	26 Jul 2003 08:38:07 -0000
+++ drivers/base/firmware_class.c	1 Aug 2003 14:26:41 -0000
@@ -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);
 

[-- Attachment #5: request_firmware_own-workqueue.diff --]
[-- Type: text/plain, Size: 1295 bytes --]

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
--- drivers/base/firmware_class.c	4 Jul 2003 02:21:18 -0000	1.3
+++ drivers/base/firmware_class.c	26 Jul 2003 08:38:07 -0000
@@ -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);
 }

Re: [PATCH] [2.6.0-test3] request_firmware related problems.

[Andrew Morton]

Manuel Estrada Sainz <ranty@debian.org> wrote:
>
>  Please apply the following patches.

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.

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

Does this mean that we have another gaggle of kernel threads for all the
time the system is up?

It might be better to create a custom kernel thread on-demand, or kill off
the workqueue when its job has completed.

Re: [PATCH] [2.6.0-test3] request_firmware related problems.

[Manuel Estrada Sainz]

On Sun, Aug 10, 2003 at 02:29:28PM -0700, Andrew Morton wrote:
> Manuel Estrada Sainz <ranty@debian.org> wrote:
> >
> >  Please apply the following patches.
> 
> 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.
 
> >   - 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.
> 
> Does this mean that we have another gaggle of kernel threads for all the
> time the system is up?

 I guess so.

> It might be better to create a custom kernel thread on-demand, or kill off
> the workqueue when its job has completed.

 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 <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
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.

[PATCH] [2.6.0-test3] request_firmware related problems.

[Manuel Estrada Sainz]

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

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

 Since this is just a single change plus side effects I just merged it
 back together.

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

	- Adapt request_firmware() to the sysfs change.

  	- Adapt drivers/pci/pci-sysfs.c to the sysfs change.

 Sorry again for the multi-patch email

 	Manuel

-- 
--- Manuel Estrada Sainz <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
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.

[-- Attachment #2: sysfs-bin-unbreak-2.diff --]
[-- Type: text/plain, Size: 3420 bytes --]

diff -u drivers/base/firmware_class.c drivers/base/firmware_class.c
--- drivers/base/firmware_class.c	26 Jul 2003 08:38:07 -0000
+++ drivers/base/firmware_class.c	1 Aug 2003 14:26:41 -0000
@@ -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);
 
only in patch2:
unchanged:
--- drivers/pci/pci-sysfs.c	4 Jul 2003 02:21:18 -0000	1.6
+++ drivers/pci/pci-sysfs.c	1 Aug 2003 14:26:43 -0000
@@ -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;
 	}
only in patch2:
unchanged:
--- fs/sysfs/bin.c	4 Jul 2003 02:21:18 -0000	1.9
+++ fs/sysfs/bin.c	1 Aug 2003 14:26:45 -0000
@@ -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);

Re: [PATCH] [2.6.0-test3] request_firmware related problems.

[Jeff Garzik]

Andrew Morton wrote:
> Does this mean that we have another gaggle of kernel threads for all the
> time the system is up?
> 
> It might be better to create a custom kernel thread on-demand, or kill off
> the workqueue when its job has completed.


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

Re: [PATCH] [2.6.0-test3] request_firmware related problems.

[Jeff Garzik]

Jeff Garzik wrote:
> 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.


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

Handling workqueue threads (was: Re: [PATCH] [2.6.0-test3] request_firmware related problems.)

[Manuel Estrada Sainz]

On Sun, Aug 10, 2003 at 07:00:29PM -0400, Jeff Garzik wrote:
> Jeff Garzik wrote:
> >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.
> 
> 
> 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 <ranty@debian.org>
                         <ranty@bigfoot.com>
			 <ranty@users.sourceforge.net>
------------------------ <manuel.estrada@hispalinux.es> -------------------
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.

Re: [PATCH] [2.6.0-test3] request_firmware related problems.

[Alan Cox]

On Llu, 2003-08-11 at 00:00, Jeff Garzik wrote:
> > 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.
> 
> 
> 3) offer private workqueue interface like we have now -- but one thread 
> only, not NN threads

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 ?