Re: [PATCH-next] media: s2255drv: Remove unneeded if else blocks

[walter harms]

Am 24.01.2018 22:40, schrieb Christopher Díaz Riveros:
> Given the following definitions from s2255drv.c
> 
>  #define LINE_SZ_4CIFS_NTSC  640
>  #define LINE_SZ_2CIFS_NTSC  640
>  #define LINE_SZ_1CIFS_NTSC  320
> 
> and
> 
>  #define LINE_SZ_4CIFS_PAL   704
>  #define LINE_SZ_2CIFS_PAL   704
>  #define LINE_SZ_1CIFS_PAL   352
> 
> f->fmt.pix.width possible values can be reduced to
> LINE_SZ_4CIFS_NTSC or LINE_SZ_1CIFS_NTSC.
> 
> This patch removes unneeded if else blocks in vidioc_try_fmt_vid_cap
> function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Christopher Díaz Riveros 

mmmh, yes and no.
i guess the author tries to document the change from 4->2->1

The whole thing gets more obvoius when you use hex and look at the bits:
704 = 0x2C0 = 00101100
640 = 0x280 = 00101000
352 = 0x160 = 00010110
320 = 0x140 = 00010100

so they only flip one bit and shift the mask. perhaps you can use that
to simplify the code ?

re
 wh
> ---
>  drivers/media/usb/s2255/s2255drv.c | 8 
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/media/usb/s2255/s2255drv.c 
> b/drivers/media/usb/s2255/s2255drv.c
> index 8c2a86d71e8a..a00a15f55d37 100644
> --- a/drivers/media/usb/s2255/s2255drv.c
> +++ b/drivers/media/usb/s2255/s2255drv.c
> @@ -803,10 +803,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, 
> void *priv,
>   }
>   if (f->fmt.pix.width >= LINE_SZ_4CIFS_NTSC)
>   f->fmt.pix.width = LINE_SZ_4CIFS_NTSC;
> - else if (f->fmt.pix.width >= LINE_SZ_2CIFS_NTSC)
> - f->fmt.pix.width = LINE_SZ_2CIFS_NTSC;
> - else if (f->fmt.pix.width >= LINE_SZ_1CIFS_NTSC)
> - f->fmt.pix.width = LINE_SZ_1CIFS_NTSC;
>   else
>   f->fmt.pix.width = LINE_SZ_1CIFS_NTSC;
>   } else {
> @@ -820,10 +816,6 @@ static int vidioc_try_fmt_vid_cap(struct file *file, 
> void *priv,
>   }
>   if (f->fmt.pix.width >= LINE_SZ_4CIFS_PAL)
>   f->fmt.pix.width = LINE_SZ_4CIFS_PAL;
> - else if (f->fmt.pix.width >= LINE_SZ_2CIFS_PAL)
> - f->fmt.pix.width = LINE_SZ_2CIFS_PAL;
> - else if (f->fmt.pix.width >= LINE_SZ_1CIFS_PAL)
> - f->fmt.pix.width = LINE_SZ_1CIFS_PAL;
>   else
>   f->fmt.pix.width = LINE_SZ_1CIFS_PAL;
>   }

Re: [PATCH 2/2] staging/atomisp: putting NULs in the wrong place

[walter harms]

Am 15.05.2017 12:01, schrieb Dan Carpenter:
> We're putting the NUL terminators one space beyond where they belong.
> This doesn't show up in testing because all but the callers put a NUL in
> the correct place themselves.  LOL.  It causes a static checker warning
> about buffer overflows.
> 
> Fixes: a49d25364dfb ("staging/atomisp: Add support for the Intel IPU v2")
> Signed-off-by: Dan Carpenter 
> 
> diff --git 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
>  
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> index 74b5a1c7ac9a..c53241a7a281 100644
> --- 
> a/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> +++ 
> b/drivers/staging/media/atomisp/pci/atomisp2/css2400/hive_isp_css_include/string_support.h
> @@ -117,7 +117,7 @@ STORAGE_CLASS_INLINE int strncpy_s(
>  
>   /* dest_str is big enough for the len */
>   strncpy(dest_str, src_str, len);
> - dest_str[len+1] = '\0';
> + dest_str[len] = '\0';
>   return 0;
>  }
>  
> @@ -157,7 +157,7 @@ STORAGE_CLASS_INLINE int strcpy_s(
>  
>   /* dest_str is big enough for the len */
>   strncpy(dest_str, src_str, len);
> - dest_str[len+1] = '\0';
> + dest_str[len] = '\0';
>   return 0;
>  }
>  

can this strcpy_s() replaced with strlcpy ?

re,
 wh


> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] staging: atomisp: use local variable to reduce the number of reference

[walter harms]

Am 30.03.2017 08:25, schrieb Daeseok Youn:
> Define new local variable to reduce the number of reference.
> The new local variable is added to save the addess of dfs
> and used in atomisp_freq_scaling() function.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 37 
> --
>  1 file changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index eebfccd..d76a95c 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -251,6 +251,7 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>  {
>   /* FIXME! Only use subdev[0] status yet */
>   struct atomisp_sub_device *asd = >asd[0];
> + const struct atomisp_dfs_config *dfs;
>   unsigned int new_freq;
>   struct atomisp_freq_scaling_rule curr_rules;
>   int i, ret;
> @@ -268,20 +269,22 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>   ATOMISP_USE_YUVPP(asd))
>   isp->dfs = _config_cht_soc;
>  
> - if (isp->dfs->lowest_freq == 0 || isp->dfs->max_freq_at_vmin == 0 ||
> - isp->dfs->highest_freq == 0 || isp->dfs->dfs_table_size == 0 ||
> - !isp->dfs->dfs_table) {
> + dfs = isp->dfs;
> +
> + if (dfs->lowest_freq == 0 || dfs->max_freq_at_vmin == 0 ||
> + dfs->highest_freq == 0 || dfs->dfs_table_size == 0 ||
> + !dfs->dfs_table) {
>   dev_err(isp->dev, "DFS configuration is invalid.\n");
>   return -EINVAL;
>   }
>  
>   if (mode == ATOMISP_DFS_MODE_LOW) {
> - new_freq = isp->dfs->lowest_freq;
> + new_freq = dfs->lowest_freq;
>   goto done;
>   }
>  
>   if (mode == ATOMISP_DFS_MODE_MAX) {
> - new_freq = isp->dfs->highest_freq;
> + new_freq = dfs->highest_freq;
>   goto done;
>   }
>  
> @@ -307,26 +310,26 @@ int atomisp_freq_scaling(struct atomisp_device *isp,
>   }
>  
>   /* search for the target frequency by looping freq rules*/
> - for (i = 0; i < isp->dfs->dfs_table_size; i++) {
> - if (curr_rules.width != isp->dfs->dfs_table[i].width &&
> - isp->dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
> + for (i = 0; i < dfs->dfs_table_size; i++) {
> + if (curr_rules.width != dfs->dfs_table[i].width &&
> + dfs->dfs_table[i].width != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.height != isp->dfs->dfs_table[i].height &&
> - isp->dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
> + if (curr_rules.height != dfs->dfs_table[i].height &&
> + dfs->dfs_table[i].height != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.fps != isp->dfs->dfs_table[i].fps &&
> - isp->dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
> + if (curr_rules.fps != dfs->dfs_table[i].fps &&
> + dfs->dfs_table[i].fps != ISP_FREQ_RULE_ANY)
>   continue;
> - if (curr_rules.run_mode != isp->dfs->dfs_table[i].run_mode &&
> - isp->dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
> + if (curr_rules.run_mode != dfs->dfs_table[i].run_mode &&
> + dfs->dfs_table[i].run_mode != ISP_FREQ_RULE_ANY)
>   continue;
>   break;
>   }

>  
> - if (i == isp->dfs->dfs_table_size)
> - new_freq = isp->dfs->max_freq_at_vmin;
> + if (i == dfs->dfs_table_size)
> + new_freq = dfs->max_freq_at_vmin;
>   else
> - new_freq = isp->dfs->dfs_table[i].isp_freq;
> + new_freq = dfs->dfs_table[i].isp_freq;
>  

you can eliminate the last block by setting

 new_freq = dfs->max_freq_at_vmin;

  for(i=0;) {

new_freq = dfs->dfs_table[i].isp_freq;
break;
}

unfortunately i have no good idea how to make the loop more readable.


re,
 wh


>  done:
>   dev_dbg(isp->dev, "DFS target frequency=%d.\n", new_freq);

Re: [PATCH] [media] coda: remove redundant call to v4l2_m2m_get_vq

[walter harms]

Am 27.03.2017 12:10, schrieb Philipp Zabel:
> On Mon, 2017-03-27 at 11:46 +0200, Hans Verkuil wrote:
>> On 23/03/17 12:57, Colin King wrote:
>>> From: Colin Ian King 
>>>
>>> The call to v4ls_m2m_get_vq is only used to get the return value
>>> which is not being used, so it appears to be redundant and can
>>> be removed.
>>>
>>> Detected with CoverityScan, CID#1420674 ("Useless call")
>>>
>>> Signed-off-by: Colin Ian King 
>>> ---
>>>  drivers/media/platform/coda/coda-common.c | 2 --
>>>  1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/coda/coda-common.c 
>>> b/drivers/media/platform/coda/coda-common.c
>>> index 800d2477f1a0..95e4648f18e6 100644
>>> --- a/drivers/media/platform/coda/coda-common.c
>>> +++ b/drivers/media/platform/coda/coda-common.c
>>> @@ -817,8 +817,6 @@ static int coda_qbuf(struct file *file, void *priv,
>>>  static bool coda_buf_is_end_of_stream(struct coda_ctx *ctx,
>>>   struct vb2_v4l2_buffer *buf)
>>>  {
>>> -   v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT);
>>> -
>>> return ((ctx->bit_stream_param & CODA_BIT_STREAM_END_FLAG) &&
>>> (buf->sequence == (ctx->qsequence - 1)));
>>>  }
>>>
>>
>> Philipp, is this correct, or should this actually check whether the queue is 
>> an
>> output queue?
> 
> Yes, this was previously assigned to an unused local variable src_vq,
> since initial commit 918c66fd4126 ("[media] coda: add CODA7541 decoding
> support").
> 
> coda_buf_is_end_of_stream is called from coda_m2m_buf_done, which is
> exclusively used on destination buffers on the capture queue.
> 
> Acked-by: Philipp Zabel 
> 
> regards
> Philipp
> 

Is that function needed at all ?

re,
 wh

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

Am 20.03.2017 13:51, schrieb DaeSeok Youn:
> 2017-03-20 21:04 GMT+09:00 walter harms <wha...@bfs.de>:
>>
>>
>> Am 20.03.2017 11:59, schrieb Daeseok Youn:
>>> If v4l2_subdev_call() gets the global frame interval values,
>>> it returned 0 and it could be checked whether numerator is zero or not.
>>>
>>> If the numerator is not zero, the fps could be calculated in this function.
>>> If not, it just returns 0.
>>>
>>> Signed-off-by: Daeseok Youn <daeseok.y...@gmail.com>
>>> ---
>>>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
>>> ++
>>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
>>> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> index 8bdb224..6bdd19e 100644
>>> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
>>> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
>>> video_device *dev)
>>>
>>>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device 
>>> *asd)
>>>  {
>>> - struct v4l2_subdev_frame_interval frame_interval;
>>> + struct v4l2_subdev_frame_interval fi;
>>>   struct atomisp_device *isp = asd->isp;
>>> - unsigned short fps;
>>>
>>> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> - video, g_frame_interval, _interval)) {
>>> - fps = 0;
>>> - } else {
>>> - if (frame_interval.interval.numerator)
>>> - fps = frame_interval.interval.denominator /
>>> - frame_interval.interval.numerator;
>>> - else
>>> - fps = 0;
>>> - }
>>> + unsigned short fps = 0;
>>> + int ret;
>>> +
>>> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
>>> +video, g_frame_interval, );
>>> +
>>> + if (!ret && fi.interval.numerator)
>>> + fps = fi.interval.denominator / fi.interval.numerator;
>>> +
>>>   return fps;
>>>  }
>>
>>
>>
>> do you need to check ret at all ? if an error occurs can 
>> fi.interval.numerator
>> be something else than 0 ?
> the return value from the v4l2_subdev_call() function is zero when it
> is done without any error. and also I checked
> the ret value whether is 0 or not. if the ret is 0 then the value of
> numerator should be checked to avoid for dividing by 0.
>>
>> if ret is an ERRNO it would be wise to return ret not fps, but this may 
>> require
>> changes at other places also.
> hmm.., yes, you are right. but I think it is ok because the
> atomisp_get_sensor_fps() function is needed to get fps value.
> (originally, zero or calculated fps value was returned.)

maybe its better to divide this in:
if (ret)
   return 0; // error case

return (fi.interval.numerator>0)?fi.interval.denominator / 
fi.interval.numerator:0;

So there is a chance that someone will a) understand and b) fix the error 
return.

