Re: New driver mtipx2xx submission

Re: New driver mtipx2xx submission

[Alan Cox]

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

Re: New driver mtipx2xx submission

[Christoph Hellwig]

On Tue, Jul 26, 2011 at 11:46:40AM +0100, Alan Cox wrote:
> Sorry this has taken a while - I've been away and also dealing with
> various bits of graphics security stuff.

Alan, where did the mail that you reply to originate from?  Care to post
the whole driver to lkml?

Re: New driver mtipx2xx submission

[Alan Cox]

On Tue, 26 Jul 2011 07:44:45 -0400
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Jul 26, 2011 at 11:46:40AM +0100, Alan Cox wrote:
> > Sorry this has taken a while - I've been away and also dealing with
> > various bits of graphics security stuff.
> 
> Alan, where did the mail that you reply to originate from?  Care to post
> the whole driver to lkml?

linux-ide

Alan

Re: New driver mtipx2xx submission

[Jeff Garzik]

On 07/26/2011 07:44 AM, Christoph Hellwig wrote:
> On Tue, Jul 26, 2011 at 11:46:40AM +0100, Alan Cox wrote:
>> Sorry this has taken a while - I've been away and also dealing with
>> various bits of graphics security stuff.
>
> Alan, where did the mail that you reply to originate from?  Care to post
> the whole driver to lkml?

The original, full post was back in April:
http://marc.info/?l=linux-ide&m=130400649131349&w=2

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

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.

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