[PATCH] alsa: 6fire: don't use custom hex_to_bin()

[PATCH] alsa: 6fire: don't use custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/usb/6fire/firmware.c |   25 ++++++++++++-------------
 1 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c
index 3ebbdec..3b5f517 100644
--- a/sound/usb/6fire/firmware.c
+++ b/sound/usb/6fire/firmware.c
@@ -17,6 +17,7 @@
 #include <linux/firmware.h>
 #include <linux/module.h>
 #include <linux/bitrev.h>
+#include <linux/kernel.h>
 
 #include "firmware.h"
 #include "chip.h"
@@ -60,21 +61,19 @@ struct ihex_record {
 	unsigned int txt_offset; /* current position in txt_data */
 };
 
-static u8 usb6fire_fw_ihex_nibble(const u8 n)
-{
-	if (n >= '0' && n <= '9')
-		return n - '0';
-	else if (n >= 'A' && n <= 'F')
-		return n - ('A' - 10);
-	else if (n >= 'a' && n <= 'f')
-		return n - ('a' - 10);
-	return 0;
-}
-
 static u8 usb6fire_fw_ihex_hex(const u8 *data, u8 *crc)
 {
-	u8 val = (usb6fire_fw_ihex_nibble(data[0]) << 4) |
-			usb6fire_fw_ihex_nibble(data[1]);
+	u8 val = 0;
+	int hval;
+
+	hval = hex_to_bin(data[0]);
+	if (hval >= 0)
+		val |= (hval << 4);
+
+	hval = hex_to_bin(data[1]);
+	if (hval >= 0)
+		val |= hval;
+
 	*crc += val;
 	return val;
 }
-- 
1.7.6.3

[PATCH] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/namei_vfat.c |   22 ++++++----------------
 1 files changed, 6 insertions(+), 16 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index bb3f29c..09cec4c 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 #include <linux/buffer_head.h>
 #include <linux/namei.h>
