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

[Zoltan Boszormenyi <zboszor@dunaweb.hu>]

Jeff Garzik írta:
> Zoltan Boszormenyi wrote:
>> Jeff Garzik írta:
>>> 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.
>>
>> Thanks, but you took the wrong cleanup patch.
>> My original, i.e. this chunk below is wrong.
>>
>> @@ -2291,8 
>> <http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=blob;f=drivers/ata/sata_nv.c;h=540f218e10e56c8a85a937bc920a5d96434bcc8f;hb=41f0f8af3edfccd58b5c9d714928d773a072b693#l2291> 
>> +2283,7 
>> <http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.git;a=blob;f=drivers/ata/sata_nv.c;h=ffd00f5c533d7b58dde67cfef7d2191ca15ea140;hb=5a5a9e1890b8260686218a68862d880daee1a817#l2283> 
>> @@ static void nv_swncq_host_interrupt(struct ata_port *ap, u16 fis)
>>                 */
>>                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;
>>
>> It should be a binary OR between the flags. This caused immediate
>> NCQ errors upon boot and lead to disabling NCQ.
>> My second patch has it corrected.
>
> fixed

Thanks. And would you please also enable swncq by default? :-)
It's very stable.

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