On 9/9/2011 2:58 AM, Jens Axboe wrote: > 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. For the device to support surprise removal and surprise insertion (SRSI), need pciehp module loaded. -- Regards, Asai Thambi -- 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