+#include <linux/kernel.h>
+
 #include "fat.h"
 
 /*
@@ -505,7 +507,6 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
 	unsigned int ec;
 	int i, k, fill;
@@ -530,21 +531,10 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 						return -EINVAL;
 					ec = 0;
 					for (k = 1; k < 5; k++) {
-						nc = ip[k];
-						ec <<= 4;
-						if (nc >= '0' && nc <= '9') {
-							ec |= nc - '0';
-							continue;
-						}
-						if (nc >= 'a' && nc <= 'f') {
-							ec |= nc - ('a' - 10);
-							continue;
-						}
-						if (nc >= 'A' && nc <= 'F') {
-							ec |= nc - ('A' - 10);
-							continue;
-						}
-						return -EINVAL;
+						int val = hex_to_bin(ip[k]);
+						if (val < 0)
+							return -EINVAL;
+						ec = (ec << 4) | val;
 					}
 					*op++ = ec & 0xFF;
 					*op++ = ec >> 8;
-- 
1.7.6.3

Re: [PATCH] fat: don't use custom hex_to_bin()

[Denys Vlasenko]

On Fri, Sep 23, 2011 at 1:32 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
>  fs/fat/namei_vfat.c |   22 ++++++----------------
>  1 files changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index bb3f29c..09cec4c 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -21,6 +21,8 @@
>  #include <linux/slab.h>
>  #include <linux/buffer_head.h>
>  #include <linux/namei.h>
> +#include <linux/kernel.h>
> +
>  #include "fat.h"
>
>  /*
> @@ -505,7 +507,6 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>             struct nls_table *nls)
>  {
>        const unsigned char *ip;
> -       unsigned char nc;
>        unsigned char *op;
>        unsigned int ec;
>        int i, k, fill;
> @@ -530,21 +531,10 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>                                                return -EINVAL;
>                                        ec = 0;
>                                        for (k = 1; k < 5; k++) {
> -                                               nc = ip[k];
> -                                               ec <<= 4;
> -                                               if (nc >= '0' && nc <= '9') {
> -                                                       ec |= nc - '0';
> -                                                       continue;
> -                                               }
> -                                               if (nc >= 'a' && nc <= 'f') {
> -                                                       ec |= nc - ('a' - 10);
> -                                                       continue;
> -                                               }
> -                                               if (nc >= 'A' && nc <= 'F') {
> -                                                       ec |= nc - ('A' - 10);
> -                                                       continue;
> -                                               }
> -                                               return -EINVAL;
> +                                               int val = hex_to_bin(ip[k]);
> +                                               if (val < 0)
> +                                                       return -EINVAL;
> +                                               ec = (ec << 4) | val;
>                                        }
>                                        *op++ = ec & 0xFF;
>                                        *op++ = ec >> 8;

Function call per byte? That's expensive!
Can't we have a function which converts many sequential bytes?

-- 
vda
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

On Fri, 2011-09-23 at 14:05 +0200, Denys Vlasenko wrote: 
> On Fri, Sep 23, 2011 at 1:32 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> > ---
> >  fs/fat/namei_vfat.c |   22 ++++++----------------
> >  1 files changed, 6 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> > index bb3f29c..09cec4c 100644
> > --- a/fs/fat/namei_vfat.c
> > +++ b/fs/fat/namei_vfat.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/slab.h>
> >  #include <linux/buffer_head.h>
> >  #include <linux/namei.h>
> > +#include <linux/kernel.h>
> > +
> >  #include "fat.h"
> >
> >  /*
> > @@ -505,7 +507,6 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
> >             struct nls_table *nls)
> >  {
> >        const unsigned char *ip;
> > -       unsigned char nc;
> >        unsigned char *op;
> >        unsigned int ec;
> >        int i, k, fill;
> > @@ -530,21 +531,10 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
> >                                                return -EINVAL;
> >                                        ec = 0;
> >                                        for (k = 1; k < 5; k++) {
> > -                                               nc = ip[k];
> > -                                               ec <<= 4;
> > -                                               if (nc >= '0' && nc <= '9') {
> > -                                                       ec |= nc - '0';
> > -                                                       continue;
> > -                                               }
> > -                                               if (nc >= 'a' && nc <= 'f') {
> > -                                                       ec |= nc - ('a' - 10);
> > -                                                       continue;
> > -                                               }
> > -                                               if (nc >= 'A' && nc <= 'F') {
> > -                                                       ec |= nc - ('A' - 10);
> > -                                                       continue;
> > -                                               }
> > -                                               return -EINVAL;
> > +                                               int val = hex_to_bin(ip[k]);
> > +                                               if (val < 0)
> > +                                                       return -EINVAL;
> > +                                               ec = (ec << 4) | val;
> >                                        }
> >                                        *op++ = ec & 0xFF;
> >                                        *op++ = ec >> 8;
> 
> Function call per byte?
4 times in our case per 16 bit.

> That's expensive!
By the way, you can do those optimizations across the kernel. There are
dozens of places where one or few bytes are converted per half-byte.

> Can't we have a function which converts many sequential bytes?
We have one, but it doesn't return any error. We could wait until Mimi
integrates his patch series.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/namei_vfat.c |   30 ++++++++----------------------
 1 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index bb3f29c..cf0fd96 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 #include <linux/buffer_head.h>
 #include <linux/namei.h>
+#include <linux/kernel.h>
+
 #include "fat.h"
 
 /*
@@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
-	unsigned int ec;
-	int i, k, fill;
+	int i, rc, fill;
 	int charlen;
 
 	if (utf8) {
@@ -528,26 +528,12 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 				if (escape && (*ip == ':')) {
 					if (i > len - 5)
 						return -EINVAL;
-					ec = 0;
-					for (k = 1; k < 5; k++) {
-						nc = ip[k];
-						ec <<= 4;
-						if (nc >= '0' && nc <= '9') {
-							ec |= nc - '0';
-							continue;
-						}
-						if (nc >= 'a' && nc <= 'f') {
-							ec |= nc - ('a' - 10);
-							continue;
-						}
-						if (nc >= 'A' && nc <= 'F') {
-							ec |= nc - ('A' - 10);
-							continue;
-						}
+
+					rc = hex2bin(op, ip + 1, 2);
+					if (rc < 0)
 						return -EINVAL;
-					}
-					*op++ = ec & 0xFF;
-					*op++ = ec >> 8;
+
+					op += 2;
 					ip += 5;
 					i += 5;
 				} else {
-- 
1.7.6.3

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

Looks good to me.

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Or if it's needed, I'll apply this to fat tree.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> ---
>  fs/fat/namei_vfat.c |   30 ++++++++----------------------
>  1 files changed, 8 insertions(+), 22 deletions(-)
>
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
> index bb3f29c..cf0fd96 100644
> --- a/fs/fat/namei_vfat.c
> +++ b/fs/fat/namei_vfat.c
> @@ -21,6 +21,8 @@
>  #include <linux/slab.h>
>  #include <linux/buffer_head.h>
>  #include <linux/namei.h>
> +#include <linux/kernel.h>
> +
>  #include "fat.h"
>  
>  /*
> @@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>  	     struct nls_table *nls)
>  {
>  	const unsigned char *ip;
> -	unsigned char nc;
>  	unsigned char *op;
> -	unsigned int ec;
> -	int i, k, fill;
> +	int i, rc, fill;
>  	int charlen;
>  
>  	if (utf8) {
> @@ -528,26 +528,12 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>  				if (escape && (*ip == ':')) {
>  					if (i > len - 5)
>  						return -EINVAL;
> -					ec = 0;
> -					for (k = 1; k < 5; k++) {
> -						nc = ip[k];
> -						ec <<= 4;
> -						if (nc >= '0' && nc <= '9') {
> -							ec |= nc - '0';
> -							continue;
> -						}
> -						if (nc >= 'a' && nc <= 'f') {
> -							ec |= nc - ('a' - 10);
> -							continue;
> -						}
> -						if (nc >= 'A' && nc <= 'F') {
> -							ec |= nc - ('A' - 10);
> -							continue;
> -						}
> +
> +					rc = hex2bin(op, ip + 1, 2);
> +					if (rc < 0)
>  						return -EINVAL;
> -					}
> -					*op++ = ec & 0xFF;
> -					*op++ = ec >> 8;
> +
> +					op += 2;
>  					ip += 5;
>  					i += 5;
>  				} else {

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 4284 bytes --]

On Tue, Sep 27, 2011 at 8:19 PM, OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> Looks good to me.
>
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> Or if it's needed, I'll apply this to fat tree.
I guess so. Thanks!

>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>> ---
>>  fs/fat/namei_vfat.c |   30 ++++++++----------------------
>>  1 files changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
>> index bb3f29c..cf0fd96 100644
>> --- a/fs/fat/namei_vfat.c
>> +++ b/fs/fat/namei_vfat.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/buffer_head.h>
>>  #include <linux/namei.h>
>> +#include <linux/kernel.h>
>> +
>>  #include "fat.h"
>>
>>  /*
>> @@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>>            struct nls_table *nls)
>>  {
>>       const unsigned char *ip;
>> -     unsigned char nc;
>>       unsigned char *op;
>> -     unsigned int ec;
>> -     int i, k, fill;
>> +     int i, rc, fill;
>>       int charlen;
>>
>>       if (utf8) {
>> @@ -528,26 +528,12 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>>                               if (escape && (*ip == ':')) {
>>                                       if (i > len - 5)
>>                                               return -EINVAL;
>> -                                     ec = 0;
>> -                                     for (k = 1; k < 5; k++) {
>> -                                             nc = ip[k];
>> -                                             ec <<= 4;
>> -                                             if (nc >= '0' && nc <= '9') {
>> -                                                     ec |= nc - '0';
>> -                                                     continue;
>> -                                             }
>> -                                             if (nc >= 'a' && nc <= 'f') {
>> -                                                     ec |= nc - ('a' - 10);
>> -                                                     continue;
>> -                                             }
>> -                                             if (nc >= 'A' && nc <= 'F') {
>> -                                                     ec |= nc - ('A' - 10);
>> -                                                     continue;
>> -                                             }
>> +
>> +                                     rc = hex2bin(op, ip + 1, 2);
>> +                                     if (rc < 0)
>>                                               return -EINVAL;
>> -                                     }
>> -                                     *op++ = ec & 0xFF;
>> -                                     *op++ = ec >> 8;
>> +
>> +                                     op += 2;
>>                                       ip += 5;
>>                                       i += 5;
>>                               } else {
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
With Best Regards,
Andy Shevchenko
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 4622 bytes --]

On Tue, Sep 27, 2011 at 8:19 PM, OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>
> Looks good to me.
>
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> Or if it's needed, I'll apply this to fat tree.
Please, hold on. Read my comments below.

>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>> ---
>>  fs/fat/namei_vfat.c |   30 ++++++++----------------------
>>  1 files changed, 8 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
>> index bb3f29c..cf0fd96 100644
>> --- a/fs/fat/namei_vfat.c
>> +++ b/fs/fat/namei_vfat.c
>> @@ -21,6 +21,8 @@
>>  #include <linux/slab.h>
>>  #include <linux/buffer_head.h>
>>  #include <linux/namei.h>
>> +#include <linux/kernel.h>
>> +
>>  #include "fat.h"
>>
>>  /*
>> @@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>>            struct nls_table *nls)
>>  {
>>       const unsigned char *ip;
>> -     unsigned char nc;
>>       unsigned char *op;
>> -     unsigned int ec;
>> -     int i, k, fill;
>> +     int i, rc, fill;
>>       int charlen;
>>
>>       if (utf8) {
>> @@ -528,26 +528,12 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>>                               if (escape && (*ip == ':')) {
>>                                       if (i > len - 5)
>>                                               return -EINVAL;
>> -                                     ec = 0;
>> -                                     for (k = 1; k < 5; k++) {
>> -                                             nc = ip[k];
>> -                                             ec <<= 4;
>> -                                             if (nc >= '0' && nc <= '9') {
>> -                                                     ec |= nc - '0';
>> -                                                     continue;
>> -                                             }
>> -                                             if (nc >= 'a' && nc <= 'f') {
>> -                                                     ec |= nc - ('a' - 10);
>> -                                                     continue;
>> -                                             }
>> -                                             if (nc >= 'A' && nc <= 'F') {
>> -                                                     ec |= nc - ('A' - 10);
>> -                                                     continue;
>> -                                             }
>> +
>> +                                     rc = hex2bin(op, ip + 1, 2);
>> +                                     if (rc < 0)
>>                                               return -EINVAL;
>> -                                     }
>> -                                     *op++ = ec & 0xFF;
>> -                                     *op++ = ec >> 8;
Actually here we will change endianess.
So, my question is what endianess is right here? If the original code
okay, then patch should be rewritten like this:

     rc = hex2bin(op++, ip + 3, 1);
     if (rc < 0)
          return -EINVAL;

     rc = hex2bin(op++, ip + 1, 1);
     if (rc < 0)
          return -EINVAL;

>> +
>> +                                     op += 2;
>>                                       ip += 5;
>>                                       i += 5;
>>                               } else {
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
With Best Regards,
Andy Shevchenko
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> Actually here we will change endianess.
> So, my question is what endianess is right here? If the original code
> okay, then patch should be rewritten like this:
>
>      rc = hex2bin(op++, ip + 3, 1);
>      if (rc < 0)
>           return -EINVAL;
>
>      rc = hex2bin(op++, ip + 1, 1);
>      if (rc < 0)
>           return -EINVAL;

Original code may work only for little endian. Well, anyway, the output
should be wchar_t (u16) of native endian.

So, I think it should be something like

	u8 uc[2];
	if (hex2bin(uc, ip + 1, 2) < 0)
        	return -EINVAL;
	*(wchar_t *)op = uc[0] << 8 | uc[1];

This should be readable more.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

On Wed, 2011-09-28 at 08:15 +0900, OGAWA Hirofumi wrote: 
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> 
> > Actually here we will change endianess.
> > So, my question is what endianess is right here? If the original code
> > okay, then patch should be rewritten like this:
> >
> >      rc = hex2bin(op++, ip + 3, 1);
> >      if (rc < 0)
> >           return -EINVAL;
> >
> >      rc = hex2bin(op++, ip + 1, 1);
> >      if (rc < 0)
> >           return -EINVAL;
> 
> Original code may work only for little endian. Well, anyway, the output
> should be wchar_t (u16) of native endian.

> So, I think it should be something like
> 
> 	u8 uc[2];
> 	if (hex2bin(uc, ip + 1, 2) < 0)
>         	return -EINVAL;
> 	*(wchar_t *)op = uc[0] << 8 | uc[1];
> This should be readable more.
It might be so, but it's not okay to do such constructions in C

fs/fat/namei_vfat.c:535:1: warning: ISO C90 forbids mixed declarations
and code [-Wdeclaration-after-statement]

So, I will resend patch as I proposed with additional comments.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

>> So, I think it should be something like
>> 
>> 	u8 uc[2];
>> 	if (hex2bin(uc, ip + 1, 2) < 0)
>>         	return -EINVAL;
>> 	*(wchar_t *)op = uc[0] << 8 | uc[1];
>> This should be readable more.
> It might be so, but it's not okay to do such constructions in C
>
> fs/fat/namei_vfat.c:535:1: warning: ISO C90 forbids mixed declarations
> and code [-Wdeclaration-after-statement]
>
> So, I will resend patch as I proposed with additional comments.

I guess you just did copy&paste. "u8 uc[2];" part should be before
statement as C90.

I.e.

				if (escape && (*ip == ':')) {
                                	u8 uc[2];

					if (i > len - 5)
						return -EINVAL;

					if (hex2bin(uc, ip + 1, 2) < 0)
						return -EINVAL;
					*(wchar_t *)op = uc[0] << 8 | uc[1];

					op += 2;
					ip += 5;
					i += 5;
				} else {

This should be work?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

On Thu, 2011-09-29 at 23:43 +0900, OGAWA Hirofumi wrote: 
> I guess you just did copy&paste. "u8 uc[2];" part should be before
> statement as C90.
> 
> I.e.
> 
> 				if (escape && (*ip == ':')) {
>                                 	u8 uc[2];
> 
> 					if (i > len - 5)
> 						return -EINVAL;
> 
> 					if (hex2bin(uc, ip + 1, 2) < 0)
> 						return -EINVAL;
> 					*(wchar_t *)op = uc[0] << 8 | uc[1];
Actually, why do not use 
*(wchar_t *)op++ = ... 
instead of additional op+=2?

> 
> 					op += 2;
> 					ip += 5;
> 					i += 5;
> 				} else {
> 
> This should be work?
Oh, my bad! So much minor mistakes last time. Really need vacation.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCHv2] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> On Thu, 2011-09-29 at 23:43 +0900, OGAWA Hirofumi wrote: 
>> I guess you just did copy&paste. "u8 uc[2];" part should be before
>> statement as C90.
>> 
>> I.e.
>> 
>> 				if (escape && (*ip == ':')) {
>>                                 	u8 uc[2];
>> 
>> 					if (i > len - 5)
>> 						return -EINVAL;
>> 
>> 					if (hex2bin(uc, ip + 1, 2) < 0)
>> 						return -EINVAL;
>> 					*(wchar_t *)op = uc[0] << 8 | uc[1];
> Actually, why do not use 
> *(wchar_t *)op++ = ... 
> instead of additional op+=2?

IMO, the reason would be, complex oneliner is possible to mistake. Well,
anyway, it doesn't work. It actually increments 1 byte as "char *", not
2 bytes as "wchar_t *".

>> 
>> 					op += 2;
>> 					ip += 5;
>> 					i += 5;
>> 				} else {
>> 
>> This should be work?
> Oh, my bad! So much minor mistakes last time. Really need vacation.

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCHv4] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/namei_vfat.c |   32 ++++++++++----------------------
 1 files changed, 10 insertions(+), 22 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index bb3f29c..93f7009 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 #include <linux/buffer_head.h>
 #include <linux/namei.h>
+#include <linux/kernel.h>
+
 #include "fat.h"
 
 /*
@@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
-	unsigned int ec;
-	int i, k, fill;
+	int i, fill;
 	int charlen;
 
 	if (utf8) {
@@ -526,28 +526,16 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 			     *outlen += 1)
 			{
 				if (escape && (*ip == ':')) {
+					u8 uc[2];
+
 					if (i > len - 5)
 						return -EINVAL;
-					ec = 0;
-					for (k = 1; k < 5; k++) {
-						nc = ip[k];
-						ec <<= 4;
-						if (nc >= '0' && nc <= '9') {
-							ec |= nc - '0';
-							continue;
-						}
-						if (nc >= 'a' && nc <= 'f') {
-							ec |= nc - ('a' - 10);
-							continue;
-						}
-						if (nc >= 'A' && nc <= 'F') {
-							ec |= nc - ('A' - 10);
-							continue;
-						}
+
+					if (hex2bin(uc, ip + 1, 2) < 0)
 						return -EINVAL;
-					}
-					*op++ = ec & 0xFF;
-					*op++ = ec >> 8;
+
+					*(wchar_t *)op++ = uc[0] << 8 | uc[1];
+
 					ip += 5;
 					i += 5;
 				} else {
-- 
1.7.6.3

Re: [PATCHv4] fat: don't use custom hex_to_bin()

[Joe Perches]

On Thu, 2011-09-29 at 18:10 +0300, Andy Shevchenko wrote:
> diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
[]
> @@ -526,28 +526,16 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
>  			     *outlen += 1)
>  			{
>  				if (escape && (*ip == ':')) {
> +					u8 uc[2];
> +
>  					if (i > len - 5)
>  						return -EINVAL;
> -					ec = 0;
> -					for (k = 1; k < 5; k++) {
> -						nc = ip[k];
> -						ec <<= 4;
> -						if (nc >= '0' && nc <= '9') {
> -							ec |= nc - '0';
> -							continue;
> -						}
> -						if (nc >= 'a' && nc <= 'f') {
> -							ec |= nc - ('a' - 10);
> -							continue;
> -						}
> -						if (nc >= 'A' && nc <= 'F') {
> -							ec |= nc - ('A' - 10);
> -							continue;
> -						}
> +
> +					if (hex2bin(uc, ip + 1, 2) < 0)
>  						return -EINVAL;
> -					}
> -					*op++ = ec & 0xFF;
> -					*op++ = ec >> 8;
> +
> +					*(wchar_t *)op++ = uc[0] << 8 | uc[1];

perhaps:

	__le16	foo;

	if (hex2bin((u8 *)&foo, ip + 1, 2) < 0)
		return -EINVAL;
	*(u16 *)op = le16_to_cpu(foo);

	op += 2;

Re: [PATCHv4] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Joe Perches <joe@perches.com> writes:

>> +
>> +					if (hex2bin(uc, ip + 1, 2) < 0)
>>  						return -EINVAL;
>> -					}
>> -					*op++ = ec & 0xFF;
>> -					*op++ = ec >> 8;
>> +
>> +					*(wchar_t *)op++ = uc[0] << 8 | uc[1];
>
> perhaps:
>
> 	__le16	foo;
>
> 	if (hex2bin((u8 *)&foo, ip + 1, 2) < 0)
> 		return -EINVAL;
> 	*(u16 *)op = le16_to_cpu(foo);
>
> 	op += 2;

I also thought about it. But I think it is not so good, like you had
mistake.  It should actually be "__be16 foo" and "be16_to_cpu()".

If we used sscanf() instead of hex2bin(), I might agree to use
"le16_to_cpu()" though.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCHv4] fat: don't use custom hex_to_bin()

[Joe Perches]

On Fri, 2011-09-30 at 03:27 +0900, OGAWA Hirofumi wrote:
> Joe Perches <joe@perches.com> writes:
> 
> >> +
> >> +					if (hex2bin(uc, ip + 1, 2) < 0)
> >>  						return -EINVAL;
> >> -					}
> >> -					*op++ = ec & 0xFF;
> >> -					*op++ = ec >> 8;
> >> +
> >> +					*(wchar_t *)op++ = uc[0] << 8 | uc[1];
> >
> > perhaps:
> >
> > 	__le16	foo;
> >
> > 	if (hex2bin((u8 *)&foo, ip + 1, 2) < 0)
> > 		return -EINVAL;
> > 	*(u16 *)op = le16_to_cpu(foo);
> >
> > 	op += 2;
> 
> I also thought about it. But I think it is not so good, like you had
> mistake.  It should actually be "__be16 foo" and "be16_to_cpu()".
> 
> If we used sscanf() instead of hex2bin(), I might agree to use
> "le16_to_cpu()" though.
> 
> Thanks.

Funny.

I guess that's what I get for reading Andy's comment
and coding that readably.

+                                       /* The ip contains 2 bytes in little
+                                        * endian format. We need to get them
+                                        * in native endian. */

