linux-kernel - Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 21 Mar 2017 10:43:27 +0900
From:   DaeSeok Youn <daeseok.youn@...il.com>
To:     wharms@....de
Cc:     mchehab@...nel.org, Greg KH <gregkh@...uxfoundation.org>,
        Alan Cox <alan@...ux.intel.com>,
        SIMRAN SINGHAL <singhalsimran0@...il.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        linux-media@...r.kernel.org, devel <devel@...verdev.osuosl.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        kernel-janitors <kernel-janitors@...r.kernel.org>
Subject: Re: [PATCH 2/4] staging: atomisp: simplify if statement in atomisp_get_sensor_fps()

2017-03-20 22:11 GMT+09:00 walter harms <wharms@....de>:
>
>
> Am 20.03.2017 13:51, schrieb DaeSeok Youn:
>> 2017-03-20 21:04 GMT+09:00 walter harms <wharms@....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.youn@...il.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, &frame_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, &fi);
>>>> +
>>>> +     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.
yes, it looks better than mine. I will update it and resend it.

Thanks walter,
Regards,
Daeseok Youn.

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