Message-ID: <20110726114640.2b05dd34@lxorguk.ukuu.org.uk>
Date:	Tue, 26 Jul 2011 11:46:40 +0100
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	<asamymuthupa@...ron.com>
Cc:	Jeff Moyer <jmoyer@...hat.com>, <linux-ide@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: New driver mtipx2xx submission

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


Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists