Patchwork [1/1] UBUNTU: SAUCE: (no-up) Modularize vesafb -- fix initialisation

login
register
mail settings
Submitter Andy Whitcroft
Date July 29, 2010, 8:42 a.m.
Message ID <1280392941-27701-2-git-send-email-apw@canonical.com>
Download mbox | patch
Permalink /patch/60197/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Andy Whitcroft - July 29, 2010, 8:42 a.m.
When this patch was rolled forward, likely between Dapper and Hardy
a chunk of initialisation was lost.  Pull this back in so we actually
have an vesafb_info structure initialised.  Else we may well panic when
we rmmod vesafb.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/video/vesafb.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)
Stefan Bader - July 29, 2010, 9:10 a.m.
On 07/29/2010 10:42 AM, Andy Whitcroft wrote:
> When this patch was rolled forward, likely between Dapper and Hardy
> a chunk of initialisation was lost.  Pull this back in so we actually
> have an vesafb_info structure initialised.  Else we may well panic when
> we rmmod vesafb.
> 
> Signed-off-by: Andy Whitcroft <apw@canonical.com>
> ---
>  drivers/video/vesafb.c |   14 +++++++-------
>  1 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/video/vesafb.c b/drivers/video/vesafb.c
> index 5a95b55..7137226 100644
> --- a/drivers/video/vesafb.c
> +++ b/drivers/video/vesafb.c
> @@ -257,6 +257,7 @@ static int __init vesafb_setup(char *options)
>  static int __init vesafb_probe(struct platform_device *dev)
>  {
>  	struct fb_info *info;
> +	struct vesafb_info *vfb_info;
>  	int i, err;
>  	unsigned int size_vmode;
>  	unsigned int size_remap;
> @@ -315,13 +316,14 @@ static int __init vesafb_probe(struct platform_device *dev)
>  		   spaces our resource handlers simply don't know about */
>  	}
>  
> -	info = framebuffer_alloc(sizeof(u32) * 256, &dev->dev);
> +	info = framebuffer_alloc(sizeof(struct vesafb_info), &dev->dev);
>  	if (!info) {
>  		release_mem_region(vesafb_fix.smem_start, size_total);
>  		return -ENOMEM;
>  	}
> -	info->pseudo_palette = info->par;
> -	info->par = NULL;
> +	vfb_info = (struct vesafb_info *) info->par;
> +	vfb_info->mtrr_hdl = -1;
> +	info->pseudo_palette = vfb_info->pseudo_palette;

Hm, before info->pseudo_palette was info->par and info->par was reset to NULL.
Now info->pseudo_palette is set to ((struct vesafb_info *)
info->par)->pseudo_palette (which would be ok if that is the first element) and
info->par is left unchanged. Just double checking if thats the intention.

>  
>  	/* set vesafb aperture size for generic probing */
>  	info->apertures = alloc_apertures(1);
> @@ -464,18 +466,16 @@ static int __init vesafb_probe(struct platform_device *dev)
>  		}
>  
>  		if (type) {
> -			int rc;
> -
>  			/* Find the largest power-of-two */
>  			while (temp_size & (temp_size - 1))
>  				temp_size &= (temp_size - 1);
>  
>  			/* Try and find a power of two to add */
>  			do {
> -				rc = mtrr_add(vesafb_fix.smem_start, temp_size,
> +				vfb_info->mtrr_hdl = mtrr_add(vesafb_fix.smem_start, temp_size,
>  					      type, 1);
>  				temp_size >>= 1;
> -			} while (temp_size >= PAGE_SIZE && rc == -EINVAL);
> +			} while (temp_size >= PAGE_SIZE && vfb_info->mtrr_hdl == -EINVAL);
>  		}
>  	}
>  #endif
Andy Whitcroft - July 29, 2010, 9:28 a.m.
On Thu, Jul 29, 2010 at 11:10:00AM +0200, Stefan Bader wrote:
> On 07/29/2010 10:42 AM, Andy Whitcroft wrote:
> > When this patch was rolled forward, likely between Dapper and Hardy
> > a chunk of initialisation was lost.  Pull this back in so we actually
> > have an vesafb_info structure initialised.  Else we may well panic when
> > we rmmod vesafb.
> > 
> > Signed-off-by: Andy Whitcroft <apw@canonical.com>
> > ---
> >  drivers/video/vesafb.c |   14 +++++++-------
> >  1 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/video/vesafb.c b/drivers/video/vesafb.c
> > index 5a95b55..7137226 100644
> > --- a/drivers/video/vesafb.c
> > +++ b/drivers/video/vesafb.c
> > @@ -257,6 +257,7 @@ static int __init vesafb_setup(char *options)
> >  static int __init vesafb_probe(struct platform_device *dev)
> >  {
> >  	struct fb_info *info;
> > +	struct vesafb_info *vfb_info;
> >  	int i, err;
> >  	unsigned int size_vmode;
> >  	unsigned int size_remap;
> > @@ -315,13 +316,14 @@ static int __init vesafb_probe(struct platform_device *dev)
> >  		   spaces our resource handlers simply don't know about */
> >  	}
> >  
> > -	info = framebuffer_alloc(sizeof(u32) * 256, &dev->dev);
> > +	info = framebuffer_alloc(sizeof(struct vesafb_info), &dev->dev);
> >  	if (!info) {
> >  		release_mem_region(vesafb_fix.smem_start, size_total);
> >  		return -ENOMEM;
> >  	}
> > -	info->pseudo_palette = info->par;
> > -	info->par = NULL;
> > +	vfb_info = (struct vesafb_info *) info->par;
> > +	vfb_info->mtrr_hdl = -1;
> > +	info->pseudo_palette = vfb_info->pseudo_palette;
> 
> Hm, before info->pseudo_palette was info->par and info->par was reset to NULL.
> Now info->pseudo_palette is set to ((struct vesafb_info *)
> info->par)->pseudo_palette (which would be ok if that is the first element) and
> info->par is left unchanged. Just double checking if thats the intention.

