[PATCH 2.6.18-rc4] sisusbvga: fix sisusb.c

I had a problem with usb2vga and ppc linux.
The background of the console greens, and the font is not displayed.

This patch can be normally displayed by the Big endian machine.
Moreover, because KAIREN's usb2vga has two or more devices id, it adds it.

Signed-off-by: Mitsuhiro Koga <redacted>
Cc: linux-usb-devel@lists.sourceforge.net
Cc: linux-fbdev-devel@lists.sourceforge.net
---
 drivers/usb/misc/sisusbvga/sisusb.c      |   27 ++++++++++++++++++++++++++-
 1 files changed, 26 insertions(+), 1 deletions(-)

--- a/drivers/usb/misc/sisusbvga/sisusb.c
+++ b/drivers/usb/misc/sisusbvga/sisusb.c

@@ -76,6 +76,16 @@

 DEFINE_MUTEX(disconnect_mutex);

+#ifdef __BIG_ENDIAN
+static void
+correct_endianness_buffer(u16* data, const int length)
+{
+   int i;
+   for (i = 0; i < length; i++)
+       *(data+i) = cpu_to_le16(*(data+i));
+}
+#endif
+
 static void
 sisusb_free_buffers(struct sisusb_usb_data *sisusb)
 {

@@ -411,6 +421,10 @@
            memcpy(buffer, kernbuffer, passsize);
            kernbuffer += passsize;

+#ifdef __BIG_ENDIAN
+           if ((len & 3) == 0)
+               correct_endianness_buffer((u16 *)buffer, thispass/2);
+#endif
        }

        retry = 5;

@@ -906,9 +920,13 @@
            if (userbuffer) {
                if (get_user(swap32, (u32 __user *)userbuffer))
                    return -EFAULT;
-           } else
+           } else {
                swap32 = *((u32 *)kernbuffer);

+#ifdef __BIG_ENDIAN
+               swap32 = ((swap32 << 16) & 0xffff0000) | ((swap32 >>

16) & 0xffff);
+#endif
+           }
            ret = sisusb_write_memio_long(sisusb,
                            SISUSB_TYPE_MEM,
                            addr,

@@ -1259,7 +1277,11 @@

                    userbuffer += 4;
                } else {
+#ifdef __BIG_ENDIAN
+                   *((u32 *)kernbuffer) = ((swap32 << 16) &

0xffff0000) | ((swap32 >> 16) & 0xffff);
+#else
                    *((u32 *)kernbuffer) = swap32;
+#endif
                    kernbuffer += 4;
                }
                addr += 4;