Re: [PATCHv4] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Joe Perches <joe@perches.com> writes:

>> > perhaps:
>> >
>> > 	__le16	foo;
>> >
>> > 	if (hex2bin((u8 *)&foo, ip + 1, 2) < 0)
>> > 		return -EINVAL;
>> > 	*(u16 *)op = le16_to_cpu(foo);
>> >
>> > 	op += 2;
>> 
>> I also thought about it. But I think it is not so good, like you had
>> mistake.  It should actually be "__be16 foo" and "be16_to_cpu()".
>> 
>> If we used sscanf() instead of hex2bin(), I might agree to use
>> "le16_to_cpu()" though.
>> 
>> Thanks.
>
> Funny.
>
> I guess that's what I get for reading Andy's comment
> and coding that readably.
>
> +                                       /* The ip contains 2 bytes in little
> +                                        * endian format. We need to get them
> +                                        * in native endian. */

In here, simply comment is wrong.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

[PATCHv3] fat: don't use custom hex_to_bin()

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---
 fs/fat/namei_vfat.c |   37 +++++++++++++++----------------------
 1 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index bb3f29c..7deb604 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -21,6 +21,8 @@
 #include <linux/slab.h>
 #include <linux/buffer_head.h>
 #include <linux/namei.h>
+#include <linux/kernel.h>
+
 #include "fat.h"
 
 /*
@@ -505,10 +507,8 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 	     struct nls_table *nls)
 {
 	const unsigned char *ip;
-	unsigned char nc;
 	unsigned char *op;
-	unsigned int ec;
-	int i, k, fill;
+	int i, rc, fill;
 	int charlen;
 
 	if (utf8) {
@@ -528,26 +528,19 @@ xlate_to_uni(const unsigned char *name, int len, unsigned char *outname,
 				if (escape && (*ip == ':')) {
 					if (i > len - 5)
 						return -EINVAL;
-					ec = 0;
-					for (k = 1; k < 5; k++) {
-						nc = ip[k];
-						ec <<= 4;
-						if (nc >= '0' && nc <= '9') {
-							ec |= nc - '0';
-							continue;
-						}
-						if (nc >= 'a' && nc <= 'f') {
-							ec |= nc - ('a' - 10);
-							continue;
-						}
-						if (nc >= 'A' && nc <= 'F') {
-							ec |= nc - ('A' - 10);
-							continue;
-						}
+
+					/* The ip contains 2 bytes in little
+					 * endian format. We need to get them
+					 * in native endian. */
+
+					rc = hex2bin(op++, ip + 3, 1);
+					if (rc < 0)
+						return -EINVAL;
+
+					rc = hex2bin(op++, ip + 1, 1);
+					if (rc < 0)
 						return -EINVAL;
