Re: [PATCH v2 05/26] media: s5c73m3-core: fix logic on a timeout condition

[Andrzej Hajda]

On 01.11.2017 22:05, Mauro Carvalho Chehab wrote:
> As warned by smatch:
>   drivers/media/i2c/s5c73m3/s5c73m3-core.c:268 s5c73m3_check_status() 
> error: uninitialized symbol 'status'.
>
> if s5c73m3_check_status() is called too late, time_is_after_jiffies(end)
> will return 0, causing the while to abort before reading status.
>
> The current code will do the wrong thing here, as it will still
> check if status != value. The right fix here is to just change
> the initial state of ret to -ETIMEDOUT. This way, if the
> routine is called too late, it will skip the flawed check
> and return that a timeout happened.

First of all it is rather uncommon situation, scenario in which two
consecutive lines of simple code takes more than 2 seconds is rather rare.
Of course it is possible but in such case proposed solution does not
look like as a proper fix, it reports timeout on the sensor without even
touching it.
I think the right fix would be to re-factor the loop to read the status
first and check timeout later.

Regards
Andrzej

>
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c 
> b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> index cdc4f2392ef9..45345f8b27a5 100644
> --- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> +++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
> @@ -248,7 +248,7 @@ static int s5c73m3_check_status(struct s5c73m3 *state, 
> unsigned int value)
>  {
>   unsigned long start = jiffies;
>   unsigned long end = start + msecs_to_jiffies(2000);
> - int ret = 0;
> + int ret = -ETIMEDOUT;
>   u16 status;
>   int count = 0;
>  

[PATCH v2 05/26] media: s5c73m3-core: fix logic on a timeout condition

[Mauro Carvalho Chehab]

As warned by smatch:
drivers/media/i2c/s5c73m3/s5c73m3-core.c:268 s5c73m3_check_status() 
error: uninitialized symbol 'status'.

if s5c73m3_check_status() is called too late, time_is_after_jiffies(end)
will return 0, causing the while to abort before reading status.

The current code will do the wrong thing here, as it will still
check if status != value. The right fix here is to just change
the initial state of ret to -ETIMEDOUT. This way, if the
routine is called too late, it will skip the flawed check
and return that a timeout happened.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/i2c/s5c73m3/s5c73m3-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/s5c73m3/s5c73m3-core.c 
b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
index cdc4f2392ef9..45345f8b27a5 100644
--- a/drivers/media/i2c/s5c73m3/s5c73m3-core.c
+++ b/drivers/media/i2c/s5c73m3/s5c73m3-core.c
@@ -248,7 +248,7 @@ static int s5c73m3_check_status(struct s5c73m3 *state, 
unsigned int value)
 {
unsigned long start = jiffies;
unsigned long end = start + msecs_to_jiffies(2000);
-   int ret = 0;
+   int ret = -ETIMEDOUT;
u16 status;
int count = 0;
 
-- 
2.13.6