| Message ID | 20090805192055.8F2DE1007C@gold.linx.net |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 08/05/2009 01:20 PM, Tony Vroon wrote: > The nVidia MCP55 SATA2 controller quite happily supports MSI. > This adds an option to use it. It is disabled by default and > will only be honoured on the specific controller I tested. > This was suggested in 2007 back when the driver was less mature, > perhaps now is a better time for it. I'm not sure the conditional on MCP55 is necessary, I would be inclined to just try to enable it on any device if the option is specified. pci_enable_msi will just fail harmlessly if the device doesn't support MSI capability or the kernel detects MSI is not usable on the machine (which these days I think we should be able to do fairly accurately on HT chipsets..) > > Signed-off-by: Tony Vroon<tony@linx.net> > > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > index b2d11f3..5ec29c4 100644 > --- a/drivers/ata/sata_nv.c > +++ b/drivers/ata/sata_nv.c > @@ -602,6 +602,7 @@ MODULE_VERSION(DRV_VERSION); > > static int adma_enabled; > static int swncq_enabled = 1; > +static int msi_enabled; > > static void nv_adma_register_mode(struct ata_port *ap) > { > @@ -2459,6 +2460,13 @@ static int nv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) > } else if (type == SWNCQ) > nv_swncq_host_init(host); > > + /* enable MSI if requested */ > + if (pdev->device == PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SATA2&& > + msi_enabled) { > + dev_printk(KERN_NOTICE,&pdev->dev, "Using MSI\n"); > + pci_enable_msi(pdev); > + } > + > pci_set_master(pdev); > return ata_host_activate(host, pdev->irq, ipriv->irq_handler, > IRQF_SHARED, ipriv->sht); > @@ -2558,4 +2566,6 @@ module_param_named(adma, adma_enabled, bool, 0444); > MODULE_PARM_DESC(adma, "Enable use of ADMA (Default: false)"); > module_param_named(swncq, swncq_enabled, bool, 0444); > MODULE_PARM_DESC(swncq, "Enable use of SWNCQ (Default: true)"); > +module_param_named(msi, msi_enabled, bool, 0444); > +MODULE_PARM_DESC(msi, "Enable use of MSI (Default: false)"); > > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel- > owner@vger.kernel.org] On Behalf Of Robert Hancock > Sent: Wednesday, August 05, 2009 7:33 PM > To: Tony Vroon > Cc: Jeff Garzik; linux-ide@vger.kernel.org; LKML; Philip Langdale > Subject: Re: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for > sata_nv > > On 08/05/2009 01:20 PM, Tony Vroon wrote: > > The nVidia MCP55 SATA2 controller quite happily supports MSI. > > This adds an option to use it. It is disabled by default and > > will only be honoured on the specific controller I tested. > > This was suggested in 2007 back when the driver was less mature, > > perhaps now is a better time for it. > > I'm not sure the conditional on MCP55 is necessary, I would be inclined > to just try to enable it on any device if the option is specified. > pci_enable_msi will just fail harmlessly if the device doesn't support > MSI capability or the kernel detects MSI is not usable on the machine > (which these days I think we should be able to do fairly accurately on > HT chipsets..) > disable_msi() is missing right? > > > > Signed-off-by: Tony Vroon<tony@linx.net> > > > > diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c > > index b2d11f3..5ec29c4 100644 > > --- a/drivers/ata/sata_nv.c > > +++ b/drivers/ata/sata_nv.c > > @@ -602,6 +602,7 @@ MODULE_VERSION(DRV_VERSION); > > > > static int adma_enabled; > > static int swncq_enabled = 1; > > +static int msi_enabled; > > > > static void nv_adma_register_mode(struct ata_port *ap) > > { > > @@ -2459,6 +2460,13 @@ static int nv_init_one(struct pci_dev *pdev, > const struct pci_device_id *ent) > > } else if (type == SWNCQ) > > nv_swncq_host_init(host); > > > > + /* enable MSI if requested */ > > + if (pdev->device == PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SATA2&& > > + msi_enabled) { > > + dev_printk(KERN_NOTICE,&pdev->dev, "Using MSI\n"); > > + pci_enable_msi(pdev); > > + } > > + > > pci_set_master(pdev); > > return ata_host_activate(host, pdev->irq, ipriv->irq_handler, > > IRQF_SHARED, ipriv->sht); .. msi_rc = pci_enable_msi(pdev); ... ata_rc = ata_host_activate(host, pdev->irq, ipriv->irq_handler, IRQF_SHARED, ipriv->sht); if (ata_rc && msi_enabled && !msi_rc) pci_disable_msi(pdev); return ata_rc; Chetan
> disable_msi() is missing right? I didn't add that as none of the other drivers have it: chainsaw@amalthea /cvs/linux-2.6/drivers/ata $ grep _msi * | grep pci ahci.c: pci_enable_msi(pdev); sata_mv.c: if (msi && pci_enable_msi(pdev) == 0) sata_vsc.c: if (pci_enable_msi(pdev) == 0) (This is a tree without the sata_nv change I submitted) I do believe it is safe to shut the interrupt down and unload the handler whilst it is still in MSI mode. At least, I don't see the libata core special-casing it in any way. > Chetan Regards, Tony V.
> -----Original Message----- > From: Tony Vroon [mailto:tony@linx.net] > Sent: Thursday, August 06, 2009 11:59 AM > To: Loke,Chetan > Cc: hancockrwd@gmail.com; jgarzik@pobox.com; linux-ide@vger.kernel.org; > linux-kernel@vger.kernel.org; philipl@overt.org > Subject: RE: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for > sata_nv > > > disable_msi() is missing right? > > I didn't add that as none of the other drivers have it: Then they would leak the MSI-vectors if request_irq fails. > chainsaw@amalthea /cvs/linux-2.6/drivers/ata $ grep _msi * | grep pci > ahci.c: pci_enable_msi(pdev); > sata_mv.c: if (msi && pci_enable_msi(pdev) == 0) > sata_vsc.c: if (pci_enable_msi(pdev) == 0) > > (This is a tree without the sata_nv change I submitted) > > I do believe it is safe to shut the interrupt down and unload the > handler whilst it is still in MSI mode. At least, I don't see the libata > core special-casing it in any way. If I'm not wrong then that's how it's supposed to be done. free_irq and then disable_msi. You can't reverse the order. Also the driver should know when to quiesce the ASIC. So quiesce first and then shutdown everything. > Regards, > Tony V. Regards Chetan -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/06/2009 10:28 AM, Chetan.Loke@Emulex.Com wrote: >> -----Original Message----- >> From: Tony Vroon [mailto:tony@linx.net] >> Sent: Thursday, August 06, 2009 11:59 AM >> To: Loke,Chetan >> Cc: hancockrwd@gmail.com; jgarzik@pobox.com; linux-ide@vger.kernel.org; >> linux-kernel@vger.kernel.org; philipl@overt.org >> Subject: RE: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for >> sata_nv >> >>> disable_msi() is missing right? >> I didn't add that as none of the other drivers have it: > > Then they would leak the MSI-vectors if request_irq fails. > > >> chainsaw@amalthea /cvs/linux-2.6/drivers/ata $ grep _msi * | grep pci >> ahci.c: pci_enable_msi(pdev); >> sata_mv.c: if (msi&& pci_enable_msi(pdev) == 0) >> sata_vsc.c: if (pci_enable_msi(pdev) == 0) >> >> (This is a tree without the sata_nv change I submitted) >> >> I do believe it is safe to shut the interrupt down and unload the >> handler whilst it is still in MSI mode. At least, I don't see the libata >> core special-casing it in any way. > > If I'm not wrong then that's how it's supposed to be done. free_irq and then disable_msi. You can't reverse the order. Also the driver should know when to quiesce the ASIC. So quiesce first and then shutdown everything. Seems like devres should handle this somehow, or else something in libata core.. Tejun? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Robert Hancock wrote: >> If I'm not wrong then that's how it's supposed to be done. free_irq >> and then disable_msi. You can't reverse the order. Also the driver >> should know when to quiesce the ASIC. So quiesce first and then >> shutdown everything. > > Seems like devres should handle this somehow, or else something in > libata core.. Tejun? Yeah, if the device was enabled with pcim_enable_device(), devres will take of all the cleanups. No need to worry about them. Thanks.
On Fri, 2009-08-07 at 08:26 +0900, Tejun Heo wrote: > Yeah, if the device was enabled with pcim_enable_device(), devres will > take of all the cleanups. Confirmed Tejun, that pcim_enable_device call is made earlier on in nv_init_one. I was careful to call the MSI enable at exactly the same point between enabling master & returning control as sata_mv did. > No need to worry about them. Glad to hear. > Thanks. Regards, Tony V.
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c index b2d11f3..5ec29c4 100644 --- a/drivers/ata/sata_nv.c +++ b/drivers/ata/sata_nv.c @@ -602,6 +602,7 @@ MODULE_VERSION(DRV_VERSION); static int adma_enabled; static int swncq_enabled = 1; +static int msi_enabled; static void nv_adma_register_mode(struct ata_port *ap) { @@ -2459,6 +2460,13 @@ static int nv_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) } else if (type == SWNCQ) nv_swncq_host_init(host); + /* enable MSI if requested */ + if (pdev->device == PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SATA2 && + msi_enabled) { + dev_printk(KERN_NOTICE, &pdev->dev, "Using MSI\n"); + pci_enable_msi(pdev); + } + pci_set_master(pdev); return ata_host_activate(host, pdev->irq, ipriv->irq_handler, IRQF_SHARED, ipriv->sht); @@ -2558,4 +2566,6 @@ module_param_named(adma, adma_enabled, bool, 0444); MODULE_PARM_DESC(adma, "Enable use of ADMA (Default: false)"); module_param_named(swncq, swncq_enabled, bool, 0444); MODULE_PARM_DESC(swncq, "Enable use of SWNCQ (Default: true)"); +module_param_named(msi, msi_enabled, bool, 0444); +MODULE_PARM_DESC(msi, "Enable use of MSI (Default: false)");
The nVidia MCP55 SATA2 controller quite happily supports MSI. This adds an option to use it. It is disabled by default and will only be honoured on the specific controller I tested. This was suggested in 2007 back when the driver was less mature, perhaps now is a better time for it. Signed-off-by: Tony Vroon <tony@linx.net> -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html