re,
 wh

> 
>>
>> re,
>>  wh
>>
>>>
> 

Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

[walter harms]

Am 20.03.2017 11:59, schrieb Daeseok Youn:
> If v4l2_subdev_call() gets the global frame interval values,
> it returned 0 and it could be checked whether numerator is zero or not.
> 
> If the numerator is not zero, the fps could be calculated in this function.
> If not, it just returns 0.
> 
> Signed-off-by: Daeseok Youn 
> ---
>  .../media/atomisp/pci/atomisp2/atomisp_cmd.c   | 22 
> ++
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c 
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index 8bdb224..6bdd19e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -153,20 +153,18 @@ struct atomisp_acc_pipe *atomisp_to_acc_pipe(struct 
> video_device *dev)
>  
>  static unsigned short atomisp_get_sensor_fps(struct atomisp_sub_device *asd)
>  {
> - struct v4l2_subdev_frame_interval frame_interval;
> + struct v4l2_subdev_frame_interval fi;
>   struct atomisp_device *isp = asd->isp;
> - unsigned short fps;
>  
> - if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> - video, g_frame_interval, _interval)) {
> - fps = 0;
> - } else {
> - if (frame_interval.interval.numerator)
> - fps = frame_interval.interval.denominator /
> - frame_interval.interval.numerator;
> - else
> - fps = 0;
> - }
> + unsigned short fps = 0;
> + int ret;
> +
> + ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> +video, g_frame_interval, );
> +
> + if (!ret && fi.interval.numerator)
> + fps = fi.interval.denominator / fi.interval.numerator;
> +
>   return fps;
>  }



do you need to check ret at all ? if an error occurs can fi.interval.numerator
be something else than 0 ?

if ret is an ERRNO it would be wise to return ret not fps, but this may require
changes at other places also.

re,
 wh

>  

Re: [PATCH] staging: atomisp: clean up return logic, remove redunant code

[walter harms]

Am 11.03.2017 20:32, schrieb Colin King:
> From: Colin Ian King 
> 
> There is no need to check if ret is non-zero, remove this
> redundant check and just return the error status from the call
> to mt9m114_write_reg_array.
> 
> Detected by CoverityScan, CID#1416577 ("Identical code for
> different branches")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/staging/media/atomisp/i2c/mt9m114.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/mt9m114.c 
> b/drivers/staging/media/atomisp/i2c/mt9m114.c
> index 8762124..a555aec 100644
> --- a/drivers/staging/media/atomisp/i2c/mt9m114.c
> +++ b/drivers/staging/media/atomisp/i2c/mt9m114.c
> @@ -444,12 +444,8 @@ static int mt9m114_set_suspend(struct v4l2_subdev *sd)
>  static int mt9m114_init_common(struct v4l2_subdev *sd)
>  {
>   struct i2c_client *client = v4l2_get_subdevdata(sd);
> - int ret;
>  
> - ret = mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
> - if (ret)
> - return ret;
> - return ret;
> + return mt9m114_write_reg_array(client, mt9m114_common, PRE_POLLING);
>  }


any use for "client" ?

re,
 wh

>  
>  static int power_ctrl(struct v4l2_subdev *sd, bool flag)

Re: [PATCH] [media] atmel-isc: fix off-by-one comparison and out of bounds read issue

[walter harms]

Am 09.03.2017 11:57, schrieb Hans Verkuil:
> Hi Songjun,
> 
> On 08/03/17 03:25, Wu, Songjun wrote:
>> Hi Colin,
>>
>> Thank you for your comment.
>> It is a bug, will be fixed in the next patch.
> 
> Do you mean that you will provide a new patch for this? Is there anything
> wrong with this patch? It seems reasonable to me.
> 
> Regards,
> 
>   Hans
> 



perhaps he will make it a bit more readable, like:

*hist_count += i * (*hist_entry++);

*hist_count += hist_entry[i]*i;


re,
 wh
>>
>> On 3/7/2017 22:30, Colin King wrote:
>>> From: Colin Ian King 
>>>
>>> The are only HIST_ENTRIES worth of entries in  hist_entry however the
>>> for-loop is iterating one too many times leasing to a read access off
>>> the end off the array ctrls->hist_entry.  Fix this by iterating by
>>> the correct number of times.
>>>
>>> Detected by CoverityScan, CID#1415279 ("Out-of-bounds read")
>>>
>>> Signed-off-by: Colin Ian King 
>>> ---
>>>  drivers/media/platform/atmel/atmel-isc.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c 
>>> b/drivers/media/platform/atmel/atmel-isc.c
>>> index b380a7d..7dacf8c 100644
>>> --- a/drivers/media/platform/atmel/atmel-isc.c
>>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>>> @@ -1298,7 +1298,7 @@ static void isc_hist_count(struct isc_device *isc)
>>>  regmap_bulk_read(regmap, ISC_HIS_ENTRY, hist_entry, HIST_ENTRIES);
>>>
>>>  *hist_count = 0;
>>> -for (i = 0; i <= HIST_ENTRIES; i++)
>>> +for (i = 0; i < HIST_ENTRIES; i++)
>>>  *hist_count += i * (*hist_entry++);
>>>  }
>>>
>>>
> 




> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch v2] staging: bcm2835-camera: fix error handling in init

[walter harms]

looks more readable now :)

Acked-by: wha...@bfs.de

