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

[Jeff Garzik <jeff@garzik.org>]

Kuan Luo wrote:
> Jeff Garzik wrote:
>> Kuan Luo wrote:
>>> hi,
>>> i saw the below changes  from
>>>
>> http://git.kernel.org/?p=linux/kernel/git/jgarzik/libata-dev.g
>> it;a=commi
>>> tdiff;h=5a5a9e1890b8260686218a68862d880daee1a817
>>>
>>> [libata] sata_nv: Clean up ATA_FLAG_NCQ usage; bit test micro-opt
>>>
>>>  @@ -622,7 +622,9 @@ static const struct ata_port_info 
>> nv_port_info[] =
>>> {
>>>         /* SWNCQ */
>>>         {
>>>                 .sht            = &nv_swncq_sht,
>>> -               .flags          = ATA_FLAG_SATA | 
>> ATA_FLAG_NO_LEGACY,
>>> +               .flags          = ATA_FLAG_SATA | 
>> ATA_FLAG_NO_LEGACY |
>>> +                                 ATA_FLAG_NCQ,
>>> +               .link_flags     = ATA_LFLAG_HRST_TO_RESUME,
>>>                 .pio_mask       = NV_PIO_MASK,
>>>                 .mwdma_mask     = NV_MWDMA_MASK,
>>>                 .udma_mask      = NV_UDMA_MASK,
>>>
>>> If swncq_enabled is set zero by user, nv_swncq_host_init 
>> would not be
>>> called .
>>>  Then ncq should be disabled and the libata shouldn't send ncq cmd. 
>>>  I am not sure whether the  ATA_FLAG_NCQ flag which is in 
>> default set
>>> makes libata send ncq cmd even when swncq_enable is 0?
>>>
>>> nv_init_one
>>> 	} else if (type == SWNCQ && swncq_enabled) {
>>> 		dev_printk(KERN_NOTICE, &pdev->dev, "Using SWNCQ
>>> mode\n");
>>> 		nv_swncq_host_init(host);
>> Thanks for watching!  Yes, that looks like a problem.
>>
>> I still feel the flags usage I removed was problematic.  A better 
>> solution, I think, would be to do follow the ADMA example of 
>> switching 
>> the port_info type during early initialization:
>>
>> 	if (type >= CK804 && adma_enabled) {
>> 		type = ADMA;
>>
>> which in our case would result in
>>
>> 	if (type == SWNCQ && !swncq_enabled)
>> 		type = GENERIC;
>>
>> Or, if you prefer, we could take the opposite route, _not_ 
>> set SWNCQ in 
>> the pci_device_id table, and instead do something like
>>
>> 	if (type >= MCP65 && swncq_enabled)
>> 		type = SWNCQ;
>>
>> (MCP65 is just a pulled-out-of-thin-air example)
>>
>> Regards,
>>
>> 	Jeff
>>
> 1.The former method "type =GENERIC" cann't support hotplug in
> mcp51-55-61.

'GENERIC' just an example; my apologies for the confusion.


> 2.As to the latter mehthod, do you mean we may set MCP65 in
> pci_device_id table and add the extra variable
> static const struct ata_port_operations nv_MCP65_ops ={...}.

Correct.

The main point was to fall back from SWNCQ to <something else>, if 
!swncq_enabled, to avoid changing the ap->flags value after the port has 
been allocated and initialized internally.  And that switch must occur 
prior to ata_pci_prepare_sff_host() call in nv_init_one().

Regards,

	Jeff