Right, before the original patch the info structure was only the palette
and was an allocation of 256 bytes.  They therefore they moved the pointer
over to the palette pointer and dropped the par pointer (the code as it
appears before this patch).  The original patch needs to store the mtrr
number so it switches to an info structure which contains the palette array
and the mtrr.  The palette is an array embedded in the info structure,
and palette should point at it, where in the structure is not important.
As we do info->pseudo_palette = info->par->pseudo_palette which gives us
the real address of the array whereever it is in the info structure we
do not have to worry about the order.

> >  
> >  	/* set vesafb aperture size for generic probing */
> >  	info->apertures = alloc_apertures(1);
> > @@ -464,18 +466,16 @@ static int __init vesafb_probe(struct platform_device *dev)
> >  		}
> >  
> >  		if (type) {
> > -			int rc;
> > -
> >  			/* Find the largest power-of-two */
> >  			while (temp_size & (temp_size - 1))
> >  				temp_size &= (temp_size - 1);
> >  
> >  			/* Try and find a power of two to add */
> >  			do {
> > -				rc = mtrr_add(vesafb_fix.smem_start, temp_size,
> > +				vfb_info->mtrr_hdl = mtrr_add(vesafb_fix.smem_start, temp_size,
> >  					      type, 1);
> >  				temp_size >>= 1;
> > -			} while (temp_size >= PAGE_SIZE && rc == -EINVAL);
> > +			} while (temp_size >= PAGE_SIZE && vfb_info->mtrr_hdl == -EINVAL);
> >  		}
> >  	}
> >  #endif

-apw
Stefan Bader - July 29, 2010, 9:38 a.m.
Ok, so