Am 18.02.2017 00:20, schrieb Dan Carpenter:
> The unwinding here isn't right.  We don't free gdev[0] and instead
> free 1 step past what was allocated.  Also we can't allocate "dev" then
> we should unwind instead of returning directly.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
> driver.")
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> ---
> v2: Change the style to make Walter Harms happy.  Fix some additional
> bugs I missed in the first patch.
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
> b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..c4dad30dd133 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1901,6 +1901,7 @@ static int __init bm2835_mmal_init(void)
>   unsigned int num_cameras;
>   struct vchiq_mmal_instance *instance;
>   unsigned int resolutions[MAX_BCM2835_CAMERAS][2];
> + int i;
>  
>   ret = vchiq_mmal_init();
>   if (ret < 0)
> @@ -1914,8 +1915,10 @@ static int __init bm2835_mmal_init(void)
>  
>   for (camera = 0; camera < num_cameras; camera++) {
>   dev = kzalloc(sizeof(struct bm2835_mmal_dev), GFP_KERNEL);
> - if (!dev)
> - return -ENOMEM;
> + if (!dev) {
> + ret = -ENOMEM;
> + goto cleanup_gdev;
> + }
>  
>   dev->camera_num = camera;
>   dev->max_width = resolutions[camera][0];
> @@ -1998,9 +2001,10 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>   kfree(dev);
>  
> - for ( ; camera > 0; camera--) {
> - bcm2835_cleanup_instance(gdev[camera]);
> - gdev[camera] = NULL;
> +cleanup_gdev:
> + for (i = 0; i < camera; i++) {
> + bcm2835_cleanup_instance(gdev[i]);
> + gdev[i] = NULL;
>   }
>   pr_info("%s: error %d while loading driver\n",
>   BM2835_MMAL_MODULE_NAME, ret);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch] staging: bcm2835-camera: free first element in array

[walter harms]

Am 15.02.2017 13:25, schrieb Dan Carpenter:
> We should free gdev[0] so the > should be >=.
> 
> Fixes: 7b3ad5abf027 ("staging: Import the BCM2835 MMAL-based V4L2 camera 
> driver.")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c 
> b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> index ca15a698e018..9bcd8e546a14 100644
> --- a/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> +++ b/drivers/staging/media/platform/bcm2835/bcm2835-camera.c
> @@ -1998,7 +1998,7 @@ static int __init bm2835_mmal_init(void)
>  free_dev:
>   kfree(dev);
>  
> - for ( ; camera > 0; camera--) {
> + for ( ; camera >= 0; camera--) {
>   bcm2835_cleanup_instance(gdev[camera]);
>   gdev[camera] = NULL;
>   }

since we already know that programmers are bad in counting backwards ...

is is possible to change that into std. loop like:

 for(i=0, i< camera; i++ {
bcm2835_cleanup_instance(gdev[i]);
gdev[i] = NULL;
}

this is way a much more common pattern.


just my 2 cents,
re,
 wh

Re: [patch] [media] mantis_dvb: fix some error codes in mantis_dvb_init()

[walter harms]

Am 27.01.2017 09:06, schrieb Dan Carpenter:
> We should be returning negative error codes here or it leads to a crash.
> This also silences a static checker warning.
> 
>   drivers/media/pci/mantis/mantis_cards.c:250 mantis_pci_probe()
>   warn: 'mantis->dmxdev.dvbdev->fops' double freed
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/media/pci/mantis/mantis_dvb.c 
> b/drivers/media/pci/mantis/mantis_dvb.c
> index 5a71e1791cf5..0db4de3a2285 100644
> --- a/drivers/media/pci/mantis/mantis_dvb.c
> +++ b/drivers/media/pci/mantis/mantis_dvb.c
> @@ -226,11 +226,12 @@ int mantis_dvb_init(struct mantis_pci *mantis)
>   goto err5;
>   } else {
>   if (mantis->fe == NULL) {
> + result = -ENOMEM;
>   dprintk(MANTIS_ERROR, 1, "FE ");
>   goto err5;
>   }
> -
> - if (dvb_register_frontend(>dvb_adapter, 
> mantis->fe)) {
> + result = dvb_register_frontend(>dvb_adapter, 
> mantis->fe);
> + if (result) {
>   dprintk(MANTIS_ERROR, 1, "ERROR: Frontend 
> registration failed");
>  
>   if (mantis->fe->ops.release)


hi,
just one remark:
the indent level is deep.
using  if ( !mantis->hwconfig) return 0;
and killing the "else" would help with readability.

just my 2 cents
re,
 wh

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] uvcvideo: freeing an error pointer

[walter harms]

Am 25.11.2016 14:57, schrieb Laurent Pinchart:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Friday 25 Nov 2016 13:28:35 Dan Carpenter wrote:
>> A recent cleanup introduced a potential dereference of -EFAULT when we
>> call kfree(map->menu_info).
> 
> I should have caught that, my apologies :-(
> 
> Thinking a bit more about this class of problems, would the following patch 
> make sense ?
> 
> commit 034b71306510643f9f059249a0c14418099eb436
> Author: Laurent Pinchart 
> Date:   Fri Nov 25 15:54:22 2016 +0200
> 
> mm/slab: WARN_ON error pointers passed to kfree()
> 
> Passing an error pointer to kfree() is invalid and can lead to crashes
> or memory corruption. Reject those pointers and WARN in order to catch
> the problems and fix them in the callers.
> 
> Signed-off-by: Laurent Pinchart 
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 0b0550ca85b4..a7eb830c6684 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3819,6 +3819,8 @@ void kfree(const void *objp)
>  
>   if (unlikely(ZERO_OR_NULL_PTR(objp)))
>   return;
> + if (WARN_ON(IS_ERR(objp)))
> + return;
>   local_irq_save(flags);
>   kfree_debugcheck(objp);
>   c = virt_to_cache(objp);


I this is better in kfree_debugcheck().
1. it has the right name
2. is contains already a check

static void kfree_debugcheck(const void *objp)
 {
 if (!virt_addr_valid(objp)) {
 pr_err("kfree_debugcheck: out of range ptr %lxh\n",
(unsigned long)objp);
 BUG();
 }
  }

btw: should this read %p ?

re,
 wh



>> Fixes: 4cc5bed1caeb ("[media] uvcvideo: Use memdup_user() rather than
>> duplicating its implementation")
>> Signed-off-by: Dan Carpenter 
> 
> Reviewed-by: Laurent Pinchart 
> 
> Mauro, the bug is present in your branch only at the moment and queued for 
> v4.10. Could you please pick this patch up as well ?
> 
>> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
>> b/drivers/media/usb/uvc/uvc_v4l2.c index a7e12fd..3e7e283 100644
>> --- a/drivers/media/usb/uvc/uvc_v4l2.c
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -66,14 +66,14 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, if (xmap->menu_count == 0 ||
>>  xmap->menu_count > UVC_MAX_CONTROL_MENU_ENTRIES) {
>>  ret = -EINVAL;
>> -goto done;
>> +goto free_map;
>>  }
>>
>>  size = xmap->menu_count * sizeof(*map->menu_info);
>>  map->menu_info = memdup_user(xmap->menu_info, size);
>>  if (IS_ERR(map->menu_info)) {
>>  ret = PTR_ERR(map->menu_info);
>> -goto done;
>> +goto free_map;
>>  }
>>
>>  map->menu_count = xmap->menu_count;
>> @@ -83,13 +83,13 @@ static int uvc_ioctl_ctrl_map(struct uvc_video_chain
>> *chain, uvc_trace(UVC_TRACE_CONTROL, "Unsupported V4L2 control type "
>>"%u.\n", xmap->v4l2_type);
>>  ret = -ENOTTY;
>> -goto done;
>> +goto free_map;
>>  }
>>
>>  ret = uvc_ctrl_add_mapping(chain, map);
>>
>> -done:
>>  kfree(map->menu_info);
>> +free_map:
>>  kfree(map);
>>
>>  return ret;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] dvb-tc90522: Rename a jump label in tc90522_probe()

[walter harms]

Am 07.10.2016 21:46, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Fri, 7 Oct 2016 21:13:57 +0200
> 
> Adjust a jump label according to the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/media/dvb-frontends/tc90522.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/dvb-frontends/tc90522.c 
> b/drivers/media/dvb-frontends/tc90522.c
> index c2d45f0..4687e15 100644
> --- a/drivers/media/dvb-frontends/tc90522.c
> +++ b/drivers/media/dvb-frontends/tc90522.c
> @@ -794,14 +794,13 @@ static int tc90522_probe(struct i2c_client *client,
>   i2c_set_adapdata(adap, state);
>   ret = i2c_add_adapter(adap);
>   if (ret < 0)
> - goto err;
> + goto free_state;
>   cfg->tuner_i2c = state->cfg.tuner_i2c = adap;
>  
>   i2c_set_clientdata(client, >cfg);
>   dev_info(>dev, "Toshiba TC90522 attached.\n");
>   return 0;
> -
> -err:
> +free_state:
>   kfree(state);
>   return ret;
>  }

there is only one user, IMHO this can be moved to the if block.

re,
 wh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] v4l2-common: Delete an unnecessary check before the function call "spi_unregister_device"

[walter harms]

Am 19.07.2016 20:02, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Tue, 19 Jul 2016 19:54:16 +0200
> 
> The spi_unregister_device() function tests whether its argument is NULL
> and then returns immediately. Thus the test around the call is not needed.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c 
> b/drivers/media/v4l2-core/v4l2-common.c
> index 5b80850..57cfe26a 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -291,7 +291,7 @@ struct v4l2_subdev *v4l2_spi_new_subdev(struct 
> v4l2_device *v4l2_dev,
>  error:
>   /* If we have a client but no subdev, then something went wrong and
>  we must unregister the client. */
> - if (spi && sd == NULL)
> + if (!sd)
>   spi_unregister_device(spi);
>  
>   return sd;


if i read the code correct sd is always NULL at this point.
so this was wrong in the first place and you must remove sd also.


re,
 wh




--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] c8sectpfe: Combine three checks into a single if block

[walter harms]

Am 05.11.2015 19:50, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Thu, 5 Nov 2015 19:23:50 +0100
> 
> The variable "tsin" was checked three times in a loop iteration of the
> c8sectpfe_tuner_unregister_frontend() function.
> This implementation detail could be improved by the combination of the
> involved statements into a single if block so that this variable will be
> checked only once there.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/media/platform/sti/c8sectpfe/c8sectpfe-common.c | 17 
> +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/sti/c8sectpfe/c8sectpfe-common.c 
> b/drivers/media/platform/sti/c8sectpfe/c8sectpfe-common.c
> index 07fd6d9..2dfbe8a 100644
> --- a/drivers/media/platform/sti/c8sectpfe/c8sectpfe-common.c
> +++ b/drivers/media/platform/sti/c8sectpfe/c8sectpfe-common.c
> @@ -209,17 +209,18 @@ void c8sectpfe_tuner_unregister_frontend(struct 
> c8sectpfe *c8sectpfe,
>  
>   tsin = fei->channel_data[n];


if you do "if (!tsin) continue ;"
you can save one indent level

re,
 wh

>  
> - if (tsin && tsin->frontend) {
> - dvb_unregister_frontend(tsin->frontend);
> - dvb_frontend_detach(tsin->frontend);
> - }
> + if (tsin) {
> + if (tsin->frontend) {
> + dvb_unregister_frontend(tsin->frontend);
> + dvb_frontend_detach(tsin->frontend);
> + }
>  
> - if (tsin)
>   i2c_put_adapter(tsin->i2c_adapter);
>  
> - if (tsin && tsin->i2c_client) {
> - module_put(tsin->i2c_client->dev.driver->owner);
> - i2c_unregister_device(tsin->i2c_client);
> + if (tsin->i2c_client) {
> + module_put(tsin->i2c_client->dev.driver->owner);
> + i2c_unregister_device(tsin->i2c_client);
> + }
>   }
>   }
>  
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] [media] mantis: fix error handling

[walter harms]

Am 22.03.2015 18:16, schrieb Silvan Jegen:
 Remove dead code, make goto label names more expressive and add a label
 in order to call mantis_dvb_exit if mantis_uart_init fails.
 
 Also make sure that mantis_pci_exit is called if we fail the
 mantis_stream_control call and that we call mantis_i2c_exit if
 mantis_get_mac fails.
 
 Signed-off-by: Silvan Jegen s.je...@gmail.com
 ---
 V2 Changes (due to Dan Carpenter's review):
   - Remove dead code, do not activate it
   - Make goto labels more expressive
   - Add a call to mantis_dvb_exit
 
  drivers/media/pci/mantis/mantis_cards.c | 33 
 -
  1 file changed, 16 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/media/pci/mantis/mantis_cards.c 
 b/drivers/media/pci/mantis/mantis_cards.c
 index 801fc55..70df61e 100644
 --- a/drivers/media/pci/mantis/mantis_cards.c
 +++ b/drivers/media/pci/mantis/mantis_cards.c
 @@ -170,7 +170,7 @@ static int mantis_pci_probe(struct pci_dev *pdev,
   if (mantis == NULL) {
   printk(KERN_ERR %s ERROR: Out of memory\n, __func__);
   err = -ENOMEM;
 - goto fail0;
 + return err;
   }
  
   mantis-num = devs;
 @@ -183,70 +183,69 @@ static int mantis_pci_probe(struct pci_dev *pdev,
   err = mantis_pci_init(mantis);
   if (err) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis PCI initialization 
 failed %d, err);
 - goto fail1;
 + goto err_free_mantis;
   }
  
   err = mantis_stream_control(mantis, STREAM_TO_HIF);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis stream control failed 
 %d, err);
 - goto fail1;
 + goto err_pci_exit;
   }
  
   err = mantis_i2c_init(mantis);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis I2C initialization 
 failed %d, err);
 - goto fail2;
 + goto err_pci_exit;
   }
  
   err = mantis_get_mac(mantis);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis MAC address read failed 
 %d, err);
 - goto fail2;
 + goto err_i2c_exit;
   }
  
   err = mantis_dma_init(mantis);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis DMA initialization 
 failed %d, err);
 - goto fail3;
 + goto err_i2c_exit;
   }
  
   err = mantis_dvb_init(mantis);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis DVB initialization 
 failed %d, err);
 - goto fail4;
 + goto err_dma_exit;
   }
 +
   err = mantis_uart_init(mantis);
   if (err  0) {
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis UART initialization 
 failed %d, err);
 - goto fail6;
 + goto err_dvb_exit;
   }
  
   devs++;
  
   return err;
  

