On 2011-09-09 10:54, Christoph Hellwig wrote:

On Mon, Aug 22, 2011 at 02:28:11PM -0700, Sam Bradshaw (sbradshaw) wrote:

quoted

New patch for mtip32xx driver based on feedback from
Jens Axboe, Christoph Hellwig, and Alan Cox.

A few comments that need addressing:

 - the ioctl handler always returns 0 for the BLKFLSBUF ioctl, without
   doing anything.  Given that the ioctl is supposed to flush all kinds
   of higher level cached data this is a serious data integrity issue.
   It must simply do the default -ENOTTY return for it and let the block
   layer do the right thing.
 - handling of REQ_FUA / REQ_FLUSH requests is completely broken.
   There is a weird barrier flag to mtip_hw_submit_io which set the
   hwardware FUA bit if the FLUSH bit is set on a request.
   Please take a look at how this should be handled, the
   Documentation/block/writeback_cache_control.txt file is the canonical
   resource.  Implementing your driver at the make_request layer
   unfortunately means you will have to do all the hard work yourself.
 - also the call to blk_queue_flush(queue, 0); from ->make_request for
   a non-data request is completely wrong.

I noticed both of these flush/fua problems too and have fixed them up.

 - the 64-bit case in fill_command_sg will blow up on big endian
   systems, please use your current 32-bit case unconditionally
 - mtip_block_getgeo should just use sector_div instead of the ifdef
   mess.
 - please check the driver using sparse, and most importantly the
   optional endianess checking pass of it.  Currently the driver
   uses plain unsigned int and similar types for little endian
   hardware structures.  Take a look at Documentation/sparse.txt
   on how to use it.
 - the mtip_check_surprise_removal check should be unconditional, not
   under #ifdef CONFIG_HOTPLUG

In general, the dependency on HOTPLUG_PCI_PCIE is odd as well.

 - Given that the driver has a single c implementation file all symbols
   should be marked static, and there should be no prototypes in the
   header

Sam, would be nice if you could send in patches for the other bugs that
Christoph have pointed out. I already found and fixed the flush parts.

-- 
Jens Axboe