[PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[YAMANE Toshiaki]

Improved position to increment variable i,
And typo fixes.

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index 1b3e995..095d6f2 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -309,26 +309,26 @@ static void qt_status_change_check(struct tty_struct *tty,
 			case 0x00:
 				if (i > (RxCount - 4)) {
 					dev_dbg(&port->dev,
-						"Illegal escape seuences in received data\n");
+						"Illegal escape sequence in received data\n");
 					break;
 				}
 
-				ProcessLineStatus(qt_port, data[i + 3]);
-
 				i += 3;
+				ProcessLineStatus(qt_port, data[i]);
+
 				flag = 1;
 				break;
 
 			case 0x01:
 				if (i > (RxCount - 4)) {
 					dev_dbg(&port->dev,
-						"Illegal escape seuences in received data\n");
+						"Illegal escape sequence in received data\n");
 					break;
 				}
 
-				ProcessModemStatus(qt_port, data[i + 3]);
-
 				i += 3;
+				ProcessModemStatus(qt_port, data[i]);
+
 				flag = 1;
 				break;
 
-- 
1.7.9.5

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[Greg Kroah-Hartman]

On Thu, Nov 29, 2012 at 01:57:56PM +0900, YAMANE Toshiaki wrote:
> Improved position to increment variable i,
> And typo fixes.
> 
> Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
> ---
>  drivers/staging/serqt_usb2/serqt_usb2.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
> index 1b3e995..095d6f2 100644
> --- a/drivers/staging/serqt_usb2/serqt_usb2.c
> +++ b/drivers/staging/serqt_usb2/serqt_usb2.c
> @@ -309,26 +309,26 @@ static void qt_status_change_check(struct tty_struct *tty,
>  			case 0x00:
>  				if (i > (RxCount - 4)) {
>  					dev_dbg(&port->dev,
> -						"Illegal escape seuences in received data\n");
> +						"Illegal escape sequence in received data\n");

This is a different type of fix from:

>  					break;
>  				}
>  
> -				ProcessLineStatus(qt_port, data[i + 3]);
> -
>  				i += 3;
> +				ProcessLineStatus(qt_port, data[i]);

I think you just changed the logic in this function, didn't you?

> +
>  				flag = 1;
>  				break;
>  
>  			case 0x01:
>  				if (i > (RxCount - 4)) {
>  					dev_dbg(&port->dev,
> -						"Illegal escape seuences in received data\n");
> +						"Illegal escape sequence in received data\n");
>  					break;
>  				}
>  
> -				ProcessModemStatus(qt_port, data[i + 3]);
> -
>  				i += 3;
> +				ProcessModemStatus(qt_port, data[i]);

Same here, what happens to i after this?

Please break into two patches, and verify that you didn't break anything
here.

thanks,

greg k-h

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[YAMANE Toshiaki]

On Fri, Nov 30, 2012 at 11:10 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Nov 29, 2012 at 01:57:56PM +0900, YAMANE Toshiaki wrote:
>> Improved position to increment variable i,
>> And typo fixes.
>>
>> Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
>> ---
>>  drivers/staging/serqt_usb2/serqt_usb2.c |   12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
>> index 1b3e995..095d6f2 100644
>> --- a/drivers/staging/serqt_usb2/serqt_usb2.c
>> +++ b/drivers/staging/serqt_usb2/serqt_usb2.c
>> @@ -309,26 +309,26 @@ static void qt_status_change_check(struct tty_struct *tty,
>>                       case 0x00:
>>                               if (i > (RxCount - 4)) {
>>                                       dev_dbg(&port->dev,
>> -                                             "Illegal escape seuences in received data\n");
>> +                                             "Illegal escape sequence in received data\n");
>
> This is a different type of fix from:
>
>>                                       break;
>>                               }
>>
>> -                             ProcessLineStatus(qt_port, data[i + 3]);
>> -
>>                               i += 3;
>> +                             ProcessLineStatus(qt_port, data[i]);
>
> I think you just changed the logic in this function, didn't you?
>
>> +
>>                               flag = 1;
>>                               break;
>>
>>                       case 0x01:
>>                               if (i > (RxCount - 4)) {
>>                                       dev_dbg(&port->dev,
>> -                                             "Illegal escape seuences in received data\n");
>> +                                             "Illegal escape sequence in received data\n");
>>                                       break;
>>                               }
>>
>> -                             ProcessModemStatus(qt_port, data[i + 3]);
>> -
>>                               i += 3;
>> +                             ProcessModemStatus(qt_port, data[i]);
>
> Same here, what happens to i after this?
>
> Please break into two patches, and verify that you didn't break anything
> here.

Greg-san,

I am sorry for confusion.
I sent the patch twice since following patch was applied (gregkh/staging-next)

commit 9d36976fad3008fcc4209789566f7f3e7763f212
Modify qt_status_change_check() and delete qt_status_change().

-Incorporate comment of Mr.Joe Perches (sent Nov.17)
-I sent yesterday

Please discard the patches.

Thanks,


YAMANE Toshiaki

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[Dan Carpenter]

On Thu, Nov 29, 2012 at 06:10:26PM -0800, Greg Kroah-Hartman wrote:
> On Thu, Nov 29, 2012 at 01:57:56PM +0900, YAMANE Toshiaki wrote:
> >  
> > -				ProcessLineStatus(qt_port, data[i + 3]);
> > -
> >  				i += 3;
> > +				ProcessLineStatus(qt_port, data[i]);
> 
> I think you just changed the logic in this function, didn't you?

This should be ok.  The i += 3 was there in the original, it's just
moved up a line.

regards,
dan carpenter

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[YAMANE Toshiaki]