Hi Silvan Jegen,
i found the dprintk() a bit confusing, if i understand the code correctly it 
will always print on error:

the dprintf found in if (err0) like :
 dprintk(MANTIS_ERROR, 1, ERROR: Mantis DVB initialization failed %d, err);
and then
  dprintk(MANTIS_ERROR, 1, ERROR: Mantis DVB exit! %d, err);

maybe this is more a question for the maintainer, but this seems a bit useless.

re,
 wh


 +err_dvb_exit:
 + dprintk(MANTIS_ERROR, 1, ERROR: Mantis DVB exit! %d, err);
 + mantis_dvb_exit(mantis);
  
 - dprintk(MANTIS_ERROR, 1, ERROR: Mantis UART exit! %d, err);
 - mantis_uart_exit(mantis);
 -
 -fail6:
 -fail4:
 +err_dma_exit:
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis DMA exit! %d, err);
   mantis_dma_exit(mantis);
  
 -fail3:
 +err_i2c_exit:
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis I2C exit! %d, err);
   mantis_i2c_exit(mantis);
  
 -fail2:
 +err_pci_exit:
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis PCI exit! %d, err);
   mantis_pci_exit(mantis);
  
 -fail1:
 +err_free_mantis:
   dprintk(MANTIS_ERROR, 1, ERROR: Mantis free! %d, err);
   kfree(mantis);
  
 -fail0:
   return err;
  }
  
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] coda: improve safety in coda_register_device()

[walter harms]

Am 08.01.2015 11:07, schrieb Dan Carpenter:
 The i variable is used as an offset into both the dev-vfd[] and the
 dev-devtype-vdevs[] arrays.  The second array is smaller so we should
 use that as a limit instead of ARRAY_SIZE(dev-vfd).  Also the original
 check was off by one.
 
 We should use a format string as well in case the -name has any funny
 characters and also to stop static checkers from complaining.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/media/platform/coda/coda-common.c 
 b/drivers/media/platform/coda/coda-common.c
 index 39330a7..5dd6cae 100644
 --- a/drivers/media/platform/coda/coda-common.c
 +++ b/drivers/media/platform/coda/coda-common.c
 @@ -1844,10 +1844,11 @@ static int coda_register_device(struct coda_dev *dev, 
 int i)
  {
   struct video_device *vfd = dev-vfd[i];
  
 - if (i  ARRAY_SIZE(dev-vfd))
 + if (i = dev-devtype-num_vdevs)
   return -EINVAL;

hi,
 just a minor question. if i can not be trusted, i feel you should move the
 array access:
   struct video_device *vfd = dev-vfd[i];
 after the check
   i = dev-devtype-num_vdevs
at least that would improve the readability by not trigger my internal alarm
check after access

re,
 wh


 - snprintf(vfd-name, sizeof(vfd-name), dev-devtype-vdevs[i]-name);
 + snprintf(vfd-name, sizeof(vfd-name), %s,
 +  dev-devtype-vdevs[i]-name);
   vfd-fops   = coda_fops;
   vfd-ioctl_ops  = coda_ioctl_ops;
   vfd-release= video_device_release_empty,
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2] [media] stv090x: remove indent levels in stv090x_get_coldlock()

[walter harms]

