Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Herbert Xu]

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

tags 244207 pending
quit

On Sat, Apr 17, 2004 at 12:43:11PM +0200, Rolf Kutz wrote:
> Package: kernel-source-2.6.5
> Version: 2.6.5-1
> Severity: normal
> 
> The mwave module gives the following warning on suspend:
> 
> Apr 16 09:55:13 localhost kernel: Device 'mwave' does not have a release() funct
> ion, it is broken and must be fixed.
> Apr 16 09:55:13 localhost kernel: Badness in device_release at drivers/base/core
> .c:85

Thanks for the report.

This patch should shut the warning up.

Cheers,
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 767 bytes --]

Index: drivers/char/mwave/mwavedd.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.5/drivers/char/mwave/mwavedd.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 mwavedd.c
--- a/drivers/char/mwave/mwavedd.c	28 Sep 2003 04:44:12 -0000	1.1.1.7
+++ b/drivers/char/mwave/mwavedd.c	17 Apr 2004 11:37:52 -0000
@@ -470,7 +470,13 @@
  * sysfs support <paulsch@us.ibm.com>
  */
 
-struct device mwave_device;
+static void mwave_device_release(struct device *dev)
+{
+}
+
+static struct device mwave_device = {
+	.release = mwave_device_release,
+};
 
 /* Prevent code redundancy, create a macro for mwave_show_* functions. */
 #define mwave_show_function(attr_name, format_string, field)		\

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Russell King]

On Sat, Apr 17, 2004 at 09:39:18PM +1000, Herbert Xu wrote:
> On Sat, Apr 17, 2004 at 12:43:11PM +0200, Rolf Kutz wrote:
> > Package: kernel-source-2.6.5
> > Version: 2.6.5-1
> > Severity: normal
> > 
> > The mwave module gives the following warning on suspend:
> > 
> > Apr 16 09:55:13 localhost kernel: Device 'mwave' does not have a release() funct
> > ion, it is broken and must be fixed.
> > Apr 16 09:55:13 localhost kernel: Badness in device_release at drivers/base/core
> > .c:85
> 
> Thanks for the report.
> 
> This patch should shut the warning up.

And that's all it does.  It doesn't stop the oops which potentically can
happen when the struct device is freed (by the module being unloaded)
while there is still a reference to the struct device.

This is the whole point of the warning - to warn that the necessary
release functionality is not present, and that the way the device
is being used is buggy.

Providing an empty release function is not a solution - it merely papers
over the warning leaving the real bug behind.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Herbert Xu]

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

On Sat, Apr 17, 2004 at 12:48:50PM +0100, Russell King wrote:
> 
> And that's all it does.  It doesn't stop the oops which potentically can
> happen when the struct device is freed (by the module being unloaded)
> while there is still a reference to the struct device.

You're quite right.  Even worse, there was an memset in the init
function which set the release function back to NULL :)

This patch should be better.

Cheers,
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1317 bytes --]

Index: drivers/char/mwave/mwavedd.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.5/drivers/char/mwave/mwavedd.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 mwavedd.c
--- a/drivers/char/mwave/mwavedd.c	28 Sep 2003 04:44:12 -0000	1.1.1.7
+++ b/drivers/char/mwave/mwavedd.c	17 Apr 2004 12:20:07 -0000
@@ -470,7 +470,18 @@
  * sysfs support <paulsch@us.ibm.com>
  */
 
-struct device mwave_device;
+static void mwave_device_release(struct device *dev)
+{
+	pMWAVE_DEVICE_DATA pDrvData = &mwave_s_mdd;
+
+	if (pDrvData->device_registered)
+		module_put(THIS_MODULE);
+}
+
+static struct device mwave_device = {
+	.bus_id = "mwave",
+	.release = mwave_device_release,
+};
 
 /* Prevent code redundancy, create a macro for mwave_show_* functions. */
 #define mwave_show_function(attr_name, format_string, field)		\
@@ -639,11 +650,9 @@
 	/* uart is registered */
 
 	/* sysfs */
-	memset(&mwave_device, 0, sizeof (struct device));
-	snprintf(mwave_device.bus_id, BUS_ID_SIZE, "mwave");
-
 	if (device_register(&mwave_device))
 		goto cleanup_error;
+	__module_get(THIS_MODULE);
 	pDrvData->device_registered = TRUE;
 	for (i = 0; i < ARRAY_SIZE(mwave_dev_attrs); i++) {
 		if(device_create_file(&mwave_device, mwave_dev_attrs[i])) {

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Herbert Xu]

On Sat, Apr 17, 2004 at 10:23:22PM +1000, herbert wrote:
> 
> This patch should be better.

Please scrap that one, it just makes the module unloadable.
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Herbert Xu]

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

On Sat, Apr 17, 2004 at 10:29:54PM +1000, herbert wrote:
> 
> Please scrap that one, it just makes the module unloadable.

This patch resolves the problem of the module getting unloaded before
the device is released by waiting in the module_exit function.

