[PATCH] sc1200_wdt: Fix oops

[PATCH] sc1200_wdt: Fix oops

[Alan]

If loaded with isapnp = 0 the driver explodes. This is catching
people out now and then. What should happen in the working case is
a complete mystery and the code appears terminally confused, but we
can at least make the error path work properly.

Signed-off-by: Alan Cox <alan@linux.intel.com>
Partially-Resolves-bug: https://bugzilla.kernel.org/show_bug.cgi?id=53991
---
 drivers/watchdog/sc1200wdt.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/sc1200wdt.c b/drivers/watchdog/sc1200wdt.c
index 3b9fff9..131193a 100644
--- a/drivers/watchdog/sc1200wdt.c
+++ b/drivers/watchdog/sc1200wdt.c
@@ -409,8 +409,9 @@ static int __init sc1200wdt_init(void)
 #if defined CONFIG_PNP
 	/* now that the user has specified an IO port and we haven't detected
 	 * any devices, disable pnp support */
+	if (isapnp)
+		pnp_unregister_driver(&scl200wdt_pnp_driver);
 	isapnp = 0;
-	pnp_unregister_driver(&scl200wdt_pnp_driver);
 #endif
 
 	if (!request_region(io, io_len, SC1200_MODULE_NAME)) {

Re: [PATCH] sc1200_wdt: Fix oops

[Guenter Roeck]

On Wed, Dec 04, 2013 at 03:31:52PM +0000, Alan wrote:
> If loaded with isapnp = 0 the driver explodes. This is catching
> people out now and then. What should happen in the working case is
> a complete mystery and the code appears terminally confused, but we

Looks like it uses the pnp infrastructure only to auto-detect the
"io" address, and assumes that pnp_register_driver() calls the
pnp probe function immediately. Afterwards it unregisters and
works (or is supposed to work) outside the pnp subsystem. Brr.

> can at least make the error path work properly.
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>
> Partially-Resolves-bug: https://bugzilla.kernel.org/show_bug.cgi?id=53991

Actually, I think it fully resolves it, because the other part (sc520 driver
not working on SC1100 CPU) isn't a bug. It isn't working on an ARM or PPC CPU
either, nor on any Intel CPU.

Anyway,

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/sc1200wdt.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/sc1200wdt.c b/drivers/watchdog/sc1200wdt.c
> index 3b9fff9..131193a 100644
> --- a/drivers/watchdog/sc1200wdt.c
> +++ b/drivers/watchdog/sc1200wdt.c
> @@ -409,8 +409,9 @@ static int __init sc1200wdt_init(void)
>  #if defined CONFIG_PNP
>  	/* now that the user has specified an IO port and we haven't detected
>  	 * any devices, disable pnp support */
> +	if (isapnp)
> +		pnp_unregister_driver(&scl200wdt_pnp_driver);
>  	isapnp = 0;
> -	pnp_unregister_driver(&scl200wdt_pnp_driver);
>  #endif
>  
>  	if (!request_region(io, io_len, SC1200_MODULE_NAME)) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] sc1200_wdt: Fix oops

[One Thousand Gnomes]

On Wed, 4 Dec 2013 08:31:18 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On Wed, Dec 04, 2013 at 03:31:52PM +0000, Alan wrote:
> > If loaded with isapnp = 0 the driver explodes. This is catching
> > people out now and then. What should happen in the working case is
> > a complete mystery and the code appears terminally confused, but we
> 
> Looks like it uses the pnp infrastructure only to auto-detect the
> "io" address, and assumes that pnp_register_driver() calls the
> pnp probe function immediately. Afterwards it unregisters and
> works (or is supposed to work) outside the pnp subsystem. Brr.
> 
> > can at least make the error path work properly.
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> > Partially-Resolves-bug: https://bugzilla.kernel.org/show_bug.cgi?id=53991
> 
> Actually, I think it fully resolves it, because the other part (sc520 driver
> not working on SC1100 CPU) isn't a bug. It isn't working on an ARM or PPC CPU
> either, nor on any Intel CPU.

As far as I can tell the fact it fails on the sc1100 is also a bug - but
the mess of idefs around PNP is impossible to figure out without some
actual docs, time and real hardware.

Alan

Re: [PATCH] sc1200_wdt: Fix oops

[Guenter Roeck]

On 12/04/2013 04:15 PM, One Thousand Gnomes wrote:
> On Wed, 4 Dec 2013 08:31:18 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>> On Wed, Dec 04, 2013 at 03:31:52PM +0000, Alan wrote:
>>> If loaded with isapnp = 0 the driver explodes. This is catching
>>> people out now and then. What should happen in the working case is
>>> a complete mystery and the code appears terminally confused, but we
>>
>> Looks like it uses the pnp infrastructure only to auto-detect the
>> "io" address, and assumes that pnp_register_driver() calls the
>> pnp probe function immediately. Afterwards it unregisters and
>> works (or is supposed to work) outside the pnp subsystem. Brr.
>>
>>> can at least make the error path work properly.
>>>
>>> Signed-off-by: Alan Cox <alan@linux.intel.com>
>>> Partially-Resolves-bug: https://bugzilla.kernel.org/show_bug.cgi?id=53991
>>
>> Actually, I think it fully resolves it, because the other part (sc520 driver
>> not working on SC1100 CPU) isn't a bug. It isn't working on an ARM or PPC CPU
>> either, nor on any Intel CPU.
>
> As far as I can tell the fact it fails on the sc1100 is also a bug - but
> the mess of idefs around PNP is impossible to figure out without some
> actual docs, time and real hardware.
>

... unless isapnp was set to 0 via module parameter, or if SC1100 is not compatible
with SC1200. But I agree, hard to say w/o datasheet and hw.

Either case, seems to me the driver is screaming for a substantial overhaul.

Guenter