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

[Jeff Garzik <jeff@garzik.org>]

Zoltan Boszormenyi wrote:
> 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,

that's a bug.  fixed.

I also applied your cleanup.