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.

Thanks for taking the time to review the errata, performance profiles,
and early driver code.  We've cleaned up much of the ugliness in the
version you inspected so it should be easier on the eyes now.

We changed the driver name from mtipx2xx to mtip32xx. Open to a generic
name if other vendors are planning to use this driver.

Comments on the driver code

Questions:
  Should there be security checks on the ioctl interfaces ?

We added capable(CAP_SYS_ADMIN) checks on the ioctls.

Code:
  Use k[mz]alloc/kfree for small objects like structs, vmalloc has a lot
  of ovherad you don't need

All vmallocs were converted to kzallocs.

- 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

We converted all non-static functions to kernel-compatible nomenclature.
We used the mtip prefix based on your suggestion.

- 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

Here we had some trouble.  We needed the counting semaphore to put the
make_request calling context to sleep if there are no empty slots.  We
also needed the rw semaphore to prioritize internal commands and ioctls
during heavy IO load.  There seemed to be a fairness problem that was
best solved through the rw semaphore.  If you have another suggestion
for a "fair" semaphore, we'd love to hear it.

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)

We did our best to make the comments and format consistent with other
drivers.

- 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)

We've converted much of the logging to dev_* semantics.  The challenge
for us was reconciling all the messages that we feel are important
against the need to not spam the system log.  We think we've made a
reasonable compromise, at least compared to the last driver we posted.

We are of course open to removing any logging deemed superfluous.

- for ata_swap_string look at bswap()

We experimented with bswap but in the end felt that be16_to_cpus
was a better choice.

I will post the updated patch shortly.

-- 
Regards,
Asai Thambi