[patch] ns558 bug

[patch] ns558 bug

[matthieu castet]

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

Hi,

this patch is based on http://bugzilla.kernel.org/show_bug.cgi?id=2962 
patch from adam belay.

It solve a oops when pnp_register_driver(&ns558_pnp_driver) failed.

Please apply this patch.

Matthieu

[-- Attachment #2: ns558.patch --]
[-- Type: text/x-patch, Size: 1015 bytes --]

Index: drivers/input/gameport/ns558.c
===================================================================
RCS file: /home/mat/dev/linux-cvs-rep/linux-cvs/drivers/input/gameport/ns558.c,v
retrieving revision 1.15
diff -u -u -r1.15 ns558.c
--- drivers/input/gameport/ns558.c	16 Sep 2004 14:04:04 -0000	1.15
+++ drivers/input/gameport/ns558.c	4 Feb 2005 19:53:20 -0000
@@ -261,6 +261,8 @@
 
 #endif
 
+static int registered = 0;
+
 int __init ns558_init(void)
 {
 	int i = 0;
@@ -272,8 +274,10 @@
 	while (ns558_isa_portlist[i])
 		ns558_isa_probe(ns558_isa_portlist[i++]);
 
-	pnp_register_driver(&ns558_pnp_driver);
-	return list_empty(&ns558_list) ? -ENODEV : 0;
+	if (pnp_register_driver(&ns558_pnp_driver) >= 0) 
+		registered = 1;
+
+	return (list_empty(&ns558_list) && !registered) ? -ENODEV : 0;
 }
 
 void __exit ns558_exit(void)
@@ -297,7 +301,8 @@
 				break;
 		}
 	}
-	pnp_unregister_driver(&ns558_pnp_driver);
+	if (registered)
+		pnp_unregister_driver(&ns558_pnp_driver);
 }
 
 module_init(ns558_init);

Re: [patch] ns558 bug

[Adam Belay]

On Fri, Feb 04, 2005 at 09:00:54PM +0100, matthieu castet wrote:
> Hi,
> 
> this patch is based on http://bugzilla.kernel.org/show_bug.cgi?id=2962 
> patch from adam belay.
> 
> It solve a oops when pnp_register_driver(&ns558_pnp_driver) failed.
> 
> Please apply this patch.
> 
> Matthieu

I remember writing a version of this patch a while ago.  The current behavior
is broken because it shouldn't be considered a failure if the driver doesn't
find any devices.

Thanks,
Adam


> Index: drivers/input/gameport/ns558.c
> ===================================================================
> RCS file: /home/mat/dev/linux-cvs-rep/linux-cvs/drivers/input/gameport/ns558.c,v
> retrieving revision 1.15
> diff -u -u -r1.15 ns558.c
> --- drivers/input/gameport/ns558.c	16 Sep 2004 14:04:04 -0000	1.15
> +++ drivers/input/gameport/ns558.c	4 Feb 2005 19:53:20 -0000
> @@ -261,6 +261,8 @@
>  
>  #endif
>  
> +static int registered = 0;
> +
>  int __init ns558_init(void)
>  {
>  	int i = 0;
> @@ -272,8 +274,10 @@
>  	while (ns558_isa_portlist[i])
>  		ns558_isa_probe(ns558_isa_portlist[i++]);
>  
> -	pnp_register_driver(&ns558_pnp_driver);
> -	return list_empty(&ns558_list) ? -ENODEV : 0;
> +	if (pnp_register_driver(&ns558_pnp_driver) >= 0) 
> +		registered = 1;
> +
> +	return (list_empty(&ns558_list) && !registered) ? -ENODEV : 0;
>  }
>  
>  void __exit ns558_exit(void)
> @@ -297,7 +301,8 @@
>  				break;
>  		}
>  	}
> -	pnp_unregister_driver(&ns558_pnp_driver);
> +	if (registered)
> +		pnp_unregister_driver(&ns558_pnp_driver);
>  }
>  
>  module_init(ns558_init);

Re: [patch] ns558 bug

[Andrew Morton]

ambx1@neo.rr.com (Adam Belay) wrote:
>
> On Fri, Feb 04, 2005 at 09:00:54PM +0100, matthieu castet wrote:
>  > Hi,
>  > 
>  > this patch is based on http://bugzilla.kernel.org/show_bug.cgi?id=2962 
>  > patch from adam belay.
>  > 
>  > It solve a oops when pnp_register_driver(&ns558_pnp_driver) failed.
>  > 
>  > Please apply this patch.
>  > 
>  > Matthieu
> 
>  I remember writing a version of this patch a while ago.  The current behavior
>  is broken because it shouldn't be considered a failure if the driver doesn't
>  find any devices.

