| Message ID | 51882E90.3020803@ridgerun.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Todd, On Tue, May 7, 2013 at 3:58 AM, Todd Fischer <todd.fischer@ridgerun.com> wrote: > Hi, > > In reviewing vpfe_capture.c, we are seeing some code that seems wrong. We > don't fully understand when the VPFE device lock needs to be held, but it > seems the lock is not used consistently. Also in some cases we found a > pointer to a structure was passed in as a parameter and instead of accessing > the contents of the structure the value of the pointer parameter that is > local to the function was modified (effectively doing nothing). The code To which function are you referring to ? > has been in the kernel for 3+ years. > > Below is an example of what we think needs to be changed. We haven't > tested the changes as we are still trying to verify the mutex usage. > > Can someone familiar with the VPFE code give us some feedback if we are on > the right track? > > Todd > > diff --git a/drivers/media/platform/davinci/vpfe_capture.c > b/drivers/media/platform/davinci/vpfe_capture.c > index 28d019d..dc050b5 100644 > --- a/drivers/media/platform/davinci/vpfe_capture.c > +++ b/drivers/media/platform/davinci/vpfe_capture.c > @@ -945,7 +945,9 @@ static int vpfe_g_fmt_vid_cap(struct file *file, void > *priv, > > v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_g_fmt_vid_cap\n"); > /* Fill in the information about format */ > - *fmt = vpfe_dev->fmt; > + mutex_lock(&vpfe_dev->lock); > + memcpy(fmt, vpfe_dev->fmt, sizeof(struct v4l2_format)); > + mutex_unlock(&vpfe_dev->lock); > return ret; > } This is not required this is something similar to read, so while reading the lock isn’t required. > > @@ -1001,7 +1003,7 @@ static int vpfe_s_fmt_vid_cap(struct file *file, void > *priv, > > /* First detach any IRQ if currently attached */ > vpfe_detach_irq(vpfe_dev); > - vpfe_dev->fmt = *fmt; > + memcpy(vpfe_dev->fmt, fmt, sizeof(struct v4l2_format)); This change also doesn’t make sense as it is one and the same. Regards, --Prabhakar Lad
diff --git a/drivers/media/platform/davinci/vpfe_capture.c b/drivers/media/platform/davinci/vpfe_capture.c index 28d019d..dc050b5 100644 --- a/drivers/media/platform/davinci/vpfe_capture.c +++ b/drivers/media/platform/davinci/vpfe_capture.c @@ -945,7 +945,9 @@ static int vpfe_g_fmt_vid_cap(struct file *file, void *priv, v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_g_fmt_vid_cap\n"); /* Fill in the information about format */ - *fmt = vpfe_dev->fmt; + mutex_lock(&vpfe_dev->lock); + memcpy(fmt, vpfe_dev->fmt, sizeof(struct v4l2_format)); + mutex_unlock(&vpfe_dev->lock); return ret; }
Hi, In reviewing vpfe_capture.c, we are seeing some code that seems wrong. We don't fully understand when the VPFE device lock needs to be held, but it seems the lock is not used consistently. Also in some cases we found a pointer to a structure was passed in as a parameter and instead of accessing the contents of the structure the value of the pointer parameter that is local to the function was modified (effectively doing nothing). The code has been in the kernel for 3+ years. Below is an example of what we think needs to be changed. We haven't tested the changes as we are still trying to verify the mutex usage. Can someone familiar with the VPFE code give us some feedback if we are on the right track? Todd @@ -1001,7 +1003,7 @@ static int vpfe_s_fmt_vid_cap(struct file *file, void *priv, /* First detach any IRQ if currently attached */ vpfe_detach_irq(vpfe_dev); - vpfe_dev->fmt = *fmt; + memcpy(vpfe_dev->fmt, fmt, sizeof(struct v4l2_format)); /* set image capture parameters in the ccdc */ ret = vpfe_config_ccdc_image_format(vpfe_dev); mutex_unlock(&vpfe_dev->lock);