Cheers,
-- 
Debian GNU/Linux 3.0 is out! ( http://www.debian.org/ )
Email:  Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1610 bytes --]

Index: drivers/char/mwave/mwavedd.c
===================================================================
RCS file: /home/gondolin/herbert/src/CVS/debian/kernel-source-2.5/drivers/char/mwave/mwavedd.c,v
retrieving revision 1.1.1.7
diff -u -r1.1.1.7 mwavedd.c
--- a/drivers/char/mwave/mwavedd.c	28 Sep 2003 04:44:12 -0000	1.1.1.7
+++ b/drivers/char/mwave/mwavedd.c	18 Apr 2004 10:26:41 -0000
@@ -57,6 +57,7 @@
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/delay.h>
+#include <linux/completion.h>
 #include "smapi.h"
 #include "mwavedd.h"
 #include "3780i.h"
@@ -470,7 +471,17 @@
  * sysfs support <paulsch@us.ibm.com>
  */
 
-struct device mwave_device;
+static DECLARE_COMPLETION(mwave_device_released);
+
+static void mwave_device_release(struct device *dev)
+{
+	complete(&mwave_device_released);
+}
+
+static struct device mwave_device = {
+	.bus_id = "mwave",
+	.release = mwave_device_release,
+};
 
 /* Prevent code redundancy, create a macro for mwave_show_* functions. */
 #define mwave_show_function(attr_name, format_string, field)		\
@@ -518,7 +529,9 @@
 	pDrvData->nr_registered_attrs = 0;
 
 	if (pDrvData->device_registered) {
+		INIT_COMPLETION(mwave_device_released);
 		device_unregister(&mwave_device);
+		wait_for_completion(&mwave_device_released);
 		pDrvData->device_registered = FALSE;
 	}
 
@@ -639,9 +652,6 @@
 	/* uart is registered */
 
 	/* sysfs */
-	memset(&mwave_device, 0, sizeof (struct device));
-	snprintf(mwave_device.bus_id, BUS_ID_SIZE, "mwave");
-
 	if (device_register(&mwave_device))
 		goto cleanup_error;
 	pDrvData->device_registered = TRUE;

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Christoph Hellwig]

On Sun, Apr 18, 2004 at 08:31:09PM +1000, Herbert Xu wrote:
> On Sat, Apr 17, 2004 at 10:29:54PM +1000, herbert wrote:
> > 
> > Please scrap that one, it just makes the module unloadable.
> 
> This patch resolves the problem of the module getting unloaded before
> the device is released by waiting in the module_exit function.

It's still bogus.  The driver gets the sysfs support completely wrong
and the right thing would be to find that change in the BK history and
back it out.

Re: Bug#244207: kernel-source-2.6.5: mwave gives warning on suspend

[Christoph Hellwig]

On Sun, Apr 18, 2004 at 12:17:55PM +0100, Christoph Hellwig wrote:
> It's still bogus.  The driver gets the sysfs support completely wrong
> and the right thing would be to find that change in the BK history and
> back it out.

Here's a patch to #if 0 out all the crap:

--- 1.12/drivers/char/mwave/mwavedd.c	Wed Sep 10 08:41:45 2003
+++ edited/drivers/char/mwave/mwavedd.c	Sun Apr 18 13:31:22 2004
@@ -466,6 +466,7 @@
 
 static struct miscdevice mwave_misc_dev = { MWAVE_MINOR, "mwave", &mwave_fops };
 
+#if 0 /* totally b0rked */
 /*
  * sysfs support <paulsch@us.ibm.com>
  */
@@ -499,6 +500,7 @@
 	&dev_attr_uart_irq,
 	&dev_attr_uart_io,
 };
+#endif
 
 /*
 * mwave_init is called on module load
@@ -508,11 +510,11 @@
 */
 static void mwave_exit(void)
 {
-	int i;
 	pMWAVE_DEVICE_DATA pDrvData = &mwave_s_mdd;
 
 	PRINTK_1(TRACE_MWAVE, "mwavedd::mwave_exit entry\n");
 
+#if 0
 	for (i = 0; i < pDrvData->nr_registered_attrs; i++)
 		device_remove_file(&mwave_device, mwave_dev_attrs[i]);
 	pDrvData->nr_registered_attrs = 0;
@@ -521,6 +523,7 @@
 		device_unregister(&mwave_device);
 		pDrvData->device_registered = FALSE;
 	}
+#endif
 
 	if ( pDrvData->sLine >= 0 ) {
 		unregister_serial(pDrvData->sLine);
@@ -638,6 +641,7 @@
 	}
 	/* uart is registered */
 
+#if 0
 	/* sysfs */
 	memset(&mwave_device, 0, sizeof (struct device));
 	snprintf(mwave_device.bus_id, BUS_ID_SIZE, "mwave");
@@ -655,6 +659,7 @@
 		}
 		pDrvData->nr_registered_attrs++;
 	}
+#endif
 
 	/* SUCCESS! */
 	return 0;