Am 21.02.2014 09:50, schrieb Dan Carpenter:
 This code is needlessly complicated and checkpatch.pl complains that we
 go over the 80 characters per line limit.
 
 If we flip the if (!lock) { test to if (lock) return; then we can
 remove an indent level from the rest of the function.
 
 We can add two returns in the if (state-srate = 1000) {
 condition and move the else statement back an additional indent level.
 
 There is another if (!lock) { check which can be removed since we have
 already checked lock and know it is zero at this point.  This second
 check on lock is also a problem because it sets off a static checker
 warning.  I have reviewed this code for some time to see if something
 else was intended, but have concluded that it was simply an oversight
 and should be removed.  Removing this duplicative check gains us an
 third indent level.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 v2: add the returns in the if (state-srate = 1000) { condition.
 
 diff --git a/drivers/media/dvb-frontends/stv090x.c 
 b/drivers/media/dvb-frontends/stv090x.c
 index 23e872f84742..93f4979ea6e9 100644
 --- a/drivers/media/dvb-frontends/stv090x.c
 +++ b/drivers/media/dvb-frontends/stv090x.c
 @@ -2146,7 +2146,7 @@ static int stv090x_get_coldlock(struct stv090x_state 
 *state, s32 timeout_dmd)
  
   u32 reg;
   s32 car_step, steps, cur_step, dir, freq, timeout_lock;
 - int lock = 0;
 + int lock;
  
   if (state-srate = 1000)
   timeout_lock = timeout_dmd / 3;
 @@ -2154,98 +2154,96 @@ static int stv090x_get_coldlock(struct stv090x_state 
 *state, s32 timeout_dmd)
   timeout_lock = timeout_dmd / 2;
  
   lock = stv090x_get_dmdlock(state, timeout_lock); /* cold start wait */
 - if (!lock) {
 - if (state-srate = 1000) {
 - if (stv090x_chk_tmg(state)) {
 - if (STV090x_WRITE_DEMOD(state, DMDISTATE, 0x1f) 
  0)
 - goto err;
 - if (STV090x_WRITE_DEMOD(state, DMDISTATE, 0x15) 
  0)
 - goto err;
 - lock = stv090x_get_dmdlock(state, timeout_dmd);
 - } else {
 - lock = 0;
 - }
 - } else {
 - if  (state-srate =  400)
 - car_step = 1000;
 - else if (state-srate =  700)
 - car_step = 2000;
 - else if (state-srate = 1000)
 - car_step = 3000;
 - else
 - car_step = 5000;
 -
 - steps  = (state-search_range / 1000) / car_step;
 - steps /= 2;
 - steps  = 2 * (steps + 1);
 - if (steps  0)
 - steps = 2;
 - else if (steps  12)
 - steps = 12;
 -
 - cur_step = 1;
 - dir = 1;
 -
 - if (!lock) {
 - freq = state-frequency;
 - state-tuner_bw = 
 stv090x_car_width(state-srate, state-rolloff) + state-srate;
 - while ((cur_step = steps)  (!lock)) {
 - if (dir  0)
 - freq += cur_step * car_step;
 - else
 - freq -= cur_step * car_step;
 -
 - /* Setup tuner */
 - if (stv090x_i2c_gate_ctrl(state, 1)  0)
 - goto err;
 + if (lock)
 + return lock;
  
 - if (state-config-tuner_set_frequency) 
 {
 - if 
 (state-config-tuner_set_frequency(fe, freq)  0)
 - goto err_gateoff;
 - }
 + if (state-srate = 1000) {
 + if (stv090x_chk_tmg(state)) {
 + if (STV090x_WRITE_DEMOD(state, DMDISTATE, 0x1f)  0)
 + goto err;
 + if (STV090x_WRITE_DEMOD(state, DMDISTATE, 0x15)  0)
 + goto err;
 + return stv090x_get_dmdlock(state, timeout_dmd);
 + }
 + return 0;
 + }
  
 - if (state-config-tuner_set_bandwidth) 
 {
 - if 
 (state-config-tuner_set_bandwidth(fe, state-tuner_bw)  0)
 - goto err_gateoff;
 - }
 + if (state-srate = 400)
 + car_step 

Re: [PATCH] trivial: adjust code alignment

[walter harms]

Hello Julia,

IMHO keep the patch as it is.
It does not change any code that is good.
Suspicious code that comes up here can be addressed
in a separate patch.

just my 2 cents,
re,
 wh

Am 05.08.2013 18:19, schrieb Julia Lawall:
 On Mon, 5 Aug 2013, Dan Carpenter wrote:
 
 On Mon, Aug 05, 2013 at 04:47:39PM +0200, Julia Lawall wrote:
 diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
 index e8a1ce2..4a5a5dc 100644
 --- a/drivers/media/i2c/ov7670.c
 +++ b/drivers/media/i2c/ov7670.c
 @@ -1369,8 +1369,8 @@ static int ov7670_s_exp(struct v4l2_subdev *sd,
 int value)
  unsigned char com1, com8, aech, aechh;

  ret = ov7670_read(sd, REG_COM1, com1) +
 -ov7670_read(sd, REG_COM8, com8);
 -ov7670_read(sd, REG_AECHH, aechh);
 +ov7670_read(sd, REG_COM8, com8);
 +ov7670_read(sd, REG_AECHH, aechh);
  if (ret)
  return ret;


 The new indenting isn't correct here and anyway the intent was to
 combine all the error codes together and return them as an error
 code jumble.  I'm not a fan of error code jumbles, probably the
 right thing is to check each function call or, barring that, to
 return -EIO.
 
 Oops, thanks for spotting that.  I'm not sure whether it is safe to
 abort these calls as soon as the first one fails, but perhaps I could
 introduce some more variables, and test them all afterwards.
 
 What should I do with the big patch?  Resend it with this cut out?  Or,
 considering that I might have overlooked something else, send 90 some
 little ones?
 
 thanks,
 julia
 -- 
 To unsubscribe from this list: send the line unsubscribe
 kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] bt8xx: info leak in ca_get_slot_info()

[walter harms]

Am 25.07.2013 18:46, schrieb Dan Carpenter:
 p_ca_slot_info was allocated with kmalloc() so we need to clear it
 before passing it to the user.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/media/pci/bt8xx/dst_ca.c 
 b/drivers/media/pci/bt8xx/dst_ca.c
 index 0e788fc..6b9dc3f 100644
 --- a/drivers/media/pci/bt8xx/dst_ca.c
 +++ b/drivers/media/pci/bt8xx/dst_ca.c
 @@ -302,8 +302,11 @@ static int ca_get_slot_info(struct dst_state *state, 
 struct ca_slot_info *p_ca_s
   p_ca_slot_info-flags = CA_CI_MODULE_READY;
   p_ca_slot_info-num = 1;
   p_ca_slot_info-type = CA_CI;
 - } else
 + } else {
   p_ca_slot_info-flags = 0;
 + p_ca_slot_info-num = 0;
 + p_ca_slot_info-type = 0;
 + }
  
   if (copy_to_user(arg, p_ca_slot_info, sizeof (struct ca_slot_info)))
   return -EFAULT;

note: i have no clue how p_ca_slot_info looks like,
but to avoid information leaks via compiler padding etc. i could be more wise
to do a  memset(p_ca_slot_info,0,sizeof (struct ca_slot_info))
and then set the
p_ca_slot_info-flags = CA_CI_MODULE_READY;
p_ca_slot_info-num = 1;
p_ca_slot_info-type = CA_CI;

just my 2 cents,
re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] media: info leak in media_device_enum_entities()

[walter harms]

Am 21.04.2013 13:10, schrieb Dan Carpenter:
 The last part of the u_ent.name buffer isn't cleared so it still has
 uninitialized stack memory.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
 index 99b80b6..1957c0d 100644
 --- a/drivers/media/media-device.c
 +++ b/drivers/media/media-device.c
 @@ -102,9 +102,12 @@ static long media_device_enum_entities(struct 
 media_device *mdev,
   return -EINVAL;
  
   u_ent.id = ent-id;
 - u_ent.name[0] = '\0';
 - if (ent-name)
 - strlcpy(u_ent.name, ent-name, sizeof(u_ent.name));
 + if (ent-name) {
 + strncpy(u_ent.name, ent-name, sizeof(u_ent.name));
 + u_ent.name[sizeof(u_ent.name) - 1] = '\0';
 + } else {
 + memset(u_ent.name, 0, sizeof(u_ent.name));
 + }

I would always memset()
and then do strncpy() for sizeof(u_ent.name) - 1
the rest is always zero.

re,
 wh


   u_ent.type = ent-type;
   u_ent.revision = ent-revision;
   u_ent.flags = ent-flags;
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

[walter harms]

Am 07.10.2012 17:38, schrieb Julia Lawall:
 From: Julia Lawall julia.law...@lip6.fr
 
 Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
 
 In the second i2c_msg structure, a length expressed as an explicit constant
 is also re-expressed as the size of the buffer, reg.
 
 A simplified version of the semantic patch that makes this change is as
 follows: (http://coccinelle.lip6.fr/)
 
 // smpl
 @@
 expression a,b,c;
 identifier x;
 @@
 
 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
 + I2C_MSG_READ(a,b,c)
  ;
 
 @@
 expression a,b,c;
 identifier x;
 @@
 
 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = 0}
 + I2C_MSG_WRITE(a,b,c)
  ;
 
 @@
 expression a,b,c,d;
 identifier x;
 @@
 
 struct i2c_msg x = 
 - {.addr = a, .buf = b, .len = c, .flags = d}
 + I2C_MSG_OP(a,b,c,d)
  ;
 // /smpl
 
 Signed-off-by: Julia Lawall julia.law...@lip6.fr
 
 ---
  drivers/media/tuners/e4000.c |   20 +++-
  1 file changed, 3 insertions(+), 17 deletions(-)
 
 diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
 index 1b33ed3..8f182fc 100644
 --- a/drivers/media/tuners/e4000.c
 +++ b/drivers/media/tuners/e4000.c
 @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, 
 u8 *val, int len)
   int ret;
   u8 buf[1 + len];
   struct i2c_msg msg[1] = {
 - {
 - .addr = priv-cfg-i2c_addr,
 - .flags = 0,
 - .len = sizeof(buf),
 - .buf = buf,
 - }
 + I2C_MSG_WRITE(priv-cfg-i2c_addr, buf, sizeof(buf))
   };
  

Any reason why struct i2c_msg is an array ?

re,
 wh

   buf[0] = reg;
 @@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, 
 u8 *val, int len)
   int ret;
   u8 buf[len];
   struct i2c_msg msg[2] = {
 - {
 - .addr = priv-cfg-i2c_addr,
 - .flags = 0,
 - .len = 1,
 - .buf = reg,
 - }, {
 - .addr = priv-cfg-i2c_addr,
 - .flags = I2C_M_RD,
 - .len = sizeof(buf),
 - .buf = buf,
 - }
 + I2C_MSG_WRITE(priv-cfg-i2c_addr, reg, sizeof(reg)),
 + I2C_MSG_READ(priv-cfg-i2c_addr, buf, sizeof(buf))
   };
  
   ret = i2c_transfer(priv-i2c, msg, 2);
 
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

[walter harms]

Am 07.10.2012 17:38, schrieb Julia Lawall:
 From: Julia Lawall julia.law...@lip6.fr
 
 Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
 
 A length expressed as an explicit constant is also re-expressed as the size
 of the buffer in each case.
 
 A simplified version of the semantic patch that makes this change is as
 follows: (http://coccinelle.lip6.fr/)
 
 // smpl
 @@
 expression a,b,c;
 identifier x;
 @@
 
 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
 + I2C_MSG_READ(a,b,c)
  ;
 
 @@
 expression a,b,c;
 identifier x;
 @@
 
 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = 0}
 + I2C_MSG_WRITE(a,b,c)
  ;
 
 @@
 expression a,b,c,d;
 identifier x;
 @@
 
 struct i2c_msg x = 
 - {.addr = a, .buf = b, .len = c, .flags = d}
 + I2C_MSG_OP(a,b,c,d)
  ;
 // /smpl
 
 Signed-off-by: Julia Lawall julia.law...@lip6.fr
 
 ---
  drivers/media/tuners/fc0011.c |9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)
 
 diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
 index e488254..5dbba98 100644
 --- a/drivers/media/tuners/fc0011.c
 +++ b/drivers/media/tuners/fc0011.c
 @@ -80,8 +80,7 @@ struct fc0011_priv {
  static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
  {
   u8 buf[2] = { reg, val };
 - struct i2c_msg msg = { .addr = priv-addr,
 - .flags = 0, .buf = buf, .len = 2 };
 + struct i2c_msg msg = I2C_MSG_WRITE(priv-addr, buf, sizeof(buf));
  
   if (i2c_transfer(priv-i2c, msg, 1) != 1) {
   dev_err(priv-i2c-dev,
 @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 
 reg, u8 *val)
  {
   u8 dummy;
   struct i2c_msg msg[2] = {
 - { .addr = priv-addr,
 -   .flags = 0, .buf = reg, .len = 1 },
 - { .addr = priv-addr,
 -   .flags = I2C_M_RD, .buf = val ? : dummy, .len = 1 },
 + I2C_MSG_WRITE(priv-addr, reg, sizeof(reg)),
 + I2C_MSG_READ(priv-addr, val ? : dummy, sizeof(dummy)),
   };
  

This dummy looks strange, can it be that this is used uninitialised ?

re,
 wh


   if (i2c_transfer(priv-i2c, msg, 2) != 2) {
 
 --
 To unsubscribe from this list: send the line unsubscribe kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

[walter harms]

Am 07.10.2012 18:50, schrieb Julia Lawall:
 On Sun, 7 Oct 2012, walter harms wrote:
 


 Am 07.10.2012 17:38, schrieb Julia Lawall:
 From: Julia Lawall julia.law...@lip6.fr

 Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

 A length expressed as an explicit constant is also re-expressed as
 the size
 of the buffer in each case.

 A simplified version of the semantic patch that makes this change is as
 follows: (http://coccinelle.lip6.fr/)

 // smpl
 @@
 expression a,b,c;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
 + I2C_MSG_READ(a,b,c)
  ;

 @@
 expression a,b,c;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = 0}
 + I2C_MSG_WRITE(a,b,c)
  ;

 @@
 expression a,b,c,d;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = d}
 + I2C_MSG_OP(a,b,c,d)
  ;
 // /smpl

 Signed-off-by: Julia Lawall julia.law...@lip6.fr

 ---
  drivers/media/tuners/fc0011.c |9 +++--
  1 file changed, 3 insertions(+), 6 deletions(-)

 diff --git a/drivers/media/tuners/fc0011.c
 b/drivers/media/tuners/fc0011.c
 index e488254..5dbba98 100644
 --- a/drivers/media/tuners/fc0011.c
 +++ b/drivers/media/tuners/fc0011.c
 @@ -80,8 +80,7 @@ struct fc0011_priv {
  static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
  {
  u8 buf[2] = { reg, val };
 -struct i2c_msg msg = { .addr = priv-addr,
 -.flags = 0, .buf = buf, .len = 2 };
 +struct i2c_msg msg = I2C_MSG_WRITE(priv-addr, buf, sizeof(buf));

  if (i2c_transfer(priv-i2c, msg, 1) != 1) {
  dev_err(priv-i2c-dev,
 @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv
 *priv, u8 reg, u8 *val)
  {
  u8 dummy;
  struct i2c_msg msg[2] = {
 -{ .addr = priv-addr,
 -  .flags = 0, .buf = reg, .len = 1 },
 -{ .addr = priv-addr,
 -  .flags = I2C_M_RD, .buf = val ? : dummy, .len = 1 },
 +I2C_MSG_WRITE(priv-addr, reg, sizeof(reg)),
 +I2C_MSG_READ(priv-addr, val ? : dummy, sizeof(dummy)),
  };


 This dummy looks strange, can it be that this is used uninitialised ?
 
 I'm not sure to understand the question.  The read, when it happens in
 i2c_transfer will initialize dummy.  On the other hand, I don't know
 what i2c_transfer does when the buffer is NULL and the size is 1.  It
 does not look very elegant at least.
 

mea culpa, i mixed read and write

re,
 wh

 julia
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

[walter harms]

Am 07.10.2012 18:44, schrieb Julia Lawall:
 On Sun, 7 Oct 2012, walter harms wrote:
 


 Am 07.10.2012 17:38, schrieb Julia Lawall:
 From: Julia Lawall julia.law...@lip6.fr

 Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

 In the second i2c_msg structure, a length expressed as an explicit
 constant
 is also re-expressed as the size of the buffer, reg.

 A simplified version of the semantic patch that makes this change is as
 follows: (http://coccinelle.lip6.fr/)

 // smpl
 @@
 expression a,b,c;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
 + I2C_MSG_READ(a,b,c)
  ;

 @@
 expression a,b,c;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = 0}
 + I2C_MSG_WRITE(a,b,c)
  ;

 @@
 expression a,b,c,d;
 identifier x;
 @@

 struct i2c_msg x =
 - {.addr = a, .buf = b, .len = c, .flags = d}
 + I2C_MSG_OP(a,b,c,d)
  ;
 // /smpl

 Signed-off-by: Julia Lawall julia.law...@lip6.fr

 ---
  drivers/media/tuners/e4000.c |   20 +++-
  1 file changed, 3 insertions(+), 17 deletions(-)

 diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
 index 1b33ed3..8f182fc 100644
 --- a/drivers/media/tuners/e4000.c
 +++ b/drivers/media/tuners/e4000.c
 @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv,
 u8 reg, u8 *val, int len)
  int ret;
  u8 buf[1 + len];
  struct i2c_msg msg[1] = {
 -{
 -.addr = priv-cfg-i2c_addr,
 -.flags = 0,
 -.len = sizeof(buf),
 -.buf = buf,
 -}
 +I2C_MSG_WRITE(priv-cfg-i2c_addr, buf, sizeof(buf))
  };


 Any reason why struct i2c_msg is an array ?
 
 I assumed that it looked more harmonious with the other uses of
 i2c_transfer, which takes as arguments an array and the number of elements.
 
 But there are some files that instead use i2c_transfer(priv-i2c, msg, 1).
 I can change them all to do that if that is preferred.  But maybe I will
 wait a little bit to see if there are other issues to address at the
 same time.
 
 thanks,
 julia
 

Hi Julia,
please be aware i am not the maintainer only a distant watcher :)

do you really thing that a macro is appropriate here ? I feel uneasy about it
but i can not offer an other solution.

nothing to worry about,
just my 2 cents.

re,
 wh



 re,
 wh

  buf[0] = reg;
 @@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv,
 u8 reg, u8 *val, int len)
  int ret;
  u8 buf[len];
  struct i2c_msg msg[2] = {
 -{
 -.addr = priv-cfg-i2c_addr,
 -.flags = 0,
 -.len = 1,
 -.buf = reg,
 -}, {
 -.addr = priv-cfg-i2c_addr,
 -.flags = I2C_M_RD,
 -.len = sizeof(buf),
 -.buf = buf,
 -}
 +I2C_MSG_WRITE(priv-cfg-i2c_addr, reg, sizeof(reg)),
 +I2C_MSG_READ(priv-cfg-i2c_addr, buf, sizeof(buf))
  };

  ret = i2c_transfer(priv-i2c, msg, 2);

 -- 
 To unsubscribe from this list: send the line unsubscribe
 kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


 -- 
 To unsubscribe from this list: send the line unsubscribe
 kernel-janitors in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html

 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] cx25821: testing the wrong variable

[walter harms]

Am 29.09.2012 09:12, schrieb Dan Carpenter:
 -input_filename could be NULL here.  The intent was to test
 -_filename.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 I'm not totally convinced that using /root/vid411.yuv is the right idea.
 
 diff --git a/drivers/media/pci/cx25821/cx25821-video-upstream.c 
 b/drivers/media/pci/cx25821/cx25821-video-upstream.c
 index 52c13e0..6759fff 100644
 --- a/drivers/media/pci/cx25821/cx25821-video-upstream.c
 +++ b/drivers/media/pci/cx25821/cx25821-video-upstream.c
 @@ -808,7 +808,7 @@ int cx25821_vidupstream_init_ch1(struct cx25821_dev *dev, 
 int channel_select,
   }
  
   /* Default if filename is empty string */
 - if (strcmp(dev-input_filename, ) == 0) {
 + if (strcmp(dev-_filename, ) == 0) {
   if (dev-_isNTSC) {
   dev-_filename =
   (dev-_pixel_format == PIXEL_FRMT_411) ?
 diff --git a/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c 
 b/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 index c8c94fb..d33fc1a 100644
 --- a/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 +++ b/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 @@ -761,7 +761,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, 
 int channel_select,
   }
  
   /* Default if filename is empty string */
 - if (strcmp(dev-input_filename_ch2, ) == 0) {
 + if (strcmp(dev-_filename_ch2, ) == 0) {
   if (dev-_isNTSC_ch2) {
   dev-_filename_ch2 = (dev-_pixel_format_ch2 ==
   PIXEL_FRMT_411) ? /root/vid411.yuv :


In this case stcmp seems a bit of a overkill. A simple
*(dev-_filename_ch2) == 0
should be ok ?

re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c: fix error return code

[walter harms]

looks ok to me,
note: i do not have the hardware
Reviewed-by: walter harms wha...@bfs.de

Am 08.09.2012 16:01, schrieb Peter Senna Tschudin:
 From: Peter Senna Tschudin peter.se...@gmail.com
 
 Convert a nonnegative error return code to a negative one, as returned
 elsewhere in the function.
 
 A simplified version of the semantic match that finds this problem is as
 follows: (http://coccinelle.lip6.fr/)
 
 // smpl
 (
 if@p1 (\(ret  0\|ret != 0\))
  { ... return ret; }
 |
 ret@p1 = 0
 )
 ... when != ret = e1
 when != ret
 *if(...)
 {
   ... when != ret = e2
   when forall
  return ret;
 }
 
 // /smpl
 
 Signed-off-by: Peter Senna Tschudin peter.se...@gmail.com
 
 ---
 walter harms wha...@bfs.de, thanks for the tip. Please take a look 
 carefully to check if I got your suggestion correctly. It was tested by 
 compilation only.
 
  drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c |   30 
 ++---
  1 file changed, 12 insertions(+), 18 deletions(-)
 
 diff --git a/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c 
 b/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 index c8c94fb..b663dac 100644
 --- a/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 +++ b/drivers/media/pci/cx25821/cx25821-video-upstream-ch2.c
 @@ -704,11 +704,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev 
 *dev, int channel_select,
  {
   struct sram_channel *sram_ch;
   u32 tmp;
 - int retval = 0;
   int err = 0;
   int data_frame_size = 0;
   int risc_buffer_size = 0;
 - int str_length = 0;
  
   if (dev-_is_running_ch2) {
   pr_info(Video Channel is still running so return!\n);
 @@ -744,20 +742,16 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev 
 *dev, int channel_select,
   risc_buffer_size = dev-_isNTSC_ch2 ?
   NTSC_RISC_BUF_SIZE : PAL_RISC_BUF_SIZE;
  
 - if (dev-input_filename_ch2) {
 - str_length = strlen(dev-input_filename_ch2);
 - dev-_filename_ch2 = kmemdup(dev-input_filename_ch2,
 -  str_length + 1, GFP_KERNEL);
 -
 - if (!dev-_filename_ch2)
 - goto error;
 - } else {
 - str_length = strlen(dev-_defaultname_ch2);
 - dev-_filename_ch2 = kmemdup(dev-_defaultname_ch2,
 -  str_length + 1, GFP_KERNEL);
 + if (dev-input_filename_ch2)
 + dev-_filename_ch2 = kstrdup(dev-input_filename_ch2,
 + GFP_KERNEL);
 + else
 + dev-_filename_ch2 = kstrdup(dev-_defaultname_ch2,
 + GFP_KERNEL);
  
 - if (!dev-_filename_ch2)
 - goto error;
 + if (!dev-_filename_ch2) {
 + err = -ENOENT;
 + goto error;
   }
  
   /* Default if filename is empty string */
 @@ -773,7 +767,7 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, 
 int channel_select,
   }
   }
  
 - retval = cx25821_sram_channel_setup_upstream(dev, sram_ch,
 + err = cx25821_sram_channel_setup_upstream(dev, sram_ch,
   dev-_line_size_ch2, 0);
  
   /* setup fifo + format */
 @@ -783,9 +777,9 @@ int cx25821_vidupstream_init_ch2(struct cx25821_dev *dev, 
 int channel_select,
   dev-upstream_databuf_size_ch2 = data_frame_size * 2;
  
   /* Allocating buffers and prepare RISC program */
 - retval = cx25821_upstream_buffer_prepare_ch2(dev, sram_ch,
 + err = cx25821_upstream_buffer_prepare_ch2(dev, sram_ch,
   dev-_line_size_ch2);
 - if (retval  0) {
 + if (err  0) {
   pr_err(%s: Failed to set up Video upstream buffers!\n,
  dev-name);
   goto error;
 
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch v2] [media] rc-core: prevent divide by zero bug in s_tx_carrier()

[walter harms]

Hi all,
I am not sure if that is a good idea.
it should be in the hands of the driver who to use these 'val'
some driver may need a higher value like this one:

static int iguanair_set_tx_carrier(struct rc_dev *dev, uint32_t carrier)
{
struct iguanair *ir = dev-priv;

if (carrier  25000 || carrier  15)
return -EINVAL;

There are also examples where 0 has a special meaning (to be fair not
with this function). Example:
  cfsetospeed() ...  The zero baud rate, B0, is used to terminate the 
connection.

I have no clue who will use the 0 but ...

just my 2 cents,
re,
 wh

Am 09.09.2012 22:31, schrieb Dan Carpenter:
 Several of the drivers use carrier as a divisor in their s_tx_carrier()
 functions.  We should do a sanity check here like we do for
 LIRC_SET_REC_CARRIER.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 ---
 v2: Ben Hutchings pointed out that my first patch was not a complete
 fix.
 
 diff --git a/drivers/media/rc/ir-lirc-codec.c 
 b/drivers/media/rc/ir-lirc-codec.c
 index 6ad4a07..28dc0f0 100644
 --- a/drivers/media/rc/ir-lirc-codec.c
 +++ b/drivers/media/rc/ir-lirc-codec.c
 @@ -211,6 +211,9 @@ static long ir_lirc_ioctl(struct file *filep, unsigned 
 int cmd,
   if (!dev-s_tx_carrier)
   return -EINVAL;
  
 + if (val = 0)
 + return -EINVAL;
 +
   return dev-s_tx_carrier(dev, val);
  
   case LIRC_SET_SEND_DUTY_CYCLE:




--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] ngene: remove an unneeded condition

[walter harms]

Am 20.04.2012 15:15, schrieb Dan Carpenter:
 stat is always zero here.  The condition used to be needed, but we
 shifted stuff around in 0f0b270f90 [media] ngene: CXD2099AR Common
 Interface driver.
 
 This doesn't change how the code works, it's just a bit tidier.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/media/dvb/ngene/ngene-core.c 
 b/drivers/media/dvb/ngene/ngene-core.c
 index f129a93..3985738 100644
 --- a/drivers/media/dvb/ngene/ngene-core.c
 +++ b/drivers/media/dvb/ngene/ngene-core.c
 @@ -1409,10 +1409,8 @@ static int ngene_start(struct ngene *dev)
   if (stat  0)
   goto fail;
  
 - if (!stat)
 - return stat;
 + return 0;
  
 - /* otherwise error: fall through */
  fail:
   ngwritel(0, NGENE_INT_ENABLE);
   free_irq(dev-pci_dev-irq, dev);

it seems more logical to use the positive exit in this case like:

  if (stat =0)
return 0;

instead of jumping over return 0:

just my 2 cents,

re,
 wh


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] staging: use mutex_lock() in s2250_probe().

[walter harms]

Am 17.03.2012 18:36, schrieb santosh nayak:
 From: Santosh Nayak santoshprasadna...@gmail.com
 
 Use uninterruptable sleep lock  'mutex_lock()'  in place of
 mutex_lock_interruptible() because there is no userspace
 for s2250_probe().
 
 Return -ENOMEM   if kzalloc() fails to allocate and initialize.
 
 Signed-off-by: Santosh Nayak santoshprasadna...@gmail.com
 ---
  drivers/staging/media/go7007/s2250-board.c |   43 +++
  1 files changed, 24 insertions(+), 19 deletions(-)
 
 diff --git a/drivers/staging/media/go7007/s2250-board.c 
 b/drivers/staging/media/go7007/s2250-board.c
 index 014d384..1406a37 100644
 --- a/drivers/staging/media/go7007/s2250-board.c
 +++ b/drivers/staging/media/go7007/s2250-board.c
 @@ -637,27 +637,32 @@ static int s2250_probe(struct i2c_client *client,
   state-audio_input = 0;
   write_reg(client, 0x08, 0x02); /* Line In */
  
 - if (mutex_lock_interruptible(usb-i2c_lock) == 0) {
 - data = kzalloc(16, GFP_KERNEL);
 - if (data != NULL) {
 - int rc;
 - rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
 -data, 16, 1);
 - if (rc  0) {
 - u8 mask;
 - data[0] = 0;
 - mask = 15;
 - data[0] = ~mask;
 - data[1] |= mask;
 - go7007_usb_vendor_request(go, 0x40, 0,
 -   (data[1]8)
 -   + data[1],
 -   data, 16, 0);
 - }
 - kfree(data);
 - }
 + mutex_lock(usb-i2c_lock);
 + data = kzalloc(16, GFP_KERNEL);
 + if (data == NULL) {
 + i2c_unregister_device(audio);
 + kfree(state);
   mutex_unlock(usb-i2c_lock);
 + return -ENOMEM;
 + } else {
 + int rc;
 + rc = go7007_usb_vendor_request(go, 0x41, 0, 0,
 +data, 16, 1);
 + if (rc  0) {
 + u8 mask;
 + data[0] = 0;
 + mask = 15;
 + data[0] = ~mask;
 + data[1] |= mask;
 + go7007_usb_vendor_request(go, 0x40, 0,
 +   (data[1]8)
 +   + data[1],
 +   data, 16, 0);
 + }
 + kfree(data);
   }
 + mutex_unlock(usb-i2c_lock);
 +
  
   v4l2_info(sd, initialized successfully\n);
   return 0;

hi,
You can drop the else
1. there is no 3. way
2. you can save 1 indent level

just one question: the (data[1]8)+ data[1] is intended ? always data[1] ?
(i have no clue, it is the original code, it just feels .. strange )

re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 2/2] [media] ds3000: off by one in ds3000_read_snr()

[walter harms]

Am 19.01.2012 10:33, schrieb Dan Carpenter:
 On Wed, Jan 18, 2012 at 06:06:46PM +0100, walter harms wrote:


 Am 17.01.2012 08:30, schrieb Dan Carpenter:
 This is a static checker patch and I don't have the hardware to test
 this, so please review it carefully.  The dvbs2_snr_tab[] array has 80
 elements so when we cap it at 80, that's off by one.  I would have
 assumed that the test was wrong but in the lines right before we have
 the same test but use snr_reading - 1 as the array offset.  I've done
 the same thing here.

 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com

 diff --git a/drivers/media/dvb/frontends/ds3000.c 
 b/drivers/media/dvb/frontends/ds3000.c
 index af65d01..3f5ae0a 100644
 --- a/drivers/media/dvb/frontends/ds3000.c
 +++ b/drivers/media/dvb/frontends/ds3000.c
 @@ -681,7 +681,7 @@ static int ds3000_read_snr(struct dvb_frontend *fe, u16 
 *snr)
 snr_reading = dvbs2_noise_reading / tmp;
 if (snr_reading  80)
 snr_reading = 80;
 -   *snr = -(dvbs2_snr_tab[snr_reading] / 1000);
 +   *snr = -(dvbs2_snr_tab[snr_reading - 1] / 1000);
 }
 dprintk(%s: raw / cooked = 0x%02x / 0x%04x\n, __func__,
 snr_reading, *snr);

 hi dan,

 perhaps it is more useful to do it in the check above ?
 
 It looks like the check is correct but we need to shift all the
 values by one.  Again, I don't have this hardware, I'm just going by
 the context.
 
I do not have the hardware either so this is pure theoretical.

Access to the data field depends on the value of dvbs2_noise_reading/tmp
even when the data are reasonable like 50/100 snr_reading would become 0
and the index suddenly is -1.

just my 2 cents.

re,
 wh


 thinking about that why not replace the number (80) with ARRAY_SIZE() ?
 
 That would be a cleanup, yes but it could go in a separate patch.
 
 regards,
 dan carpenter
 
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] [media] DVB: dvb_frontend: off by one in dtv_property_dump()

[walter harms]

Am 04.06.2011 15:36, schrieb Mauro Carvalho Chehab:
 Em 26-05-2011 08:16, Andreas Oberritter escreveu:
 Hi Dan,

 On 05/26/2011 10:44 AM, Dan Carpenter wrote:
 If the tvp-cmd == DTV_MAX_COMMAND then we read past the end of the
 array.

 Signed-off-by: Dan Carpenter erro...@gmail.com

 diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c 
 b/drivers/media/dvb/dvb-core/dvb_frontend.c
 index 9827804..607e293 100644
 --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
 +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
 @@ -981,7 +981,7 @@ static void dtv_property_dump(struct dtv_property *tvp)
  {
 int i;
  
 -   if (tvp-cmd = 0 || tvp-cmd  DTV_MAX_COMMAND) {
 +   if (tvp-cmd = 0 || tvp-cmd = DTV_MAX_COMMAND) {
 printk(KERN_WARNING %s: tvp.cmd = 0x%08x undefined\n,
 __func__, tvp-cmd);
 return;

 thanks for spotting this, but this fixes the wrong end. This does not need to
 be applied to kernels older than 2.6.40.

 From 6d8588a4546fd4df717ca61450f99fb9c1b13a5f Mon Sep 17 00:00:00 2001
 From: Andreas Oberritter o...@linuxtv.org
 Date: Thu, 26 May 2011 10:54:14 +
 Subject: [PATCH] DVB: dvb_frontend: fix dtv_property_dump for 
 DTV_DVBT2_PLP_ID

 - Add missing entry to array dtv_cmds.
 - Set array size to DTV_MAX_COMMAND + 1 to avoid future off-by-ones.
 
 Patchwork.kernel.org is not reliable at all. It missed this entire thread.
 
 Andreas patch is the right thing to do.
 
 Thank you both for reporting and fixing this issue. I'm applying the
 patch right now.
 

 Signed-off-by: Andreas Oberritter o...@linuxtv.org
 ---
  drivers/media/dvb/dvb-core/dvb_frontend.c |3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)

 diff --git a/drivers/media/dvb/dvb-core/dvb_frontend.c 
 b/drivers/media/dvb/dvb-core/dvb_frontend.c
 index 9827804..bed7bfe 100644
 --- a/drivers/media/dvb/dvb-core/dvb_frontend.c
 +++ b/drivers/media/dvb/dvb-core/dvb_frontend.c
 @@ -904,7 +904,7 @@ static int dvb_frontend_clear_cache(struct dvb_frontend 
 *fe)
  .buffer = b \
  }
  
 -static struct dtv_cmds_h dtv_cmds[] = {
 +static struct dtv_cmds_h dtv_cmds[DTV_MAX_COMMAND + 1] = {
  _DTV_CMD(DTV_TUNE, 1, 0),
  _DTV_CMD(DTV_CLEAR, 1, 0),
  
 @@ -966,6 +966,7 @@ static struct dtv_cmds_h dtv_cmds[] = {
  _DTV_CMD(DTV_ISDBT_LAYERC_TIME_INTERLEAVING, 0, 0),
  
  _DTV_CMD(DTV_ISDBS_TS_ID, 1, 0),
 +_DTV_CMD(DTV_DVBT2_PLP_ID, 1, 0),
  
  /* Get */
  _DTV_CMD(DTV_DISEQC_SLAVE_REPLY, 0, 1),
 

Do you really want a fixed size array ?
perhaps it is better to leave it struct dtv_cmds_h dtv_cmds[]
and use ARRAY_SIZE(dtv_cmds) instead of DTV_MAX_COMMAND ?

i do not see any use beyond dtv_property_dump().

re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[walter harms]

Dan Carpenter schrieb:
 copy_to_user() returns the number of bytes remaining to be copied which
 isn't the right thing to return here.  The comments say that these 
 functions in dvb_ca_en50221.c should return the number of bytes copied or
 an error return.  I've changed it to return -EFAULT.
 
 Signed-off-by: Dan Carpenter erro...@gmail.com
 
 diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c 
 b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
 index ef259a0..aa7a298 100644
 --- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
 +++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
 @@ -1318,8 +1318,10 @@ static ssize_t dvb_ca_en50221_io_write(struct file 
 *file,
  
   fragbuf[0] = connection_id;
   fragbuf[1] = ((fragpos + fraglen)  count) ? 0x80 : 0x00;
 - if ((status = copy_from_user(fragbuf + 2, buf + fragpos, 
 fraglen)) != 0)
 + if ((status = copy_from_user(fragbuf + 2, buf + fragpos, 
 fraglen)) != 0) {
 + status = -EFAULT;
   goto exit;
 + }
  
   timeout = jiffies + HZ / 2;
   written = 0;
 @@ -1494,8 +1496,10 @@ static ssize_t dvb_ca_en50221_io_read(struct file 
 *file, char __user * buf,
  
   hdr[0] = slot;
   hdr[1] = connection_id;
 - if ((status = copy_to_user(buf, hdr, 2)) != 0)
 + if ((status = copy_to_user(buf, hdr, 2)) != 0) {
 + status = -EFAULT;
   goto exit;
 + }
   status = pktlen;
  
  exit:
 --


Doint to many things at once is bad. IMHO it is more readable to do so:

+status = copy_to_user(buf, hdr, 2);
+if ( status  != 0) {

Maybe the maintainer has different ideas but especialy lines like will gain.

-if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
+status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen):
+if ( status  != 0) {

just my 2 cents,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch -next 1/2] media/az6027: doing dma on the stack

[walter harms]

Dan Carpenter schrieb:
 I changed the dma buffers to use allocated memory instead of stack
 memory.
 
 The reason for this is documented in Documentation/DMA-API-HOWTO.txt
 under the section:  What memory is DMA'able?  That document was only
 added a couple weeks ago and there are still lots of modules which
 haven't been corrected yet.  Btw. Smatch includes a pretty good test to
 find places which use stack memory as a dma buffer.  That's how I found
 these.  (http://smatch.sf.net).
 
 Signed-off-by: Dan Carpenter erro...@gmail.com
 
 diff --git a/drivers/media/dvb/dvb-usb/az6027.c 
 b/drivers/media/dvb/dvb-usb/az6027.c
 index 8934788..baaa301 100644
 --- a/drivers/media/dvb/dvb-usb/az6027.c
 +++ b/drivers/media/dvb/dvb-usb/az6027.c
 @@ -417,11 +417,15 @@ static int az6027_ci_read_attribute_mem(struct 
 dvb_ca_en50221 *ca,
   u16 value;
   u16 index;
   int blen;
 - u8 b[12];
 + u8 *b;
  
   if (slot != 0)
   return -EINVAL;
  
 + b = kmalloc(12, GFP_KERNEL);
 + if (!b)
 + return -ENOMEM;
 +
   mutex_lock(state-ca_mutex);
  
   req = 0xC1;


Hi Dan,
i am not sure if that is the way to go.
iff i understand the code correctly the b[12] seems to overcommit  only
blen bytes (not 12) is needed. There must be a cheaper way to send a few bytes
of space to send a command to a device. Perhaps gregKH has a hint ?

re,
 wh
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html