pxa_camera: Oops in pxa_camera_probe.

Message ID 20090703161140.845950e8.ospite@studenti.unina.it (mailing list archive)
State Superseded, archived
Headers

Commit Message

Antonio Ospite July 3, 2009, 2:11 p.m. UTC
  On Wed, 1 Jul 2009 20:43:25 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:

> Hi,
> 
> I get this with pxa-camera in mainline linux (from today).
> I haven't touched my board code which used to work in 2.6.30
>

I think I've tracked down the cause. The board code is triggering a
bug in pxa_camera. The same should apply to mioa701 as well.

> Linux video capture interface: v2.00
> Unable to handle kernel NULL pointer dereference at virtual address 00000060
> pgd = c0004000
> [00000060] *pgd=00000000
> Internal error: Oops: f5 [#1] PREEMPT
> Modules linked in:
> CPU: 0    Tainted: G        W   (2.6.31-rc1-ezxdev #1)
> PC is at dev_driver_string+0x0/0x38
> LR is at pxa_camera_probe+0x144/0x428

The offending dev_driver_str() here is the one in the dev_warn() call in
mclk_get_divisor().

This is what is happening: in struct pxacamera_platform_data I have:
	.mclk_10khz = 5000,

which makes the > test in mclk_get_divisor() succeed calling dev_warn
to report that the clock has been limited, but pcdev->soc_host.dev is
still uninitialized at this time.

I could lower the value in my platform data and avoid the bug, but it
would be good to have this fixed ASAP anyway.

The attached rough patch fixes the problem, but you will surely come
out with a better one :)

Thanks,
   Antonio
  

Comments

Guennadi Liakhovetski July 3, 2009, 8:03 p.m. UTC | #1
On Fri, 3 Jul 2009, Antonio Ospite wrote:

> On Wed, 1 Jul 2009 20:43:25 +0200
> Antonio Ospite <ospite@studenti.unina.it> wrote:
> 
> > Hi,
> > 
> > I get this with pxa-camera in mainline linux (from today).
> > I haven't touched my board code which used to work in 2.6.30
> >
> 
> I think I've tracked down the cause. The board code is triggering a
> bug in pxa_camera. The same should apply to mioa701 as well.
> 
> > Linux video capture interface: v2.00
> > Unable to handle kernel NULL pointer dereference at virtual address 00000060
> > pgd = c0004000
> > [00000060] *pgd=00000000
> > Internal error: Oops: f5 [#1] PREEMPT
> > Modules linked in:
> > CPU: 0    Tainted: G        W   (2.6.31-rc1-ezxdev #1)
> > PC is at dev_driver_string+0x0/0x38
> > LR is at pxa_camera_probe+0x144/0x428
> 
> The offending dev_driver_str() here is the one in the dev_warn() call in
> mclk_get_divisor().
> 
> This is what is happening: in struct pxacamera_platform_data I have:
> 	.mclk_10khz = 5000,
> 
> which makes the > test in mclk_get_divisor() succeed calling dev_warn
> to report that the clock has been limited, but pcdev->soc_host.dev is
> still uninitialized at this time.
> 
> I could lower the value in my platform data and avoid the bug, but it
> would be good to have this fixed ASAP anyway.
> 
> The attached rough patch fixes the problem, but you will surely come
> out with a better one :)

Why should I? Your patch seems correct to me so far, thanks. I'll push it 
for 2.6.31. Please, next time inline your patch as described in 
Documentation/SubmittingPatches.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antonio Ospite July 3, 2009, 9:41 p.m. UTC | #2
On Fri, 3 Jul 2009 22:03:27 +0200 (CEST)
Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:

> On Fri, 3 Jul 2009, Antonio Ospite wrote:
> 
> > > Linux video capture interface: v2.00
> > > Unable to handle kernel NULL pointer dereference at virtual address 00000060
> > > pgd = c0004000
> > > [00000060] *pgd=00000000
> > > Internal error: Oops: f5 [#1] PREEMPT
> > > Modules linked in:
> > > CPU: 0    Tainted: G        W   (2.6.31-rc1-ezxdev #1)
> > > PC is at dev_driver_string+0x0/0x38
> > > LR is at pxa_camera_probe+0x144/0x428
> > 
> > The offending dev_driver_str() here is the one in the dev_warn() call in
> > mclk_get_divisor().
> > 
> > This is what is happening: in struct pxacamera_platform_data I have:
> > 	.mclk_10khz = 5000,
> > 
> > which makes the > test in mclk_get_divisor() succeed calling dev_warn
> > to report that the clock has been limited, but pcdev->soc_host.dev is
> > still uninitialized at this time.
> > 
> > I could lower the value in my platform data and avoid the bug, but it
> > would be good to have this fixed ASAP anyway.
> > 
> > The attached rough patch fixes the problem, but you will surely come
> > out with a better one :)
> 
> Why should I? Your patch seems correct to me so far, thanks. I'll push it 
> for 2.6.31. Please, next time inline your patch as described in 
> Documentation/SubmittingPatches.
>

Well, it should be correct, I just thought it could be considered
unpretty with the pcdev->soc_host initializations scattered here and
there, that's what I was referring to.
But, if this is ok to you, it's ok to me too :)

Ciao,
   Antonio
  
Robert Jarzmik July 4, 2009, 7:35 p.m. UTC | #3
Antonio Ospite <ospite@studenti.unina.it> writes:

> On Fri, 3 Jul 2009 22:03:27 +0200 (CEST)
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>
>> On Fri, 3 Jul 2009, Antonio Ospite wrote:
>> 
>> > > Linux video capture interface: v2.00
>> > > Unable to handle kernel NULL pointer dereference at virtual address 00000060
>> > > pgd = c0004000
>> > > [00000060] *pgd=00000000
>> > > Internal error: Oops: f5 [#1] PREEMPT
>> > > Modules linked in:
>> > > CPU: 0    Tainted: G        W   (2.6.31-rc1-ezxdev #1)
>> > > PC is at dev_driver_string+0x0/0x38
>> > > LR is at pxa_camera_probe+0x144/0x428
>> > 
>> > The offending dev_driver_str() here is the one in the dev_warn() call in
>> > mclk_get_divisor().
>> > 
>> > This is what is happening: in struct pxacamera_platform_data I have:
>> > 	.mclk_10khz = 5000,
>> > 
>> > which makes the > test in mclk_get_divisor() succeed calling dev_warn
>> > to report that the clock has been limited, but pcdev->soc_host.dev is
>> > still uninitialized at this time.
Antonio,

Would you check [1] and see if your stack does correspond to the one I reported
some time ago ? As this is fresh in your memory, you'll be far quicker that me.

Ah, and by the way, I like your patch too, agree that mioa701 is touched, and I
think it should go upstream.

Cheers.

--
Robert

[1] http://osdir.com/ml/linux-media/2009-04/msg00874.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Antonio Ospite July 4, 2009, 8:07 p.m. UTC | #4
On Sat, 04 Jul 2009 21:35:22 +0200
Robert Jarzmik <robert.jarzmik@free.fr> wrote:

> >> > The offending dev_driver_str() here is the one in the dev_warn() call in
> >> > mclk_get_divisor().
> >> > 
> >> > This is what is happening: in struct pxacamera_platform_data I have:
> >> > 	.mclk_10khz = 5000,
> >> > 
> >> > which makes the > test in mclk_get_divisor() succeed calling dev_warn
> >> > to report that the clock has been limited, but pcdev->soc_host.dev is
> >> > still uninitialized at this time.
>
> Antonio,
> 
> Would you check [1] and see if your stack does correspond to the one I reported
> some time ago ? As this is fresh in your memory, you'll be far quicker that me.
>
...
> [1] http://osdir.com/ml/linux-media/2009-04/msg00874.html

Yes, I think that is it. The offsets are different of course but the
call stack is pretty much the same.

Regards,
   Antonio
  

Patch

mclk_get_divisor uses pcdev->soc_host.dev, make sure it is initialized.

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 46e0d8a..e048d25 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -1579,6 +1579,7 @@  static int __devinit pxa_camera_probe(struct platform_device *pdev)
 		pcdev->mclk = 20000000;
 	}
 
+	pcdev->soc_host.dev = &pdev->dev;
 	pcdev->mclk_divisor = mclk_get_divisor(pcdev);
 
 	INIT_LIST_HEAD(&pcdev->capture);
@@ -1644,7 +1645,6 @@  static int __devinit pxa_camera_probe(struct platform_device *pdev)
 	pcdev->soc_host.drv_name	= PXA_CAM_DRV_NAME;
 	pcdev->soc_host.ops		= &pxa_soc_camera_host_ops;
 	pcdev->soc_host.priv		= pcdev;
-	pcdev->soc_host.dev		= &pdev->dev;
 	pcdev->soc_host.nr		= pdev->id;
 
 	err = soc_camera_host_register(&pcdev->soc_host);