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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux