On 7/26/2011 4:46 AM, Alan Cox wrote: > Sorry this has taken a while - I've been away and also dealing with > various bits of graphics security stuff. > > I've now been through the errata, the timing data and the driver code in > somewhat more detail > > Overall: > The hardware deviates a bit from AHCI. The AHCI driver could be taught > to support it but even with the longer queue supported it's not clear > this is the right path, and some of the error handling needs deviate a > bit. > > The performance numbers are pretty definitive, and the data shows that > is mostly higher up in the queue handling. That's awkward in some ways > because it means there isn't an obvious way to fix it, and we still > want the queue stuff for 'normal' disks. > > Looking at other vendors there don't seem to be a pile of them also > planning to do AHCI with extras instead most seem focussed on NVHMCI so > it doesn't look like a pile of near identikit drivers will appear. Also > if they do we would probably want them all to be related to this driver > not to the general AHCI driver. > > So having gone over it all I think the case is rather well made for this > being added as its own driver matching their specific PCI idents, but with > some code clean up, and possibly some further compatibility code if it > turns out some general ide/scsi tools don't work on it as expected. > > Comments on the driver code > > Questions: > Should there be security checks on the ioctl interfaces ? > > Code: > Use k[mz]alloc/kfree for small objects like structs, vmalloc has a lot > of ovherad you don't need > > - Lots of global function names with general naming. This causes problems > in Linux because all the compiled in drivers share a common namespace. > So they really ought to be something like > > mtip_ahci_write() > > and so on > > - Semaphores. Unless you need the counting properties please use mutexes. > Sempahores really make for problems in hard real time environments if > using the -rt kernel additions > > Style: > - Confuses our kernel-doc tools as it has its own different comment > extraction format. That wants pulling into line (it looks like all the > info is there and its a 'perl script from hell' sort of conversion) > > - Various struct names in capitals - please search/replace those as for > style we keep capitals for defines > > - Various ifdefs and a lot of printk stuff. Some of this is clearly > because its a development driver, but it ought to be tidied for a final > submission. Also use of dev_info/dev_err etc is strongly preferred as > it means a user and tools can clearly identify which device generated > the message (dev_dbg() supports runtime debug switching so may also > deal with stuff you'd otherwise remove later) > > - for ata_swap_string look at bswap() Thanks for your valuable feedback. We started making changes to the driver, and expect to complete it soon. -- 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
