[PATCH mm] cfag12864b: fix crash when built-in and no parport present

[PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Miguel Ojeda]

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_ */

Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Daniel Walker]

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

Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Andrew Morton]

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.

Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Miguel Ojeda]

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

Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Daniel Walker]

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

Re: [PATCH mm] cfag12864b: fix crash when built-in and no parport present

[Miguel Ojeda]

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