On Mon, 11 Jun 2012, Albert Wang wrote: > Hi, Guennadi > > Sorry, I made a mistake, and I found marvell-ccic used > vb2_dma_contig_init_ctx() when open, but it didn't use the soc_camera. > So for the driver which had used soc_camera, you are right. Our driver should update the usage. > > So the kernel panic due to our driver misunderstand the usage, we can change the patch title. > But I think it's not a "local" variable for vb2_dma_contig_cleanup_ctx(), It's the input parameter. Sorry, I'm not in a position to clarify this. Please, consult your C textbook. Thanks Guennadi > > Since this function has the name "cleanup", it should do it cleanly. And the rule should be free point and set point to NULL. > For users who use this function in there driver, they think things had finished after called this function. > It's safer if user called it twice. > > Anyway, this patch doesn't damage the driver; on the other hand, it can make it more stable if user used it with wrong method. > Doesn't it? Of course, now we should change the patch title. > > Please correct me if I have something wrong. > > > Thanks > Albert Wang > 86-21-61092656 > -----Original Message----- > From: Guennadi Liakhovetski [mailto:g.liakhovetski@xxxxxx] > Sent: Monday, 11 June, 2012 17:21 > To: Albert Wang > Cc: Laurent Pinchart; pawel@xxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] media: videobuf2: fix kernel panic due to missing assign NULL to alloc_ctx > > 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/ > --- 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