@@ -3435,6 +3457,9 @@

 static struct usb_device_id sisusb_table [] = {
    { USB_DEVICE(0x0711, 0x0900) },
+   { USB_DEVICE(0x0711, 0x0901) },
+   { USB_DEVICE(0x0711, 0x0902) },
+   { USB_DEVICE(0x0711, 0x0920) },
    { USB_DEVICE(0x182d, 0x021c) },
    { USB_DEVICE(0x182d, 0x0269) },
    { }

-------------------------------------------------------------------------

Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

On Mon, 2006-08-14 at 05:47 +0900, Mitsuhiro KOGA wrote:

Added the author to the CC list.

It would be better if you remove the #ifdef within the functions.
So something like...

#ifdef __LITTLE_ENDIAN
static inline void sisusb_memcpy(void *d, void *s, int count)
{
	memcpy(d, s, count);
}
#else
static inline void sisusb_memcpy(void *d, void *s, int count)
{
	memcpy_and_reverse(d, s, count);
}
#endif

And for the above 2, something like...

#ifdef __BIG_ENDIAN
static inline void sisusb_reverse(u32 *s, u32 *d)
{
	*d = ((*s << 16) & 0xffff0000) | ((*s >> 16) & 0xffff);
}
#else
static inline void sisiusb_reverse(u32 *s, u32 *d)
{
	*d = *s;
}
#endif

Tony

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

Hi,

It would be better if you remove the #ifdef within the functions.
So something like...

<snip>

#ifdef __LITTLE_ENDIAN
static inline void sisusb_memcpy(void *d, void *s, int count)
{
        memcpy(d, s, count);
}
#else
static inline void sisusb_memcpy(void *d, void *s, int count)
{
        memcpy_and_reverse(d, s, count);
}
#endif

<snip>

And for the above 2, something like...

#ifdef __BIG_ENDIAN
static inline void sisusb_reverse(u32 *s, u32 *d)
{
        *d = ((*s << 16) & 0xffff0000) | ((*s >> 16) & 0xffff);
}
#else
static inline void sisiusb_reverse(u32 *s, u32 *d)
{
        *d = *s;
}
#endif

Thanks.
I re-fixed the patch according to your advice.
And, because it's used #ifdef in other part, I also fixed it.
Would you please check it?

Signed-off-by: Mitsuhiro Koga <redacted>
---
diff -urp a/drivers/usb/misc/sisusbvga/sisusb.c
b/drivers/usb/misc/sisusbvga/sisusb.c

--- a/drivers/usb/misc/sisusbvga/sisusb.c	2006-08-07 03:20:11.000000000 +0900
+++ b/drivers/usb/misc/sisusbvga/sisusb.c	2006-08-16 15:53:59.000000000 +0900

@@ -76,6 +76,63 @@ static struct usb_driver sisusb_driver;

 DEFINE_MUTEX(disconnect_mutex);

+#ifdef __BIG_ENDIAN
+static inline void sisusb_memcpy(u16 *dst, u16 *src, int count)
+{
+	while (count--)
+		*(dst++) = cpu_to_le16(*(src++));
+}
+#else
+static inline void sisusb_memcpy(u16 *dst, u16 *src, int count)
+{
+	memcpy(dst, src, count);
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_wmem_24bit(char *src, u32 *dst)
+{
+	*dst = (*src        << 16) |
+		(*(src + 1) <<  8) |
+		*(src + 2);
+}
+#else
+static inline void sisusb_order_wmem_24bit(char *src, u32 *dst)
+{
+	*dst = (*(src + 2)  << 16) |
+		(*(src + 1) <<  8) |
+		*src;
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_rmem_24bit(u32 *src, char *dst)
+{
+	*dst       = (*src >> 16) & 0xff;
+	*(dst + 1) = (*src >> 8)  & 0xff;
+	*(dst + 2) = *src         & 0xff;
+}
+#else
+static inline void sisusb_order_rmem_24bit(u32 *src, char *dst)
+{
+	*(dst + 2) = (*src >> 16) & 0xff;
+	*(dst + 1) = (*src >> 8)  & 0xff;
+	*dst       = *src         & 0xff;
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_mem_32bit(u32 *src, u32 *dst)
+{
+	*dst = ((*src << 16) & 0xffff0000) | ((*src >> 16) & 0xffff);
+}
+#else
+static inline void sisusb_order_mem_32bit(u32 *src, u32 *dst)
+{
+	*dst = *src;
+}
+#endif
+
 static void
 sisusb_free_buffers(struct sisusb_usb_data *sisusb)
 {

@@ -408,7 +465,11 @@ static int sisusb_send_bulk_msg(struct s

 		} else if (fromkern) {

-			memcpy(buffer, kernbuffer, passsize);
+			if ((len & 3) == 0)
+				sisusb_memcpy((u16 *)buffer, (u16 *)kernbuffer, passsize / 2);
+			else
+				memcpy(buffer, kernbuffer, passsize);
+
 			kernbuffer += passsize;

 		}

@@ -872,25 +933,9 @@ static int sisusb_write_mem_bulk(struct
 			if (userbuffer) {
 				if (copy_from_user(&buf, userbuffer, 3))
 					return -EFAULT;
-#ifdef __BIG_ENDIAN
-				swap32 = (buf[0] << 16) |
-					 (buf[1] <<  8) |
-					 buf[2];
-#else
-				swap32 = (buf[2] << 16) |
-					 (buf[1] <<  8) |
-					 buf[0];
-#endif
+				sisusb_order_wmem_24bit(&buf, &swap32);
 			} else
-#ifdef __BIG_ENDIAN
-				swap32 = (kernbuffer[0] << 16) |
-					 (kernbuffer[1] <<  8) |
-					 kernbuffer[2];
-#else
-				swap32 = (kernbuffer[2] << 16) |
-					 (kernbuffer[1] <<  8) |
-					 kernbuffer[0];
-#endif
+				sisusb_order_wmem_24bit(kernbuffer, &swap32);

 			ret = sisusb_write_memio_24bit(sisusb,
 							SISUSB_TYPE_MEM,

@@ -907,7 +952,7 @@ static int sisusb_write_mem_bulk(struct
 				if (get_user(swap32, (u32 __user *)userbuffer))
 					return -EFAULT;
 			} else
-				swap32 = *((u32 *)kernbuffer);
+				sisusb_order_mem_32bit((u32 *)kernbuffer, &swap32);

 			ret = sisusb_write_memio_long(sisusb,
 							SISUSB_TYPE_MEM,

@@ -1227,15 +1272,7 @@ static int sisusb_read_mem_bulk(struct s
 								addr, &swap32);
 			if (!ret) {
 				(*bytes_read) += 3;
-#ifdef __BIG_ENDIAN
-				buf[0] = (swap32 >> 16) & 0xff;
-				buf[1] = (swap32 >> 8) & 0xff;
-				buf[2] = swap32 & 0xff;
-#else
-				buf[2] = (swap32 >> 16) & 0xff;
-				buf[1] = (swap32 >> 8) & 0xff;
-				buf[0] = swap32 & 0xff;
-#endif
+				sisusb_order_rmem_24bit(&swap32, &buf[0]);
 				if (userbuffer) {
 					if (copy_to_user(userbuffer, &buf[0], 3))
 						return -EFAULT;

@@ -1259,7 +1296,7 @@ static int sisusb_read_mem_bulk(struct s

 					userbuffer += 4;
 				} else {
-					*((u32 *)kernbuffer) = swap32;
+					sisusb_order_mem_32bit((u32 *)kernbuffer, &swap32);
 					kernbuffer += 4;
 				}
 				addr += 4;

@@ -3435,6 +3472,9 @@ static void sisusb_disconnect(struct usb

 static struct usb_device_id sisusb_table [] = {
 	{ USB_DEVICE(0x0711, 0x0900) },
+	{ USB_DEVICE(0x0711, 0x0901) },
+	{ USB_DEVICE(0x0711, 0x0902) },
+	{ USB_DEVICE(0x0711, 0x0920) },
 	{ USB_DEVICE(0x182d, 0x021c) },
 	{ USB_DEVICE(0x182d, 0x0269) },
 	{ }

-------------------------------------------------------------------------

Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

On Wed, 16 Aug 2006, Mitsuhiro KOGA wrote:

Why not merge the #ifdefs, so you need only one?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

2006/8/16, Geert Uytterhoeven [off-list ref]:

On Wed, 16 Aug 2006, Mitsuhiro KOGA wrote:

quoted

@@ -76,6 +76,63 @@ static struct usb_driver sisusb_driver;

 DEFINE_MUTEX(disconnect_mutex);

+#ifdef __BIG_ENDIAN
+static inline void sisusb_memcpy(u16 *dst, u16 *src, int count)
+{
+     while (count--)
+             *(dst++) = cpu_to_le16(*(src++));
+}
+#else
+static inline void sisusb_memcpy(u16 *dst, u16 *src, int count)
+{
+     memcpy(dst, src, count);
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_wmem_24bit(char *src, u32 *dst)
+{
+     *dst = (*src        << 16) |
+             (*(src + 1) <<  8) |
+             *(src + 2);
+}
+#else
+static inline void sisusb_order_wmem_24bit(char *src, u32 *dst)
+{
+     *dst = (*(src + 2)  << 16) |
+             (*(src + 1) <<  8) |
+             *src;
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_rmem_24bit(u32 *src, char *dst)
+{
+     *dst       = (*src >> 16) & 0xff;
+     *(dst + 1) = (*src >> 8)  & 0xff;
+     *(dst + 2) = *src         & 0xff;
+}
+#else
+static inline void sisusb_order_rmem_24bit(u32 *src, char *dst)
+{
+     *(dst + 2) = (*src >> 16) & 0xff;
+     *(dst + 1) = (*src >> 8)  & 0xff;
+     *dst       = *src         & 0xff;
+}
+#endif
+
+#ifdef __BIG_ENDIAN
+static inline void sisusb_order_mem_32bit(u32 *src, u32 *dst)
+{
+     *dst = ((*src << 16) & 0xffff0000) | ((*src >> 16) & 0xffff);
+}
+#else
+static inline void sisusb_order_mem_32bit(u32 *src, u32 *dst)
+{
+     *dst = *src;
+}
+#endif
+
 static void
 sisusb_free_buffers(struct sisusb_usb_data *sisusb)
 {

Why not merge the #ifdefs, so you need only one?

I think it is because legibly two functions of the same name queues up.
Of course, both are not cared about either.

--
Mitsuhiro KOGA
[off-list ref]

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

On Wed, 2006-08-16 at 20:31 +0900, Mitsuhiro KOGA wrote:

Hi,

What happens if (len & 3) != 0? It ends up doing a regular memcpy()
without the reordering.  Better if you move the if (len & 3) == 0 check
to sisusb_memcpy(), so instead of

                       if ((len & 3) == 0)
                               sisusb_memcpy((u16 *)buffer, (u16 )kernbuffer, passsize / 2);
                       else
                               memcpy(buffer, kernbuffer, passsize);

it simply condenses into

			sisusb_memcpy().

I would agree with Geert. Combine all into a single #ifdef/#else/#endif.
Since we are still in the reviewing phase, do not submit incremental
patches.

Tony



-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

2006/8/16, Antonino A. Daplas [off-list ref]:

On Wed, 2006-08-16 at 20:31 +0900, Mitsuhiro KOGA wrote:

quoted

Hi,

quoted

 static void
 sisusb_free_buffers(struct sisusb_usb_data *sisusb)
 {

@@ -408,7 +465,11 @@ static int sisusb_send_bulk_msg(struct s

              } else if (fromkern) {

-                     memcpy(buffer, kernbuffer, passsize);
+                     if ((len & 3) == 0)
+                             sisusb_memcpy((u16 *)buffer, (u16 *)kernbuffer, passsize / 2);
+                     else
+                             memcpy(buffer, kernbuffer, passsize);

What happens if (len & 3) != 0? It ends up doing a regular memcpy()
without the reordering.  Better if you move the if (len & 3) == 0 check
to sisusb_memcpy(), so instead of

                       if ((len & 3) == 0)
                               sisusb_memcpy((u16 *)buffer, (u16 )kernbuffer, passsize / 2);
                       else
                               memcpy(buffer, kernbuffer, passsize);

(a) if (len & 3) != 0
When former data length is 4 or less, struct sisusb_packet is used.
It becomes 10 in length for writing.
Because data is not used, it becomes 6 in length for reading.

struct sisusb_packet {
    unsigned short header;
    u32 address;
    u32 data;
} __attribute__((__packed__));

(b) if (len & 3) == 0
When former data length is larger than 4,
it corresponds to this case because the length of (data & ~3) is passed.
As for the data of the remainder, the logic of (a) is used.

it simply condenses into

                        sisusb_memcpy().

yes.
I also think that it is good.

quoted

+
                      kernbuffer += passsize;

              }

@@ -872,25 +933,9 @@ static int sisusb_write_mem_bulk(struct
                      if (userbuffer) {
                              if (copy_from_user(&buf, userbuffer, 3))
                                      return -EFAULT;
-#ifdef __BIG_ENDIAN
-                             swap32 = (buf[0] << 16) |
-                                      (buf[1] <<  8) |
-                                      buf[2];
-#else
-                             swap32 = (buf[2] << 16) |
-                                      (buf[1] <<  8) |
-                                      buf[0];
-#endif
+                             sisusb_order_wmem_24bit(&buf, &swap32);
                      } else
-#ifdef __BIG_ENDIAN
-                             swap32 = (kernbuffer[0] << 16) |
-                                      (kernbuffer[1] <<  8) |
-                                      kernbuffer[2];
-#else
-                             swap32 = (kernbuffer[2] << 16) |
-                                      (kernbuffer[1] <<  8) |
-                                      kernbuffer[0];
-#endif
+                             sisusb_order_wmem_24bit(kernbuffer, &swap32);

                      ret = sisusb_write_memio_24bit(sisusb,
                                                      SISUSB_TYPE_MEM,

@@ -907,7 +952,7 @@ static int sisusb_write_mem_bulk(struct
                              if (get_user(swap32, (u32 __user *)userbuffer))
                                      return -EFAULT;
                      } else
-                             swap32 = *((u32 *)kernbuffer);
+                             sisusb_order_mem_32bit((u32 *)kernbuffer, &swap32);

                      ret = sisusb_write_memio_long(sisusb,
                                                      SISUSB_TYPE_MEM,

@@ -1227,15 +1272,7 @@ static int sisusb_read_mem_bulk(struct s
                                                              addr, &swap32);
                      if (!ret) {
                              (*bytes_read) += 3;
-#ifdef __BIG_ENDIAN
-                             buf[0] = (swap32 >> 16) & 0xff;
-                             buf[1] = (swap32 >> 8) & 0xff;
-                             buf[2] = swap32 & 0xff;
-#else
-                             buf[2] = (swap32 >> 16) & 0xff;
-                             buf[1] = (swap32 >> 8) & 0xff;
-                             buf[0] = swap32 & 0xff;
-#endif
+                             sisusb_order_rmem_24bit(&swap32, &buf[0]);
                              if (userbuffer) {
                                      if (copy_to_user(userbuffer, &buf[0], 3))
                                              return -EFAULT;

@@ -1259,7 +1296,7 @@ static int sisusb_read_mem_bulk(struct s

                                      userbuffer += 4;
                              } else {
-                                     *((u32 *)kernbuffer) = swap32;
+                                     sisusb_order_mem_32bit((u32 *)kernbuffer, &swap32);
                                      kernbuffer += 4;
                              }
                              addr += 4;

@@ -3435,6 +3472,9 @@ static void sisusb_disconnect(struct usb

I would agree with Geert. Combine all into a single #ifdef/#else/#endif.
Since we are still in the reviewing phase, do not submit incremental
patches.

Thank you for your advice. I understand.


--
Mitsuhiro KOGA
[off-list ref]

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642

I'm sorry. Because the mistake and lack were found in the explanation,
a postscript is added to it.

2006/8/17, Mitsuhiro KOGA [off-list ref]:

2006/8/16, Antonino A. Daplas [off-list ref]:

quoted

On Wed, 2006-08-16 at 20:31 +0900, Mitsuhiro KOGA wrote:

quoted

Hi,

quoted

 static void
 sisusb_free_buffers(struct sisusb_usb_data *sisusb)
 {

@@ -408,7 +465,11 @@ static int sisusb_send_bulk_msg(struct s

              } else if (fromkern) {

-                     memcpy(buffer, kernbuffer, passsize);
+                     if ((len & 3) == 0)
+                             sisusb_memcpy((u16 *)buffer, (u16 *)kernbuffer, passsize / 2);
+                     else
+                             memcpy(buffer, kernbuffer, passsize);

What happens if (len & 3) != 0? It ends up doing a regular memcpy()
without the reordering.  Better if you move the if (len & 3) == 0 check
to sisusb_memcpy(), so instead of

                       if ((len & 3) == 0)
                               sisusb_memcpy((u16 *)buffer, (u16 )kernbuffer, passsize / 2);
                       else
                               memcpy(buffer, kernbuffer, passsize);

(a) if (len & 3) != 0
When former data length is 4 or less, struct sisusb_packet is used.
It becomes 10 in length for writing.
Because data is not used, it becomes 6 in length for reading.

struct sisusb_packet {
    unsigned short header;
    u32 address;
    u32 data;
} __attribute__((__packed__));

(b) if (len & 3) == 0
When former data length is larger than 4,
it corresponds to this case because the length of (data & ~3) is passed.
As for the data of the remainder, the logic of (a) is used.

The length passed here is not (data & ~3) but (former data length & ~3).

And, variable buffer becomes a character string displayed in the monitor
a couple of byte sequence by two bytes (the character-code and the delimiter).
Because this order reverses for the Big endian, variable buffer is
being replaced.

In the case of (a), sisusb_pack->data is being replaced with
cpu_to_le32 in existing processing,
and because the order of the character reverses for four bytes, data
is being replaced
with sisusb_order_mem_32 further.

Regards,

-- 
Mitsuhiro KOGA
[off-list ref]

-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642