Re: New driver mtipx2xx submission

[Asai Thambi S P]

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/