On 07/29/2010 11:28 AM, Andy Whitcroft wrote:
> On Thu, Jul 29, 2010 at 11:10:00AM +0200, Stefan Bader wrote:
>> On 07/29/2010 10:42 AM, Andy Whitcroft wrote:
>>> When this patch was rolled forward, likely between Dapper and Hardy
>>> a chunk of initialisation was lost.  Pull this back in so we actually
>>> have an vesafb_info structure initialised.  Else we may well panic when
>>> we rmmod vesafb.
>>>
>>> Signed-off-by: Andy Whitcroft <apw@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
>>> ---
>>>  drivers/video/vesafb.c |   14 +++++++-------
>>>  1 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/video/vesafb.c b/drivers/video/vesafb.c
>>> index 5a95b55..7137226 100644
>>> --- a/drivers/video/vesafb.c
>>> +++ b/drivers/video/vesafb.c
>>> @@ -257,6 +257,7 @@ static int __init vesafb_setup(char *options)
>>>  static int __init vesafb_probe(struct platform_device *dev)
>>>  {
>>>  	struct fb_info *info;
>>> +	struct vesafb_info *vfb_info;
>>>  	int i, err;
>>>  	unsigned int size_vmode;
>>>  	unsigned int size_remap;
>>> @@ -315,13 +316,14 @@ static int __init vesafb_probe(struct platform_device *dev)
>>>  		   spaces our resource handlers simply don't know about */
>>>  	}
>>>  
>>> -	info = framebuffer_alloc(sizeof(u32) * 256, &dev->dev);
>>> +	info = framebuffer_alloc(sizeof(struct vesafb_info), &dev->dev);
>>>  	if (!info) {
>>>  		release_mem_region(vesafb_fix.smem_start, size_total);
>>>  		return -ENOMEM;
>>>  	}
>>> -	info->pseudo_palette = info->par;
>>> -	info->par = NULL;
>>> +	vfb_info = (struct vesafb_info *) info->par;
>>> +	vfb_info->mtrr_hdl = -1;
>>> +	info->pseudo_palette = vfb_info->pseudo_palette;
>>
>> Hm, before info->pseudo_palette was info->par and info->par was reset to NULL.
>> Now info->pseudo_palette is set to ((struct vesafb_info *)
>> info->par)->pseudo_palette (which would be ok if that is the first element) and
>> info->par is left unchanged. Just double checking if thats the intention.
> 
> Right, before the original patch the info structure was only the palette
> and was an allocation of 256 bytes.  They therefore they moved the pointer
> over to the palette pointer and dropped the par pointer (the code as it
> appears before this patch).  The original patch needs to store the mtrr
> number so it switches to an info structure which contains the palette array
> and the mtrr.  The palette is an array embedded in the info structure,
> and palette should point at it, where in the structure is not important.
> As we do info->pseudo_palette = info->par->pseudo_palette which gives us
> the real address of the array whereever it is in the info structure we
> do not have to worry about the order.
> 
>>>  
>>>  	/* set vesafb aperture size for generic probing */
>>>  	info->apertures = alloc_apertures(1);
>>> @@ -464,18 +466,16 @@ static int __init vesafb_probe(struct platform_device *dev)
>>>  		}
>>>  
>>>  		if (type) {
>>> -			int rc;
>>> -
>>>  			/* Find the largest power-of-two */
>>>  			while (temp_size & (temp_size - 1))
>>>  				temp_size &= (temp_size - 1);
>>>  
>>>  			/* Try and find a power of two to add */
>>>  			do {
>>> -				rc = mtrr_add(vesafb_fix.smem_start, temp_size,
>>> +				vfb_info->mtrr_hdl = mtrr_add(vesafb_fix.smem_start, temp_size,
>>>  					      type, 1);
>>>  				temp_size >>= 1;
>>> -			} while (temp_size >= PAGE_SIZE && rc == -EINVAL);
>>> +			} while (temp_size >= PAGE_SIZE && vfb_info->mtrr_hdl == -EINVAL);
>>>  		}
>>>  	}
>>>  #endif
> 
> -apw
Brad Figg - July 29, 2010, 2:47 p.m.
On 07/29/2010 02:28 AM, Andy Whitcroft wrote:
> On Thu, Jul 29, 2010 at 11:10:00AM +0200, Stefan Bader wrote:
>> On 07/29/2010 10:42 AM, Andy Whitcroft wrote:
>>> When this patch was rolled forward, likely between Dapper and Hardy
>>> a chunk of initialisation was lost.  Pull this back in so we actually
>>> have an vesafb_info structure initialised.  Else we may well panic when
>>> we rmmod vesafb.
>>>
>>> Signed-off-by: Andy Whitcroft<apw@canonical.com>
Acked-by: Brad Figg <brad.figg@canonical.com>
>>> ---
>>>   drivers/video/vesafb.c |   14 +++++++-------
>>>   1 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/video/vesafb.c b/drivers/video/vesafb.c
>>> index 5a95b55..7137226 100644
>>> --- a/drivers/video/vesafb.c
>>> +++ b/drivers/video/vesafb.c
>>> @@ -257,6 +257,7 @@ static int __init vesafb_setup(char *options)
>>>   static int __init vesafb_probe(struct platform_device *dev)
>>>   {
>>>   	struct fb_info *info;
>>> +	struct vesafb_info *vfb_info;
>>>   	int i, err;
>>>   	unsigned int size_vmode;
>>>   	unsigned int size_remap;
>>> @@ -315,13 +316,14 @@ static int __init vesafb_probe(struct platform_device *dev)
>>>   		   spaces our resource handlers simply don't know about */
>>>   	}
>>>
>>> -	info = framebuffer_alloc(sizeof(u32) * 256,&dev->dev);
>>> +	info = framebuffer_alloc(sizeof(struct vesafb_info),&dev->dev);
>>>   	if (!info) {
>>>   		release_mem_region(vesafb_fix.smem_start, size_total);
>>>   		return -ENOMEM;
>>>   	}
>>> -	info->pseudo_palette = info->par;
>>> -	info->par = NULL;
>>> +	vfb_info = (struct vesafb_info *) info->par;
>>> +	vfb_info->mtrr_hdl = -1;
>>> +	info->pseudo_palette = vfb_info->pseudo_palette;
>>
>> Hm, before info->pseudo_palette was info->par and info->par was reset to NULL.
>> Now info->pseudo_palette is set to ((struct vesafb_info *)
>> info->par)->pseudo_palette (which would be ok if that is the first element) and
>> info->par is left unchanged. Just double checking if thats the intention.
>
> Right, before the original patch the info structure was only the palette
> and was an allocation of 256 bytes.  They therefore they moved the pointer
> over to the palette pointer and dropped the par pointer (the code as it
> appears before this patch).  The original patch needs to store the mtrr
> number so it switches to an info structure which contains the palette array
> and the mtrr.  The palette is an array embedded in the info structure,
> and palette should point at it, where in the structure is not important.
> As we do info->pseudo_palette = info->par->pseudo_palette which gives us
> the real address of the array whereever it is in the info structure we
> do not have to worry about the order.
>
>>>
>>>   	/* set vesafb aperture size for generic probing */
>>>   	info->apertures = alloc_apertures(1);
>>> @@ -464,18 +466,16 @@ static int __init vesafb_probe(struct platform_device *dev)
>>>   		}
>>>
>>>   		if (type) {
>>> -			int rc;
>>> -
>>>   			/* Find the largest power-of-two */
>>>   			while (temp_size&  (temp_size - 1))
>>>   				temp_size&= (temp_size - 1);
>>>
>>>   			/* Try and find a power of two to add */
>>>   			do {
>>> -				rc = mtrr_add(vesafb_fix.smem_start, temp_size,
>>> +				vfb_info->mtrr_hdl = mtrr_add(vesafb_fix.smem_start, temp_size,
>>>   					      type, 1);
>>>   				temp_size>>= 1;
>>> -			} while (temp_size>= PAGE_SIZE&&  rc == -EINVAL);
>>> +			} while (temp_size>= PAGE_SIZE&&  vfb_info->mtrr_hdl == -EINVAL);
>>>   		}
>>>   	}
>>>   #endif
>
> -apw
>

