> On 2011-09-09 10:54, Christoph Hellwig wrote: > > On Mon, Aug 22, 2011 at 02:28:11PM -0700, Sam Bradshaw (sbradshaw) wrote: > >> 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. Will do. I'll send out patches against for-3.2/mtip32xx, given that Jens has already made some changes. Thanks for the tip on sparse endianness checking. I wasn't aware of this tool until now. -Sam -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
