Jeff Garzik wrote: > 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 > I see. Both your methods are perfect. Because the nv_swncq_ops and nv_swncq_interrupt can be also used when sending non ncq cmd, It seems like: /* MCP65 */ { .sht = &nv_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, .port_ops = &nv_swncq_ops, .irq_handler = nv_swncq_interrupt, }, Is it right? Best regards, Kuan Luo ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- - 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
