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

["Antonino A. Daplas" <adaplas@gmail.com>]

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

>  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);

it simply condenses into

			sisusb_memcpy().


> +
>  			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.

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