Hi Albert On Mon, 11 Jun 2012, Albert Wang wrote: > Hi, Laurent > > Thanks for your reply! > > We allocated the context when init_videobuf2() which will be called in > soc_camera_open(), so if we get exit with exception in > soc_camera_set_fmt() > Actually we will double call vb2_dma_contig_cleanup_ctx(). > > ret = soc_camera_set_fmt(icd, &f); > if (ret < 0) > goto esfmt; > > if (ici->ops->init_videobuf) { > ici->ops->init_videobuf(&icd->vb_vidq, icd); > } else { > ret = ici->ops->init_videobuf2(&icd->vb2_vidq, icd); > if (ret < 0) > goto einitvb; > } > > Actually, in current code, we can found some drivers allocated the > context in probe(), and some drivers also do that in soc_camera_open(). Sorry, AFAICS all soc-camera host drivers allocate vb2 context in their .probe() methods. Can you point me out to the one(s) you mean, that do that at open() time? > Of course, we can update our driver and move it to probe(), it will > stand aside the issue, that's also OK for us. > > But we still think it's not safe that leave the point be a non-NULL > after we have kfree it. Do you think so? Your patch doesn't fix anything. It only changes the local variable in the vb2_dma_contig_cleanup_ctx() function, which doesn't affect the caller in any way. Thanks Guennadi > Thanks > Albert Wang > 86-21-61092656 > > -----Original Message----- > From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] > Sent: Monday, 11 June, 2012 16:00 > To: Albert Wang > Cc: pawel@xxxxxxxxxx; g.liakhovetski@xxxxxx; linux-media@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx > > Hi Albert, > > On Friday 08 June 2012 19:55:11 Albert Wang wrote: > > In function vb2_dma_contig_cleanup_ctx(), we only kfree the alloc_ctx > > If we didn't assign NULL to this point after kfree it, > > we may encounter the following kernel panic: > > > > kernel BUG at kernel/cred.c:98! > > Unable to handle kernel NULL pointer dereference at virtual address > > 00000000 pgd = c0004000 > > [00000000] *pgd=00000000 > > Internal error: Oops: 817 [#1] PREEMPT SMP Modules linked in: > > runcase_sysfs galcore mv_wtm_drv mv_wtm_prim > > CPU: 0 Not tainted (3.0.8+ #213) > > PC is at __bug+0x18/0x24 > > LR is at __bug+0x14/0x24 > > pc : [<c0054670>] lr : [<c005466c>] psr: 60000113 > > sp : c0681ec0 ip : f683e000 fp : 00000000 > > r10: e8ab4b58 r9 : 00000fff r8 : 00000002 > > r7 : e8665698 r6 : c10079ec r5 : e8b13d80 r4 : e8b13d98 > > r3 : 00000000 r2 : c0681eb4 r1 : c05c9ccc r0 : 00000035 > > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > > Control: 10c53c7d Table: 29c3406a DAC: 00000015 > > > > the root cause is we may encounter some i2c or HW issue with sensor > > which result in driver exit with exception during soc_camera_set_fmt() > > from soc_camera_open(): > > > > ret = soc_camera_set_fmt(icd, &f); > > if (ret < 0) > > goto esfmt; > > > > it will call ici->ops->remove() in following code: > > > > esfmt: > > pm_runtime_disable(&icd->vdev->dev); > > eresume: > > ici->ops->remove(icd); > > > > ici->ops->remove() will call vb2_dma_contig_cleanup_ctx() for cleanup > > but we didn't do ici->ops->init_videobuf2() yet at that time > > it will result in kfree a non-NULL point twice > > I'm not sure to follow you. How is init_videobuf2() related ? The context is allocated once only at probe time from what I can see. Your problem is more likely caused by a double call to vb2_dma_contig_cleanup_ctx(), which looks like a driver bug to me, not a videobuf2 bug. > > > Change-Id: I1c66dd08438ae90abe555c52edcdbca0d39d829d > > Signed-off-by: Albert Wang <twang13@xxxxxxxxxxx> > > --- > > drivers/media/video/videobuf2-dma-contig.c | 1 + > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/media/video/videobuf2-dma-contig.c > > b/drivers/media/video/videobuf2-dma-contig.c index 4b71326..9881171 > > 100755 > > --- a/drivers/media/video/videobuf2-dma-contig.c > > +++ b/drivers/media/video/videobuf2-dma-contig.c > > @@ -178,6 +178,7 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_init_ctx); > > void vb2_dma_contig_cleanup_ctx(void *alloc_ctx) { > > kfree(alloc_ctx); > > + alloc_ctx = NULL; > > } > > EXPORT_SYMBOL_GPL(vb2_dma_contig_cleanup_ctx); > > -- > Regards, > > Laurent Pinchart > --- 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html