Patch

diff --git a/drivers/video/vesafb.c b/drivers/video/vesafb.c
index 5a95b55..7137226 100644
--- a/drivers/video/vesafb.c
+++ b/drivers/video/vesafb.c
@@ -257,6 +257,7 @@  static int __init vesafb_setup(char *options)
 static int __init vesafb_probe(struct platform_device *dev)
 {
 	struct fb_info *info;
+	struct vesafb_info *vfb_info;
 	int i, err;
 	unsigned int size_vmode;
 	unsigned int size_remap;
@@ -315,13 +316,14 @@  static int __init vesafb_probe(struct platform_device *dev)
 		   spaces our resource handlers simply don't know about */
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 256, &dev->dev);
+	info = framebuffer_alloc(sizeof(struct vesafb_info), &dev->dev);
 	if (!info) {
 		release_mem_region(vesafb_fix.smem_start, size_total);
 		return -ENOMEM;
 	}
-	info->pseudo_palette = info->par;
-	info->par = NULL;
+	vfb_info = (struct vesafb_info *) info->par;
+	vfb_info->mtrr_hdl = -1;
+	info->pseudo_palette = vfb_info->pseudo_palette;
 
 	/* set vesafb aperture size for generic probing */
 	info->apertures = alloc_apertures(1);
@@ -464,18 +466,16 @@  static int __init vesafb_probe(struct platform_device *dev)
 		}
 
 		if (type) {
-			int rc;
-
 			/* Find the largest power-of-two */
 			while (temp_size & (temp_size - 1))
 				temp_size &= (temp_size - 1);
 
 			/* Try and find a power of two to add */
 			do {
-				rc = mtrr_add(vesafb_fix.smem_start, temp_size,
+				vfb_info->mtrr_hdl = mtrr_add(vesafb_fix.smem_start, temp_size,
 					      type, 1);
 				temp_size >>= 1;
-			} while (temp_size >= PAGE_SIZE && rc == -EINVAL);
+			} while (temp_size >= PAGE_SIZE && vfb_info->mtrr_hdl == -EINVAL);
 		}
 	}
 #endif