Re: [patch 05/25] ata: add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61

[Zoltan Boszormenyi <zboszor@dunaweb.hu>]

[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]

Hi,

Jeff Garzik írta:
> akpm@linux-foundation.org wrote:
>> From: Kuan Luo <kluo@nvidia.com>
>>
>> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
>> controller.  NCQ function is disable by default, you can enable it with
>> 'swncq=1'.  NCQ will be turned off if the drive is Maxtor on MCP51 or 
>> MCP55
>> rev 0xa2 platform.
>>
>> [akpm@linux-foundation.org: build fix]
>> Signed-off-by: Kuan Luo <kluo@nvidia.com>
>> Signed-off-by: Peer Chen <pchen@nvidia.com>
>> Cc: Zoltan Boszormenyi <zboszor@dunaweb.hu>
>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> ---
>>
>>  drivers/ata/sata_nv.c |  860 +++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 851 insertions(+), 9 deletions(-)
>
> I finally gave this a thorough review.
>
> Overall, good work.  The state transitions all seem solid.  I made 
> several minor changes and cleanups, and checked it into the 'nv-swncq' 
> branch of 
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/libata-dev.git
>
> Two hurdles before I'm ready to push upstream:
>
> * someone please verify my minor changes did not break anything; I 
> don't have real hardware

After reading the diff between the original and your cleaned up version
it seems both the change from 4 individual flags to a single integer and the
nv_swncq_bmdma_stop() -> __ata_bmdma_stop() transition are obviously 
correct.
I attached a small cleanup patch which may make one check a bit more 
readable.

However, can you explain this chunk below? Why isn't it needed?

@@ -615,7 +622,6 @@ static const struct ata_port_info nv_por
        {
                .sht            = &nv_swncq_sht,
                .flags          = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY,
-               .link_flags     = ATA_LFLAG_HRST_TO_RESUME,
                .pio_mask       = NV_PIO_MASK,
                .mwdma_mask     = NV_MWDMA_MASK,
                .udma_mask      = NV_UDMA_MASK,


> * the pp->lock appears unnecessary at best, wrong at worst.  needs 
> additional analysis.

I will test the code without this locking. Can you give me a better idea
besides beating both my disks with separate requests? Say, bonnie++
and simultaneous hdparm -tT on both?

Best regards,
Zoltán Böszörményi


[-- Attachment #2: swncq-cleanup.patch --]
[-- Type: text/x-patch, Size: 505 bytes --]

--- sata_nv.c.committed	2007-09-21 06:49:23.000000000 +0200
+++ sata_nv.c.cleanup-swncq	2007-09-21 07:47:24.000000000 +0200
@@ -2290,8 +2290,7 @@
 		 */
 		pp->dhfis_bits |= (0x1 << pp->last_issue_tag);
 		pp->ncq_flags |= ncq_saw_d2h;
-		if ((pp->ncq_flags & ncq_saw_sdb) ||
-		    (pp->ncq_flags & ncq_saw_backout)) {
+		if (pp->ncq_flags & (ncq_saw_sdb || ncq_saw_backout)) {
 			ata_ehi_push_desc(ehi, "illegal fis transaction");
 			ehi->err_mask |= AC_ERR_HSM;
 			ehi->action |= ATA_EH_HARDRESET;