All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] [media] [pvrusb2]: remove dead retry cmd code
@ 2015-01-05 22:38 Haim Daniel
  2015-01-16 10:57 ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Haim Daniel @ 2015-01-05 22:38 UTC (permalink / raw)
  To: linux-media; +Cc: Haim Daniel

In case a command is timed out, current flow sets the retry_flag
and does nothing.

Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
---
 drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
index f7702ae..02028aa 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
@@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
 			    u32 *argp)
 {
 	unsigned int poll_count;
-	unsigned int try_count = 0;
-	int retry_flag;
 	int ret = 0;
 	unsigned int idx;
 	/* These sizes look to be limited by the FX2 firmware implementation */
@@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
 			break;
 		}
 
-		retry_flag = 0;
-		try_count++;
 		ret = 0;
 		wrData[0] = 0;
 		wrData[1] = cmd;
@@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
 			}
 			if (rdData[0] && (poll_count < 1000)) continue;
 			if (!rdData[0]) {
-				retry_flag = !0;
 				pvr2_trace(
 					PVR2_TRACE_ERROR_LEGS,
-					"Encoder timed out waiting for us"
-					"; arranging to retry");
+					"Encoder timed out waiting for us");
 			} else {
 				pvr2_trace(
 					PVR2_TRACE_ERROR_LEGS,
@@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
 			ret = -EBUSY;
 			break;
 		}
-		if (retry_flag) {
-			if (try_count < 20) continue;
-			pvr2_trace(
-				PVR2_TRACE_ERROR_LEGS,
-				"Too many retries...");
-			ret = -EBUSY;
-		}
 		if (ret) {
 			del_timer_sync(&hdw->encoder_run_timer);
 			hdw->state_encoder_ok = 0;
-- 
1.9.3


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-05 22:38 [PATCH] [media] [pvrusb2]: remove dead retry cmd code Haim Daniel
@ 2015-01-16 10:57 ` Hans Verkuil
  2015-01-16 11:29   ` Haim Daniel
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2015-01-16 10:57 UTC (permalink / raw)
  To: Haim Daniel, linux-media; +Cc: Haim Daniel

On 01/05/2015 11:38 PM, Haim Daniel wrote:
> In case a command is timed out, current flow sets the retry_flag
> and does nothing.

Really? That's not how I read the code: it retries up to 20 times before
bailing out.

Perhaps you missed the "if (try_count < 20) continue;" line?

Regards,

	Hans

> 
> Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
> ---
>  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> index f7702ae..02028aa 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>  			    u32 *argp)
>  {
>  	unsigned int poll_count;
> -	unsigned int try_count = 0;
> -	int retry_flag;
>  	int ret = 0;
>  	unsigned int idx;
>  	/* These sizes look to be limited by the FX2 firmware implementation */
> @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>  			break;
>  		}
>  
> -		retry_flag = 0;
> -		try_count++;
>  		ret = 0;
>  		wrData[0] = 0;
>  		wrData[1] = cmd;
> @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
>  			}
>  			if (rdData[0] && (poll_count < 1000)) continue;
>  			if (!rdData[0]) {
> -				retry_flag = !0;
>  				pvr2_trace(
>  					PVR2_TRACE_ERROR_LEGS,
> -					"Encoder timed out waiting for us"
> -					"; arranging to retry");
> +					"Encoder timed out waiting for us");
>  			} else {
>  				pvr2_trace(
>  					PVR2_TRACE_ERROR_LEGS,
> @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>  			ret = -EBUSY;
>  			break;
>  		}
> -		if (retry_flag) {
> -			if (try_count < 20) continue;
> -			pvr2_trace(
> -				PVR2_TRACE_ERROR_LEGS,
> -				"Too many retries...");
> -			ret = -EBUSY;
> -		}
>  		if (ret) {
>  			del_timer_sync(&hdw->encoder_run_timer);
>  			hdw->state_encoder_ok = 0;
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-16 10:57 ` Hans Verkuil
@ 2015-01-16 11:29   ` Haim Daniel
  2015-01-16 11:37     ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Haim Daniel @ 2015-01-16 11:29 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, Haim Daniel

It looks that "if (try_count < 20) continue" jumps to end of the  do ...
while(0) loop and goes out.

--hd.
On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
> On 01/05/2015 11:38 PM, Haim Daniel wrote:
> > In case a command is timed out, current flow sets the retry_flag
> > and does nothing.
> 
> Really? That's not how I read the code: it retries up to 20 times before
> bailing out.
> 
> Perhaps you missed the "if (try_count < 20) continue;" line?
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
> > ---
> >  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
> >  1 file changed, 1 insertion(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > index f7702ae..02028aa 100644
> > --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >  			    u32 *argp)
> >  {
> >  	unsigned int poll_count;
> > -	unsigned int try_count = 0;
> > -	int retry_flag;
> >  	int ret = 0;
> >  	unsigned int idx;
> >  	/* These sizes look to be limited by the FX2 firmware implementation */
> > @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >  			break;
> >  		}
> >  
> > -		retry_flag = 0;
> > -		try_count++;
> >  		ret = 0;
> >  		wrData[0] = 0;
> >  		wrData[1] = cmd;
> > @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
> >  			}
> >  			if (rdData[0] && (poll_count < 1000)) continue;
> >  			if (!rdData[0]) {
> > -				retry_flag = !0;
> >  				pvr2_trace(
> >  					PVR2_TRACE_ERROR_LEGS,
> > -					"Encoder timed out waiting for us"
> > -					"; arranging to retry");
> > +					"Encoder timed out waiting for us");
> >  			} else {
> >  				pvr2_trace(
> >  					PVR2_TRACE_ERROR_LEGS,
> > @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >  			ret = -EBUSY;
> >  			break;
> >  		}
> > -		if (retry_flag) {
> > -			if (try_count < 20) continue;
> > -			pvr2_trace(
> > -				PVR2_TRACE_ERROR_LEGS,
> > -				"Too many retries...");
> > -			ret = -EBUSY;
> > -		}
> >  		if (ret) {
> >  			del_timer_sync(&hdw->encoder_run_timer);
> >  			hdw->state_encoder_ok = 0;
> > 
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-16 11:29   ` Haim Daniel
@ 2015-01-16 11:37     ` Hans Verkuil
  2015-01-16 11:45       ` Haim Daniel
  2015-01-25  0:45       ` Mike Isely
  0 siblings, 2 replies; 7+ messages in thread
From: Hans Verkuil @ 2015-01-16 11:37 UTC (permalink / raw)
  To: Haim Daniel; +Cc: linux-media, isely

On 01/16/2015 12:29 PM, Haim Daniel wrote:
> It looks that "if (try_count < 20) continue" jumps to end of the  do ...
> while(0) loop and goes out.

Ah, you are right. But that is obviously not what was intended, so just removing
it is not a proper 'fix'.

Mike, can you take a look at this?

Regards,

	Hans

> 
> --hd.
> On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
>> On 01/05/2015 11:38 PM, Haim Daniel wrote:
>>> In case a command is timed out, current flow sets the retry_flag
>>> and does nothing.
>>
>> Really? That's not how I read the code: it retries up to 20 times before
>> bailing out.
>>
>> Perhaps you missed the "if (try_count < 20) continue;" line?
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
>>> ---
>>>  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
>>>  1 file changed, 1 insertion(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
>>> index f7702ae..02028aa 100644
>>> --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
>>> +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
>>> @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>>>  			    u32 *argp)
>>>  {
>>>  	unsigned int poll_count;
>>> -	unsigned int try_count = 0;
>>> -	int retry_flag;
>>>  	int ret = 0;
>>>  	unsigned int idx;
>>>  	/* These sizes look to be limited by the FX2 firmware implementation */
>>> @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>>>  			break;
>>>  		}
>>>  
>>> -		retry_flag = 0;
>>> -		try_count++;
>>>  		ret = 0;
>>>  		wrData[0] = 0;
>>>  		wrData[1] = cmd;
>>> @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
>>>  			}
>>>  			if (rdData[0] && (poll_count < 1000)) continue;
>>>  			if (!rdData[0]) {
>>> -				retry_flag = !0;
>>>  				pvr2_trace(
>>>  					PVR2_TRACE_ERROR_LEGS,
>>> -					"Encoder timed out waiting for us"
>>> -					"; arranging to retry");
>>> +					"Encoder timed out waiting for us");
>>>  			} else {
>>>  				pvr2_trace(
>>>  					PVR2_TRACE_ERROR_LEGS,
>>> @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
>>>  			ret = -EBUSY;
>>>  			break;
>>>  		}
>>> -		if (retry_flag) {
>>> -			if (try_count < 20) continue;
>>> -			pvr2_trace(
>>> -				PVR2_TRACE_ERROR_LEGS,
>>> -				"Too many retries...");
>>> -			ret = -EBUSY;
>>> -		}
>>>  		if (ret) {
>>>  			del_timer_sync(&hdw->encoder_run_timer);
>>>  			hdw->state_encoder_ok = 0;
>>>
>>
> 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-16 11:37     ` Hans Verkuil
@ 2015-01-16 11:45       ` Haim Daniel
  2015-01-25  0:45       ` Mike Isely
  1 sibling, 0 replies; 7+ messages in thread
From: Haim Daniel @ 2015-01-16 11:45 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media, isely

Removing 8 years old dead code seemed right to silly me.
On Fri, 2015-01-16 at 12:37 +0100, Hans Verkuil wrote:
> On 01/16/2015 12:29 PM, Haim Daniel wrote:
> > It looks that "if (try_count < 20) continue" jumps to end of the  do ...
> > while(0) loop and goes out.
> 
> Ah, you are right. But that is obviously not what was intended, so just removing
> it is not a proper 'fix'.
> 
> Mike, can you take a look at this?
> 
> Regards,
> 
> 	Hans
> 
> > 
> > --hd.
> > On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
> >> On 01/05/2015 11:38 PM, Haim Daniel wrote:
> >>> In case a command is timed out, current flow sets the retry_flag
> >>> and does nothing.
> >>
> >> Really? That's not how I read the code: it retries up to 20 times before
> >> bailing out.
> >>
> >> Perhaps you missed the "if (try_count < 20) continue;" line?
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>> Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
> >>> ---
> >>>  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
> >>>  1 file changed, 1 insertion(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> index f7702ae..02028aa 100644
> >>> --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			    u32 *argp)
> >>>  {
> >>>  	unsigned int poll_count;
> >>> -	unsigned int try_count = 0;
> >>> -	int retry_flag;
> >>>  	int ret = 0;
> >>>  	unsigned int idx;
> >>>  	/* These sizes look to be limited by the FX2 firmware implementation */
> >>> @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			break;
> >>>  		}
> >>>  
> >>> -		retry_flag = 0;
> >>> -		try_count++;
> >>>  		ret = 0;
> >>>  		wrData[0] = 0;
> >>>  		wrData[1] = cmd;
> >>> @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			}
> >>>  			if (rdData[0] && (poll_count < 1000)) continue;
> >>>  			if (!rdData[0]) {
> >>> -				retry_flag = !0;
> >>>  				pvr2_trace(
> >>>  					PVR2_TRACE_ERROR_LEGS,
> >>> -					"Encoder timed out waiting for us"
> >>> -					"; arranging to retry");
> >>> +					"Encoder timed out waiting for us");
> >>>  			} else {
> >>>  				pvr2_trace(
> >>>  					PVR2_TRACE_ERROR_LEGS,
> >>> @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			ret = -EBUSY;
> >>>  			break;
> >>>  		}
> >>> -		if (retry_flag) {
> >>> -			if (try_count < 20) continue;
> >>> -			pvr2_trace(
> >>> -				PVR2_TRACE_ERROR_LEGS,
> >>> -				"Too many retries...");
> >>> -			ret = -EBUSY;
> >>> -		}
> >>>  		if (ret) {
> >>>  			del_timer_sync(&hdw->encoder_run_timer);
> >>>  			hdw->state_encoder_ok = 0;
> >>>
> >>
> > 
> > 
> 



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-16 11:37     ` Hans Verkuil
  2015-01-16 11:45       ` Haim Daniel
@ 2015-01-25  0:45       ` Mike Isely
  2015-01-25 23:37         ` isely
  1 sibling, 1 reply; 7+ messages in thread
From: Mike Isely @ 2015-01-25  0:45 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Haim Daniel, linux-media


Sorry been asleep at the wheel here.  I'll take a look.

Please realize that the code path being talked about here HAS worked - 
because the encoder does tend to fail and this is how the driver 
recovers.

  -Mike


On Fri, 16 Jan 2015, Hans Verkuil wrote:

> On 01/16/2015 12:29 PM, Haim Daniel wrote:
> > It looks that "if (try_count < 20) continue" jumps to end of the  do ...
> > while(0) loop and goes out.
> 
> Ah, you are right. But that is obviously not what was intended, so just removing
> it is not a proper 'fix'.
> 
> Mike, can you take a look at this?
> 
> Regards,
> 
> 	Hans
> 
> > 
> > --hd.
> > On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
> >> On 01/05/2015 11:38 PM, Haim Daniel wrote:
> >>> In case a command is timed out, current flow sets the retry_flag
> >>> and does nothing.
> >>
> >> Really? That's not how I read the code: it retries up to 20 times before
> >> bailing out.
> >>
> >> Perhaps you missed the "if (try_count < 20) continue;" line?
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>> Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
> >>> ---
> >>>  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
> >>>  1 file changed, 1 insertion(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> index f7702ae..02028aa 100644
> >>> --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> >>> @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			    u32 *argp)
> >>>  {
> >>>  	unsigned int poll_count;
> >>> -	unsigned int try_count = 0;
> >>> -	int retry_flag;
> >>>  	int ret = 0;
> >>>  	unsigned int idx;
> >>>  	/* These sizes look to be limited by the FX2 firmware implementation */
> >>> @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			break;
> >>>  		}
> >>>  
> >>> -		retry_flag = 0;
> >>> -		try_count++;
> >>>  		ret = 0;
> >>>  		wrData[0] = 0;
> >>>  		wrData[1] = cmd;
> >>> @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			}
> >>>  			if (rdData[0] && (poll_count < 1000)) continue;
> >>>  			if (!rdData[0]) {
> >>> -				retry_flag = !0;
> >>>  				pvr2_trace(
> >>>  					PVR2_TRACE_ERROR_LEGS,
> >>> -					"Encoder timed out waiting for us"
> >>> -					"; arranging to retry");
> >>> +					"Encoder timed out waiting for us");
> >>>  			} else {
> >>>  				pvr2_trace(
> >>>  					PVR2_TRACE_ERROR_LEGS,
> >>> @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> >>>  			ret = -EBUSY;
> >>>  			break;
> >>>  		}
> >>> -		if (retry_flag) {
> >>> -			if (try_count < 20) continue;
> >>> -			pvr2_trace(
> >>> -				PVR2_TRACE_ERROR_LEGS,
> >>> -				"Too many retries...");
> >>> -			ret = -EBUSY;
> >>> -		}
> >>>  		if (ret) {
> >>>  			del_timer_sync(&hdw->encoder_run_timer);
> >>>  			hdw->state_encoder_ok = 0;
> >>>
> >>
> > 
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] [media] [pvrusb2]: remove dead retry cmd code
  2015-01-25  0:45       ` Mike Isely
@ 2015-01-25 23:37         ` isely
  0 siblings, 0 replies; 7+ messages in thread
From: isely @ 2015-01-25 23:37 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: Haim Daniel, linux-media


Well this is an interesting one.

It makes an incorrect assumption about the behavior of "do { ... } 
while(0)", that being issuance of a "continue" within the loop would 
force execution to go back to the top of the loop.  But what it really 
does is force a jump to the bottom, where the false condition causes the 
loop to always exit.  Effectively "continue" is the same as "break" 
here.

The intent here is that "continue" would cause the code to restart, i.e. 
go to the top of the block.  It was attempting to co-opt the behavior of 
the outer do-while loop, incorrect.

The reason for the "do { ... } while(0)" idiom was to help avoid 
accidentally jumping out of that block without freeing the lock along 
the way.  I did things like that back in 2005 when this code originated.

This is a bug, not dead code.  Shockingly enough this has gone unnoticed 
since forever, and given that it has gone unnoticed then either the 
retry functionality is not needed or it has been failing to retry when 
it should.

I need to dig into the validity of this test.  I will deal with this.

  -Mike



On Sat, 24 Jan 2015, Mike Isely wrote:

> 
> Sorry been asleep at the wheel here.  I'll take a look.
> 
> Please realize that the code path being talked about here HAS worked - 
> because the encoder does tend to fail and this is how the driver 
> recovers.
> 
>   -Mike
> 
> 
> On Fri, 16 Jan 2015, Hans Verkuil wrote:
> 
> > On 01/16/2015 12:29 PM, Haim Daniel wrote:
> > > It looks that "if (try_count < 20) continue" jumps to end of the  do ...
> > > while(0) loop and goes out.
> > 
> > Ah, you are right. But that is obviously not what was intended, so just removing
> > it is not a proper 'fix'.
> > 
> > Mike, can you take a look at this?
> > 
> > Regards,
> > 
> > 	Hans
> > 
> > > 
> > > --hd.
> > > On Fri, 2015-01-16 at 11:57 +0100, Hans Verkuil wrote:
> > >> On 01/05/2015 11:38 PM, Haim Daniel wrote:
> > >>> In case a command is timed out, current flow sets the retry_flag
> > >>> and does nothing.
> > >>
> > >> Really? That's not how I read the code: it retries up to 20 times before
> > >> bailing out.
> > >>
> > >> Perhaps you missed the "if (try_count < 20) continue;" line?
> > >>
> > >> Regards,
> > >>
> > >> 	Hans
> > >>
> > >>>
> > >>> Signed-off-by: Haim Daniel <haim.daniel@gmail.com>
> > >>> ---
> > >>>  drivers/media/usb/pvrusb2/pvrusb2-encoder.c | 15 +--------------
> > >>>  1 file changed, 1 insertion(+), 14 deletions(-)
> > >>>
> > >>> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > >>> index f7702ae..02028aa 100644
> > >>> --- a/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > >>> +++ b/drivers/media/usb/pvrusb2/pvrusb2-encoder.c
> > >>> @@ -145,8 +145,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> > >>>  			    u32 *argp)
> > >>>  {
> > >>>  	unsigned int poll_count;
> > >>> -	unsigned int try_count = 0;
> > >>> -	int retry_flag;
> > >>>  	int ret = 0;
> > >>>  	unsigned int idx;
> > >>>  	/* These sizes look to be limited by the FX2 firmware implementation */
> > >>> @@ -213,8 +211,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> > >>>  			break;
> > >>>  		}
> > >>>  
> > >>> -		retry_flag = 0;
> > >>> -		try_count++;
> > >>>  		ret = 0;
> > >>>  		wrData[0] = 0;
> > >>>  		wrData[1] = cmd;
> > >>> @@ -245,11 +241,9 @@ static int pvr2_encoder_cmd(void *ctxt,
> > >>>  			}
> > >>>  			if (rdData[0] && (poll_count < 1000)) continue;
> > >>>  			if (!rdData[0]) {
> > >>> -				retry_flag = !0;
> > >>>  				pvr2_trace(
> > >>>  					PVR2_TRACE_ERROR_LEGS,
> > >>> -					"Encoder timed out waiting for us"
> > >>> -					"; arranging to retry");
> > >>> +					"Encoder timed out waiting for us");
> > >>>  			} else {
> > >>>  				pvr2_trace(
> > >>>  					PVR2_TRACE_ERROR_LEGS,
> > >>> @@ -269,13 +263,6 @@ static int pvr2_encoder_cmd(void *ctxt,
> > >>>  			ret = -EBUSY;
> > >>>  			break;
> > >>>  		}
> > >>> -		if (retry_flag) {
> > >>> -			if (try_count < 20) continue;
> > >>> -			pvr2_trace(
> > >>> -				PVR2_TRACE_ERROR_LEGS,
> > >>> -				"Too many retries...");
> > >>> -			ret = -EBUSY;
> > >>> -		}
> > >>>  		if (ret) {
> > >>>  			del_timer_sync(&hdw->encoder_run_timer);
> > >>>  			hdw->state_encoder_ok = 0;
> > >>>
> > >>
> > > 
> > > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-media" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 

-- 

Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-01-25 23:37 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-05 22:38 [PATCH] [media] [pvrusb2]: remove dead retry cmd code Haim Daniel
2015-01-16 10:57 ` Hans Verkuil
2015-01-16 11:29   ` Haim Daniel
2015-01-16 11:37     ` Hans Verkuil
2015-01-16 11:45       ` Haim Daniel
2015-01-25  0:45       ` Mike Isely
2015-01-25 23:37         ` isely

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.