[PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Tony Vroon]

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>

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)");
 

Re: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Robert Hancock]

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
>

RE: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Chetan.Loke]

> -----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

RE: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Tony Vroon]

[-- Attachment #1: Type: text/plain, Size: 562 bytes --]

> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Chetan.Loke]

> -----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

Re: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Robert Hancock]

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?

Re: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Tejun Heo]

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.

-- 
tejun

Re: [PATCH 2.6.32 v2] MCP55 SATA2 conditional MSI support for sata_nv

[Tony Vroon]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]