On Fri, Nov 30, 2012 at 5:25 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Thu, Nov 29, 2012 at 06:10:26PM -0800, Greg Kroah-Hartman wrote:
>> On Thu, Nov 29, 2012 at 01:57:56PM +0900, YAMANE Toshiaki wrote:
>> >
>> > -                           ProcessLineStatus(qt_port, data[i + 3]);
>> > -
>> >                             i += 3;
>> > +                           ProcessLineStatus(qt_port, data[i]);
>>
>> I think you just changed the logic in this function, didn't you?
>
> This should be ok.  The i += 3 was there in the original, it's just
> moved up a line.

Thanks for your kindness reply.

But please discard this patch.
Let me consider this patch again.


Regards,

YAMANE Toshiaki

[PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[YAMANE Toshiaki]

Modify qt_status_change_check() and delete qt_status_change().

Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
---
 drivers/staging/serqt_usb2/serqt_usb2.c |   53 +++++++++++++------------------
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
index f68a855..1b3e995 100644
--- a/drivers/staging/serqt_usb2/serqt_usb2.c
+++ b/drivers/staging/serqt_usb2/serqt_usb2.c
@@ -291,33 +291,6 @@ static void qt_interrupt_callback(struct urb *urb)
 	/* FIXME */
 }
 
-static int qt_status_change(unsigned int limit,
-			    unsigned char *data,
-			    int i,
-			    struct quatech_port *qt_port,
-			    struct usb_serial_port *port)
-{
-	void (*fn)(struct quatech_port *, unsigned char);
-
-	if (0x00 == data[i + 2]) {
-		dev_dbg(&port->dev, "Line status status.\n");
-		fn = ProcessLineStatus;
-	} else {
-		dev_dbg(&port->dev, "Modem status status.\n");
-		fn = ProcessModemStatus;
-	}
-
-	if (i > limit) {
-		dev_dbg(&port->dev,
-			"Illegal escape seuences in received data\n");
-		return 0;
-	}
-
-	(*fn)(qt_port, data[i + 3]);
-
-	return 1;
-}
-
 static void qt_status_change_check(struct tty_struct *tty,
 				   struct urb *urb,
 				   struct quatech_port *qt_port,
@@ -334,11 +307,29 @@ static void qt_status_change_check(struct tty_struct *tty,
 			flag = 0;
 			switch (data[i + 2]) {
 			case 0x00:
+				if (i > (RxCount - 4)) {
+					dev_dbg(&port->dev,
+						"Illegal escape seuences in received data\n");
+					break;
+				}
+
+				ProcessLineStatus(qt_port, data[i + 3]);
+
+				i += 3;
+				flag = 1;
+				break;
+
 			case 0x01:
-				flag = qt_status_change((RxCount - 4), data, i,
-							qt_port, port);
-				if (flag == 1)
-					i += 3;
+				if (i > (RxCount - 4)) {
+					dev_dbg(&port->dev,
+						"Illegal escape seuences in received data\n");
+					break;
+				}
+
+				ProcessModemStatus(qt_port, data[i + 3]);
+
+				i += 3;
+				flag = 1;
 				break;
 
 			case 0xff:
-- 
1.7.9.5

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[Dan Carpenter]

On Sat, Nov 17, 2012 at 05:19:22AM +0900, YAMANE Toshiaki wrote:
> Modify qt_status_change_check() and delete qt_status_change().
> 

Thanks for doing that.  I think it's a lot easier to read that way.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] staging/serqt_usb2: Refactor qt_status_change_check() in serqt_usb2.c

[Joe Perches]

On Sat, 2012-11-17 at 05:19 +0900, YAMANE Toshiaki wrote:
> Modify qt_status_change_check() and delete qt_status_change().
> 
> Signed-off-by: YAMANE Toshiaki <yamanetoshi@gmail.com>
> ---
>  drivers/staging/serqt_usb2/serqt_usb2.c |   53 +++++++++++++------------------
>  1 file changed, 22 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/serqt_usb2/serqt_usb2.c b/drivers/staging/serqt_usb2/serqt_usb2.c
[]
> @@ -334,11 +307,29 @@ static void qt_status_change_check(struct tty_struct *tty,
>  			flag = 0;
>  			switch (data[i + 2]) {
>  			case 0x00:
> +				if (i > (RxCount - 4)) {
> +					dev_dbg(&port->dev,
> +						"Illegal escape seuences in received data\n");

trivia: seuences/sequence

> +					break;
> +				}
> +
> +				ProcessLineStatus(qt_port, data[i + 3]);
> +
> +				i += 3;

you could move the i += 3 before the ProcessLineStatus
and use data[i]

> +				flag = 1;
> +				break;
> +
>  			case 0x01:
> -				flag = qt_status_change((RxCount - 4), data, i,
> -							qt_port, port);
> -				if (flag == 1)
> -					i += 3;
> +				if (i > (RxCount - 4)) {
> +					dev_dbg(&port->dev,
> +						"Illegal escape seuences in received data\n");

typo here too

> +					break;
> +				}
> +
> +				ProcessModemStatus(qt_port, data[i + 3]);
> +
> +				i += 3;

same i += 3

> +				flag = 1;
>  				break;
>  
>  			case 0xff:

What about something like:

			case 0x0:
			case 0x1:
				if (i > (RxCount - 4)) {
					dev_dbg(&port->dev,
						"Illegal escape sequence in received data\n");
					break;
				}

				if (data[i + 2] == 0x0)
					ProcessLineStatus(qt_port, data[i + 3]);
				else
					ProcessModemStatus(qt_port, data[i + 3]);

				i += 3;
				flag = 1;
  				break;