-					}
-					*op++ = ec & 0xFF;
-					*op++ = ec >> 8;
+
 					ip += 5;
 					i += 5;
 				} else {
-- 
1.7.6.3

Re: [PATCHv3] fat: don't use custom hex_to_bin()

[OGAWA Hirofumi]

Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:

> +					/* The ip contains 2 bytes in little
> +					 * endian format. We need to get them
> +					 * in native endian. */
> +
> +					rc = hex2bin(op++, ip + 3, 1);
> +					if (rc < 0)
> +						return -EINVAL;
> +
> +					rc = hex2bin(op++, ip + 1, 1);
> +					if (rc < 0)
>  						return -EINVAL;
> -					}
> -					*op++ = ec & 0xFF;
> -					*op++ = ec >> 8;
> +

It will not work for big endian arch correctly like original code.

"ip" is made like following,

			if (uni_xlate == 1) {
				*op++ = ':';
				op = pack_hex_byte(op, ec >> 8);
				op = pack_hex_byte(op, ec);
				len -= 5;
			} else {


Maybe, my explanation was not good. Like above, "ip" format is like
":1234" (can say big endian, it's the string though), so, this code made
a unicode char (op) as little endian.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] alsa: 6fire: don't use custom hex_to_bin()

[Takashi Iwai]

At Fri, 23 Sep 2011 14:32:11 +0300,
Andy Shevchenko wrote:
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org

Applied now.  Thanks.


Takashi

> ---
>  sound/usb/6fire/firmware.c |   25 ++++++++++++-------------
>  1 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/usb/6fire/firmware.c b/sound/usb/6fire/firmware.c
> index 3ebbdec..3b5f517 100644
> --- a/sound/usb/6fire/firmware.c
> +++ b/sound/usb/6fire/firmware.c
> @@ -17,6 +17,7 @@
>  #include <linux/firmware.h>
>  #include <linux/module.h>
>  #include <linux/bitrev.h>
> +#include <linux/kernel.h>
>  
>  #include "firmware.h"
>  #include "chip.h"
> @@ -60,21 +61,19 @@ struct ihex_record {
>  	unsigned int txt_offset; /* current position in txt_data */
>  };
>  
> -static u8 usb6fire_fw_ihex_nibble(const u8 n)
> -{
> -	if (n >= '0' && n <= '9')
> -		return n - '0';
> -	else if (n >= 'A' && n <= 'F')
> -		return n - ('A' - 10);
> -	else if (n >= 'a' && n <= 'f')
> -		return n - ('a' - 10);
> -	return 0;
> -}
> -
>  static u8 usb6fire_fw_ihex_hex(const u8 *data, u8 *crc)
>  {
> -	u8 val = (usb6fire_fw_ihex_nibble(data[0]) << 4) |
> -			usb6fire_fw_ihex_nibble(data[1]);
> +	u8 val = 0;
> +	int hval;
> +
> +	hval = hex_to_bin(data[0]);
> +	if (hval >= 0)
> +		val |= (hval << 4);
> +
> +	hval = hex_to_bin(data[1]);
> +	if (hval >= 0)
> +		val |= hval;
> +
>  	*crc += val;
>  	return val;
>  }
> -- 
> 1.7.6.3
>