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

[Mitsuhiro K. <shi...@gm...>]

2006/8/16, Antonino A. Daplas <ad...@gm...>:
> 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);
>

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

> > +
> >                       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
<shi...@gm...>