* [PATCH mm] cfag12864b: fix crash when built-in and no parport present @ 2007-02-10 19:51 Miguel Ojeda 2007-02-12 19:52 ` Daniel Walker 2007-02-13 5:35 ` Andrew Morton 0 siblings, 2 replies; 6+ messages in thread From: Miguel Ojeda @ 2007-02-10 19:51 UTC (permalink / raw) To: akpm; +Cc: dwalker, linux-kernel Andrew, Daniel who reported the crash has tested this patch and says that it works fine avoiding it, so you can merge. drivers-add-lcd-support-fix-crash-when-built-in-and-no-parport-present.patch Signed-off-by: Miguel Ojeda Sandonis <maxextreme@gmail.com> --- diff --git a/drivers/auxdisplay/cfag12864b.c b/drivers/auxdisplay/cfag12864b.c index 889583d..cb44cb4 100644 --- a/drivers/auxdisplay/cfag12864b.c +++ b/drivers/auxdisplay/cfag12864b.c @@ -312,6 +312,17 @@ EXPORT_SYMBOL_GPL(cfag12864b_disable); EXPORT_SYMBOL_GPL(cfag12864b_isenabled); /* + * Is the module inited? + */ + +static unsigned char cfag12864b_inited; +unsigned char cfag12864b_isinited(void) +{ + return cfag12864b_inited; +} +EXPORT_SYMBOL_GPL(cfag12864b_isinited); + +/* * Module Init & Exit */ @@ -319,6 +330,13 @@ static int __init cfag12864b_init(void) { int ret = -EINVAL; + /* ks0108_init() must be called first */ + if (!ks0108_isinited()) { + printk(KERN_ERR CFAG12864B_NAME ": ERROR: " + "ks0108 is not initialized\n"); + goto none; + } + if (PAGE_SIZE < CFAG12864B_SIZE) { printk(KERN_ERR CFAG12864B_NAME ": ERROR: " "page size (%i) < cfag12864b size (%i)\n", @@ -354,6 +372,7 @@ static int __init cfag12864b_init(void) cfag12864b_clear(); cfag12864b_on(); + cfag12864b_inited = 1; return 0; cachealloced: diff --git a/drivers/auxdisplay/cfag12864bfb.c b/drivers/auxdisplay/cfag12864bfb.c index 94765e7..66fafbb 100644 --- a/drivers/auxdisplay/cfag12864bfb.c +++ b/drivers/auxdisplay/cfag12864bfb.c @@ -137,7 +137,14 @@ static struct platform_device *cfag12864 static int __init cfag12864bfb_init(void) { - int ret; + int ret = -EINVAL; + + /* cfag12864b_init() must be called first */ + if (!cfag12864b_isinited()) { + printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " + "cfag12864b is not initialized\n"); + goto none; + } if (cfag12864b_enable()) { printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " @@ -162,6 +169,7 @@ static int __init cfag12864bfb_init(void } } +none: return ret; } diff --git a/drivers/auxdisplay/ks0108.c b/drivers/auxdisplay/ks0108.c index a637575..e6c3646 100644 --- a/drivers/auxdisplay/ks0108.c +++ b/drivers/auxdisplay/ks0108.c @@ -111,6 +111,17 @@ EXPORT_SYMBOL_GPL(ks0108_address); EXPORT_SYMBOL_GPL(ks0108_page); /* + * Is the module inited? + */ + +static unsigned char ks0108_inited; +unsigned char ks0108_isinited(void) +{ + return ks0108_inited; +} +EXPORT_SYMBOL_GPL(ks0108_isinited); + +/* * Module Init & Exit */ @@ -142,6 +153,7 @@ static int __init ks0108_init(void) goto registered; } + ks0108_inited = 1; return 0; registered: diff --git a/include/linux/cfag12864b.h b/include/linux/cfag12864b.h index 0bc45e6..1605dd8 100644 --- a/include/linux/cfag12864b.h +++ b/include/linux/cfag12864b.h @@ -73,5 +73,10 @@ extern void cfag12864b_disable(void); */ extern unsigned char cfag12864b_isenabled(void); +/* + * Is the module inited? + */ +extern unsigned char cfag12864b_isinited(void); + #endif /* _CFAG12864B_H_ */ diff --git a/include/linux/ks0108.h b/include/linux/ks0108.h index 8047d4b..a2c54ac 100644 --- a/include/linux/ks0108.h +++ b/include/linux/ks0108.h @@ -43,4 +43,7 @@ extern void ks0108_address(unsigned char /* Set the controller's current page (0..7) */ extern void ks0108_page(unsigned char page); +/* Is the module inited? */ +extern unsigned char ks0108_isinited(void); + #endif /* _KS0108_H_ */ ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present 2007-02-10 19:51 [PATCH mm] cfag12864b: fix crash when built-in and no parport present Miguel Ojeda @ 2007-02-12 19:52 ` Daniel Walker 2007-02-13 5:35 ` Andrew Morton 1 sibling, 0 replies; 6+ messages in thread From: Daniel Walker @ 2007-02-12 19:52 UTC (permalink / raw) To: Miguel Ojeda; +Cc: akpm, linux-kernel On Sat, 2007-02-10 at 20:51 +0100, Miguel Ojeda wrote: > Andrew, Daniel who reported the crash has tested this patch and says that it works fine avoiding it, so you can merge. > Yes, I tested it with the ks0108 enabled alone, and with both ks0108 and CFAG12864B .. Both booted fully.. Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present 2007-02-10 19:51 [PATCH mm] cfag12864b: fix crash when built-in and no parport present Miguel Ojeda 2007-02-12 19:52 ` Daniel Walker @ 2007-02-13 5:35 ` Andrew Morton 2007-02-13 6:29 ` Miguel Ojeda 2007-02-13 9:16 ` Daniel Walker 1 sibling, 2 replies; 6+ messages in thread From: Andrew Morton @ 2007-02-13 5:35 UTC (permalink / raw) To: Miguel Ojeda; +Cc: dwalker, linux-kernel On Sat, 10 Feb 2007 20:51:23 +0100 Miguel Ojeda <maxextreme@gmail.com> wrote: > Andrew, Daniel who reported the crash has tested this patch and says that it works fine avoiding it, so you can merge. > > drivers-add-lcd-support-fix-crash-when-built-in-and-no-parport-present.patch > Signed-off-by: Miguel Ojeda Sandonis <maxextreme@gmail.com> > --- > diff --git a/drivers/auxdisplay/cfag12864b.c b/drivers/auxdisplay/cfag12864b.c > index 889583d..cb44cb4 100644 > --- a/drivers/auxdisplay/cfag12864b.c > +++ b/drivers/auxdisplay/cfag12864b.c > @@ -312,6 +312,17 @@ EXPORT_SYMBOL_GPL(cfag12864b_disable); > EXPORT_SYMBOL_GPL(cfag12864b_isenabled); > > /* > + * Is the module inited? > + */ > + > +static unsigned char cfag12864b_inited; > +unsigned char cfag12864b_isinited(void) > +{ > + return cfag12864b_inited; > +} > +EXPORT_SYMBOL_GPL(cfag12864b_isinited); > + > +/* > * Module Init & Exit > */ > > @@ -319,6 +330,13 @@ static int __init cfag12864b_init(void) > { > int ret = -EINVAL; > > + /* ks0108_init() must be called first */ > + if (!ks0108_isinited()) { > + printk(KERN_ERR CFAG12864B_NAME ": ERROR: " > + "ks0108 is not initialized\n"); > + goto none; > + } > + > if (PAGE_SIZE < CFAG12864B_SIZE) { > printk(KERN_ERR CFAG12864B_NAME ": ERROR: " > "page size (%i) < cfag12864b size (%i)\n", > @@ -354,6 +372,7 @@ static int __init cfag12864b_init(void) > cfag12864b_clear(); > cfag12864b_on(); > > + cfag12864b_inited = 1; > return 0; > > cachealloced: > diff --git a/drivers/auxdisplay/cfag12864bfb.c b/drivers/auxdisplay/cfag12864bfb.c > index 94765e7..66fafbb 100644 > --- a/drivers/auxdisplay/cfag12864bfb.c > +++ b/drivers/auxdisplay/cfag12864bfb.c > @@ -137,7 +137,14 @@ static struct platform_device *cfag12864 > > static int __init cfag12864bfb_init(void) > { > - int ret; > + int ret = -EINVAL; > + > + /* cfag12864b_init() must be called first */ > + if (!cfag12864b_isinited()) { > + printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " > + "cfag12864b is not initialized\n"); > + goto none; > + } > > if (cfag12864b_enable()) { > printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " > @@ -162,6 +169,7 @@ static int __init cfag12864bfb_init(void > } > } > > +none: > return ret; > } > > diff --git a/drivers/auxdisplay/ks0108.c b/drivers/auxdisplay/ks0108.c > index a637575..e6c3646 100644 > --- a/drivers/auxdisplay/ks0108.c > +++ b/drivers/auxdisplay/ks0108.c > @@ -111,6 +111,17 @@ EXPORT_SYMBOL_GPL(ks0108_address); > EXPORT_SYMBOL_GPL(ks0108_page); > > /* > + * Is the module inited? > + */ > + > +static unsigned char ks0108_inited; > +unsigned char ks0108_isinited(void) > +{ > + return ks0108_inited; > +} > +EXPORT_SYMBOL_GPL(ks0108_isinited); > + > +/* > * Module Init & Exit > */ > > @@ -142,6 +153,7 @@ static int __init ks0108_init(void) > goto registered; > } > > + ks0108_inited = 1; > return 0; > > registered: > diff --git a/include/linux/cfag12864b.h b/include/linux/cfag12864b.h > index 0bc45e6..1605dd8 100644 > --- a/include/linux/cfag12864b.h > +++ b/include/linux/cfag12864b.h > @@ -73,5 +73,10 @@ extern void cfag12864b_disable(void); > */ > extern unsigned char cfag12864b_isenabled(void); > > +/* > + * Is the module inited? > + */ > +extern unsigned char cfag12864b_isinited(void); > + > #endif /* _CFAG12864B_H_ */ > > diff --git a/include/linux/ks0108.h b/include/linux/ks0108.h > index 8047d4b..a2c54ac 100644 > --- a/include/linux/ks0108.h > +++ b/include/linux/ks0108.h > @@ -43,4 +43,7 @@ extern void ks0108_address(unsigned char > /* Set the controller's current page (0..7) */ > extern void ks0108_page(unsigned char page); > > +/* Is the module inited? */ > +extern unsigned char ks0108_isinited(void); > + > #endif /* _KS0108_H_ */ This looks hacky. Your patch didn't include a description of the bug (please always include such a description) so I'm not sure what's going on here. But I'm suspecting that module dependencies and appropriate choice of initcall levels is the right way to fix whatever bug this is. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present 2007-02-13 5:35 ` Andrew Morton @ 2007-02-13 6:29 ` Miguel Ojeda 2007-02-13 9:16 ` Daniel Walker 1 sibling, 0 replies; 6+ messages in thread From: Miguel Ojeda @ 2007-02-13 6:29 UTC (permalink / raw) To: Andrew Morton; +Cc: dwalker, linux-kernel On 2/13/07, Andrew Morton <akpm@linux-foundation.org> wrote: > On Sat, 10 Feb 2007 20:51:23 +0100 Miguel Ojeda <maxextreme@gmail.com> > wrote: > > > Andrew, Daniel who reported the crash has tested this patch and says that > it works fine avoiding it, so you can merge. > > > > > drivers-add-lcd-support-fix-crash-when-built-in-and-no-parport-present.patch > > Signed-off-by: Miguel Ojeda Sandonis <maxextreme@gmail.com> > > --- > > diff --git a/drivers/auxdisplay/cfag12864b.c > b/drivers/auxdisplay/cfag12864b.c > > index 889583d..cb44cb4 100644 > > --- a/drivers/auxdisplay/cfag12864b.c > > +++ b/drivers/auxdisplay/cfag12864b.c > > @@ -312,6 +312,17 @@ EXPORT_SYMBOL_GPL(cfag12864b_disable); > > EXPORT_SYMBOL_GPL(cfag12864b_isenabled); > > > > /* > > + * Is the module inited? > > + */ > > + > > +static unsigned char cfag12864b_inited; > > +unsigned char cfag12864b_isinited(void) > > +{ > > + return cfag12864b_inited; > > +} > > +EXPORT_SYMBOL_GPL(cfag12864b_isinited); > > + > > +/* > > * Module Init & Exit > > */ > > > > @@ -319,6 +330,13 @@ static int __init cfag12864b_init(void) > > { > > int ret = -EINVAL; > > > > + /* ks0108_init() must be called first */ > > + if (!ks0108_isinited()) { > > + printk(KERN_ERR CFAG12864B_NAME ": ERROR: " > > + "ks0108 is not initialized\n"); > > + goto none; > > + } > > + > > if (PAGE_SIZE < CFAG12864B_SIZE) { > > printk(KERN_ERR CFAG12864B_NAME ": ERROR: " > > "page size (%i) < cfag12864b size (%i)\n", > > @@ -354,6 +372,7 @@ static int __init cfag12864b_init(void) > > cfag12864b_clear(); > > cfag12864b_on(); > > > > + cfag12864b_inited = 1; > > return 0; > > > > cachealloced: > > diff --git a/drivers/auxdisplay/cfag12864bfb.c > b/drivers/auxdisplay/cfag12864bfb.c > > index 94765e7..66fafbb 100644 > > --- a/drivers/auxdisplay/cfag12864bfb.c > > +++ b/drivers/auxdisplay/cfag12864bfb.c > > @@ -137,7 +137,14 @@ static struct platform_device *cfag12864 > > > > static int __init cfag12864bfb_init(void) > > { > > - int ret; > > + int ret = -EINVAL; > > + > > + /* cfag12864b_init() must be called first */ > > + if (!cfag12864b_isinited()) { > > + printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " > > + "cfag12864b is not initialized\n"); > > + goto none; > > + } > > > > if (cfag12864b_enable()) { > > printk(KERN_ERR CFAG12864BFB_NAME ": ERROR: " > > @@ -162,6 +169,7 @@ static int __init cfag12864bfb_init(void > > } > > } > > > > +none: > > return ret; > > } > > > > diff --git a/drivers/auxdisplay/ks0108.c b/drivers/auxdisplay/ks0108.c > > index a637575..e6c3646 100644 > > --- a/drivers/auxdisplay/ks0108.c > > +++ b/drivers/auxdisplay/ks0108.c > > @@ -111,6 +111,17 @@ EXPORT_SYMBOL_GPL(ks0108_address); > > EXPORT_SYMBOL_GPL(ks0108_page); > > > > /* > > + * Is the module inited? > > + */ > > + > > +static unsigned char ks0108_inited; > > +unsigned char ks0108_isinited(void) > > +{ > > + return ks0108_inited; > > +} > > +EXPORT_SYMBOL_GPL(ks0108_isinited); > > + > > +/* > > * Module Init & Exit > > */ > > > > @@ -142,6 +153,7 @@ static int __init ks0108_init(void) > > goto registered; > > } > > > > + ks0108_inited = 1; > > return 0; > > > > registered: > > diff --git a/include/linux/cfag12864b.h b/include/linux/cfag12864b.h > > index 0bc45e6..1605dd8 100644 > > --- a/include/linux/cfag12864b.h > > +++ b/include/linux/cfag12864b.h > > @@ -73,5 +73,10 @@ extern void cfag12864b_disable(void); > > */ > > extern unsigned char cfag12864b_isenabled(void); > > > > +/* > > + * Is the module inited? > > + */ > > +extern unsigned char cfag12864b_isinited(void); > > + > > #endif /* _CFAG12864B_H_ */ > > > > diff --git a/include/linux/ks0108.h b/include/linux/ks0108.h > > index 8047d4b..a2c54ac 100644 > > --- a/include/linux/ks0108.h > > +++ b/include/linux/ks0108.h > > @@ -43,4 +43,7 @@ extern void ks0108_address(unsigned char > > /* Set the controller's current page (0..7) */ > > extern void ks0108_page(unsigned char page); > > > > +/* Is the module inited? */ > > +extern unsigned char ks0108_isinited(void); > > + > > #endif /* _KS0108_H_ */ > > This looks hacky. > > Your patch didn't include a description of the bug (please always include > such a description) so I'm not sure what's going on here. But I'm > Indeed, sorry. The patch is about "crash on CONFIG_CFAG12864B=y in 2.6.20-rc3-mm1" ( http://lkml.org/lkml/2007/1/7/118 ). > suspecting that module dependencies and appropriate choice of initcall > levels is the right way to fix whatever bug this is. > The problem comes when ks0108/cfag12864b are built-in and any parallel port is present. ks0108_init() is called first, as it should be, but fails to load (as there is no parallel port to use). After that, cfag12864b_init() gets called, without knowing anything about ks0108 failed, and calls ks0108_writecontrol(), which dereferences an uninitialized pointer. Init order is OK, I think. The problem is how to stop cfag12864b_init() being called if ks0108 failed to load. modprobe does it for us, but, how when built-in? As anybody suggested another solution, I send that patch. Regards, Miguel -- Miguel Ojeda http://maxextreme.googlepages.com/index.htm ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present 2007-02-13 5:35 ` Andrew Morton 2007-02-13 6:29 ` Miguel Ojeda @ 2007-02-13 9:16 ` Daniel Walker 2007-02-13 13:29 ` Miguel Ojeda 1 sibling, 1 reply; 6+ messages in thread From: Daniel Walker @ 2007-02-13 9:16 UTC (permalink / raw) To: Andrew Morton; +Cc: Miguel Ojeda, linux-kernel On Mon, 2007-02-12 at 21:35 -0800, Andrew Morton wrote: > > This looks hacky. > One other thing that could be added is a change in the initcalls . ks0108 should be subsys_initcall() and the LCD devices device_initcall(). That would make sure one runs before the other. I don't think that alone would fix the crash tho. I didn't suggest it before cause I think the order of the sources in the build system might do this implicitly even if both are in device_initcall .. Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present 2007-02-13 9:16 ` Daniel Walker @ 2007-02-13 13:29 ` Miguel Ojeda 0 siblings, 0 replies; 6+ messages in thread From: Miguel Ojeda @ 2007-02-13 13:29 UTC (permalink / raw) To: Daniel Walker; +Cc: Andrew Morton, linux-kernel On 2/13/07, Daniel Walker <dwalker@mvista.com> wrote: > On Mon, 2007-02-12 at 21:35 -0800, Andrew Morton wrote: > > > > > This looks hacky. > > > > One other thing that could be added is a change in the initcalls . > ks0108 should be subsys_initcall() and the LCD devices > device_initcall(). That would make sure one runs before the other. I > Yes, it could be better, although it is not necessary for now (link order is telling who should get called first). > > don't think that alone would fix the crash tho. I didn't suggest it > before cause I think the order of the sources in the build system might > do this implicitly even if both are in device_initcall .. > Yes, link order matters; however, the problem is related to cfag12864b_init() being called if ks0108 failed. Indeed, if cfag12864b is called before ks0108 is a mistake too, but it isn't happening here. I also think the solution looks hacky (I said that I was waiting for a better solution), and also I think it should be checked by the kernel at boot like modprobe does; but I don't know any other solution. Any ideas? -- Miguel Ojeda http://maxextreme.googlepages.com/index.htm ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-02-13 13:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-10 19:51 [PATCH mm] cfag12864b: fix crash when built-in and no parport present Miguel Ojeda 2007-02-12 19:52 ` Daniel Walker 2007-02-13 5:35 ` Andrew Morton 2007-02-13 6:29 ` Miguel Ojeda 2007-02-13 9:16 ` Daniel Walker 2007-02-13 13:29 ` Miguel Ojeda
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.