On Thu, 2013-06-13 at 10:30 -0400, Bradley Grove wrote: > This is a new driver for ATTO Technology's ExpressSAS series of hardware RAID > adapters. It supports the following adapters: > > - ExpressSAS R60F > - ExpressSAS R680 > - ExpressSAS R608 > - ExpressSAS R644 > > This patch is split into ten parts to make reviewing easier. You'll need to > apply all of them to get the complete patch. > > We have done some testing under x86 and x64 architectures, but the code is > still under development and we expect to find additional issues. We appreciate > any review comments. This isn't an exhaustive review by any means, but what I noticed just from a quick skim over the driver is this: > +#ifdef HOST_BIG_ENDIAN > + *((u16 *)&vrq->scsi.handle + 0) = a->cmd_ref_no++; > +#else > + *((u16 *)&vrq->scsi.handle + 1) = a->cmd_ref_no++; > +#endif Looks obviously wrong (there is no HOST_BIG_ENDIAN define in the code). I think what you want here is *((u16 *)&vrq->scsi.handle) = cpu_to_be16(a->cmd_ref_no++); There's also a lot of open coded linked list handling code like this: > +/* Remove a request from a double-linked list */ > +static inline void esas2r_dequeue(struct esas2r_request *rq) and > +/* Add a request to a double-linked list */ > +static inline void esas2r_enqueue(struct esas2r_request *head, > + struct esas2r_request *rq) Please don't do this. Use the linux struct list_head (in list.h) instead ... it's a common pattern that everyone thinks they have to write their own linked lists and everyone invariably makes a mistake doing it ... just using the provided handlers avoids a lot of the problems. You also have some hidden singly linked list open coding (like the free_sg_list) which you should use the struct hlist primitives for. Could you go through the driver and check for primitives which should already be in Linux? Another example of this is all the alignment ones you have in the file ... you should be using the ALIGN macro. You have another pattern of using memmove() where you should be using memcpy(). memmove() is for the case where source and destination may overlap and it can be less efficient than memcpy on a lot of platforms. If I read the code right, you've got a lun limit at 255, so you should probably be setting max_lun in the host template (and checking the value) to prevent accidents since you use the rest of the flags word for command data. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