So would this be the appropriate fix?

--- 25/drivers/input/gameport/ns558.c~ns558-oops-fix	2005-02-04 19:03:11.065813120 -0800
+++ 25-akpm/drivers/input/gameport/ns558.c	2005-02-04 19:05:52.607255088 -0800
@@ -264,6 +264,7 @@ static struct pnp_driver ns558_pnp_drive
 static int __init ns558_init(void)
 {
 	int i = 0;
+	int ret;
 
 /*
  * Probe for ISA ports.
@@ -272,8 +273,8 @@ static int __init ns558_init(void)
 	while (ns558_isa_portlist[i])
 		ns558_isa_probe(ns558_isa_portlist[i++]);
 
-	pnp_register_driver(&ns558_pnp_driver);
-	return list_empty(&ns558_list) ? -ENODEV : 0;
+	ret = pnp_register_driver(&ns558_pnp_driver);
+	return (ret < 0) ? ret : 0;
 }
 
 static void __exit ns558_exit(void)
_

Re: [patch] ns558 bug

[Adam Belay]

On Fri, Feb 04, 2005 at 07:06:14PM -0800, Andrew Morton wrote:

It looks ok.  My only concern is what would happen if the isa probe succeded
but the pnp_register_driver failed?  "pnp_register_driver" return -ENODEV if
"CONFIG_PNP" isn't enabled.  Do you think this would conflict with legacy
probing?

Thanks,
Adam


> So would this be the appropriate fix?
> 
> --- 25/drivers/input/gameport/ns558.c~ns558-oops-fix	2005-02-04 19:03:11.065813120 -0800
> +++ 25-akpm/drivers/input/gameport/ns558.c	2005-02-04 19:05:52.607255088 -0800
> @@ -264,6 +264,7 @@ static struct pnp_driver ns558_pnp_drive
>  static int __init ns558_init(void)
>  {
>  	int i = 0;
> +	int ret;
>  
>  /*
>   * Probe for ISA ports.
> @@ -272,8 +273,8 @@ static int __init ns558_init(void)
>  	while (ns558_isa_portlist[i])
>  		ns558_isa_probe(ns558_isa_portlist[i++]);
>  
> -	pnp_register_driver(&ns558_pnp_driver);
> -	return list_empty(&ns558_list) ? -ENODEV : 0;
> +	ret = pnp_register_driver(&ns558_pnp_driver);
> +	return (ret < 0) ? ret : 0;
>  }
>  
>  static void __exit ns558_exit(void)
> _

Re: [patch] ns558 bug

[Andrew Morton]

ambx1@neo.rr.com (Adam Belay) wrote:
>
>  It looks ok.  My only concern is what would happen if the isa probe succeded
>  but the pnp_register_driver failed?  "pnp_register_driver" return -ENODEV if
>  "CONFIG_PNP" isn't enabled.  Do you think this would conflict with legacy
>  probing?

Fair enough.  How about this?

static void ns588_unregister_ports(void)
{
	struct ns558 *port;

	list_for_each_entry(port, &ns558_list, node) {
		gameport_unregister_port(&port->gameport);
		switch (port->type) {

#ifdef CONFIG_PNP
			case NS558_PNP:
				/* fall through */
#endif
			case NS558_ISA:
				release_region(port->gameport.io &
					~(port->size - 1), port->size);
				kfree(port);
				break;

			default:
				break;
		}
	}
}

static int __init ns558_init(void)
{
	int i = 0;
	int ret;

/*
 * Probe for ISA ports.
 */

	while (ns558_isa_portlist[i])
		ns558_isa_probe(ns558_isa_portlist[i++]);

	ret = pnp_register_driver(&ns558_pnp_driver);
	if (ret < 0) {
		ns588_unregister_ports();
		return ret;
	}
	return 0;
}

static void __exit ns558_exit(void)
{
	ns588_unregister_ports();
	pnp_unregister_driver(&ns558_pnp_driver);
}

Re: [patch] ns558 bug

[Adam Belay]

On Fri, Feb 04, 2005 at 07:21:15PM -0800, Andrew Morton wrote:
> ambx1@neo.rr.com (Adam Belay) wrote:
> >
> >  It looks ok.  My only concern is what would happen if the isa probe succeded
> >  but the pnp_register_driver failed?  "pnp_register_driver" return -ENODEV if
> >  "CONFIG_PNP" isn't enabled.  Do you think this would conflict with legacy
> >  probing?
> 
> Fair enough.  How about this?

