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