The code no longer is leaking devices.  I may be overlooking something, but I
think it still doesn't retain legacy probed devices when PnP isn't available.

If "pnp_register_driver" fails then "ns588_unregister_ports" will unregister
the correctly detected ISA ports.  "pnp_register_driver" will always fail if
pnp support isn't compiled into the kernel.  We need a way of not 
unregistering "ns558_pnp_driver" in "ns558_exit" but retaining the ports
detected by the legacy probe.  In a way, this is sort of a driver model
problem.

As a more general solution for all drivers, I've been thinking about doing
something like this in the long term.

int ret;
if (!(ret = register_driver(&ns558_driver)))
	return ret;
add_driver_protocol(&ns558_driver, &ns558_pnp);
add_driver_protocol(&ns558_driver, &ns558_isa);

and then on exit:

unregister_driver(&ns558_driver); /* this tears down any successfully
				     registered bus protocol automatically */


For now a less invasive solution might be better.

Comments?

Thanks,
Adam


> 
> static void ns588_unregister_ports(void)
> {
> 	struct ns558 *port;
> 
> 	list_for_each_entry(port, &ns558_list, node) {
> 		gameport_unregister_port(&port->gameport);
> 		switch (port->type) {
> 
> #ifdef CONFIG_PNP
> 			case NS558_PNP:
> 				/* fall through */
> #endif
> 			case NS558_ISA:
> 				release_region(port->gameport.io &
> 					~(port->size - 1), port->size);
> 				kfree(port);
> 				break;
> 
> 			default:
> 				break;
> 		}
> 	}
> }
> 
> static int __init ns558_init(void)
> {
> 	int i = 0;
> 	int ret;
> 
> /*
>  * Probe for ISA ports.
>  */
> 
> 	while (ns558_isa_portlist[i])
> 		ns558_isa_probe(ns558_isa_portlist[i++]);
> 
> 	ret = pnp_register_driver(&ns558_pnp_driver);
> 	if (ret < 0) {
> 		ns588_unregister_ports();
> 		return ret;
> 	}
> 	return 0;
> }
> 
> static void __exit ns558_exit(void)
> {
> 	ns588_unregister_ports();
> 	pnp_unregister_driver(&ns558_pnp_driver);
> }

Re: [patch] ns558 bug

[Andrew Morton]

ambx1@neo.rr.com (Adam Belay) wrote:
>
>  As a more general solution for all drivers, I've been thinking about doing
>  something like this in the long term.
> 
>  int ret;
>  if (!(ret = register_driver(&ns558_driver)))
>  	return ret;
>  add_driver_protocol(&ns558_driver, &ns558_pnp);
>  add_driver_protocol(&ns558_driver, &ns558_isa);
> 
>  and then on exit:
> 
>  unregister_driver(&ns558_driver); /* this tears down any successfully
>  				     registered bus protocol automatically */
> 
> 
>  For now a less invasive solution might be better.

Well the thing oopses the kernel at present.  Your call, Adam.  Something
to make it work acceptably for now would suit.

Re: [patch] ns558 bug

[matthieu castet]

Hi,

Andrew Morton wrote:
> ambx1@neo.rr.com (Adam Belay) wrote:
> 
>>On Fri, Feb 04, 2005 at 09:00:54PM +0100, matthieu castet wrote:
>> > Hi,
>> > 
>> > this patch is based on http://bugzilla.kernel.org/show_bug.cgi?id=2962 
>> > patch from adam belay.
>> > 
>> > It solve a oops when pnp_register_driver(&ns558_pnp_driver) failed.
>> > 
>> > Please apply this patch.
>> > 
>> > Matthieu
>>
>> I remember writing a version of this patch a while ago.  The current behavior
>> is broken because it shouldn't be considered a failure if the driver doesn't
>> find any devices.
Why ?
If isa detection failed, the modules could not find any new isa devices.
If the pnp detection find no device, the module is load.
If it is the kernel modules behaviour, lot's of modules are broken...

If you want that, why not simply alway "return 0" in the __init like it 
is done in the bugzilla patch ?


Matthieu

Re: [patch] ns558 bug

[Vojtech Pavlik]

On Fri, Feb 04, 2005 at 09:00:54PM +0100, matthieu castet wrote:
> Hi,
> 
> this patch is based on http://bugzilla.kernel.org/show_bug.cgi?id=2962 
> patch from adam belay.
> 
> It solve a oops when pnp_register_driver(&ns558_pnp_driver) failed.
> 
> Please apply this patch.

Thanks; applied.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR