Re: [PATCH] Add MSI support to sata_vsc driver

Re: [PATCH] Add MSI support to sata_vsc driver

[Jeremy Higdon]

On Tue, Aug 15, 2006 at 05:25:14PM -0700, Wolstenholme, Daniel E wrote:
> 
>    In the course of some work I'm doing with MSI interrupts, I had an
>    Intel GD81344 card I wanted to try out with MSI interrupts, and I
>    found that it does indeed support MSI.  Here's a patch to add support.

You sent the patch as html.  That's not going to work.  Even the text/plain
part of it is quoted/printable with lots of extra blank lines, etc.  If
you're using Outlook, the only way to send a patch is to first turn off
html and then send the patch as a text/plain attachment (as far as I know).

>    I implemented this rather simple patch using #ifdef CONFIG_PCI_MSI.  I
>    noticed some other drivers, such as sata_mv and LSI's fusion drivers,
>    use a kernel command-line parameter to enable MSI support.  I honestly
>    don't know the advantage to this; it seems to me to be easier if it's
>    enabled by default, as long as MSI support is selected in the kernel
>    configuration.  If not, someone please let me know.

It might be that some platforms that have MSI enabled won't support it
on certain machines where these chips are installed.  So it's probably
best to make it an option.  I'll defer to jgarzik on this, however.

jeremy

>    This patch is against 2.6.18-rc1.  There shouldn't be any problems
>    applying it to other 2.6.18 versions.
> 
> 
>    Dan
> 
> 
>    diff -Naur linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c
>    linux-2.6.18-rc1/drivers/scsi/sata_vsc.c
> 
>    --- linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c   2006-08-15
>    17:13:53.000000000 -0700
> 
>    +++ linux-2.6.18-rc1/drivers/scsi/sata_vsc.c    2006-08-15
>    16:02:26.000000000 -0700
> 
>    @@ -406,13 +406,23 @@
> 
>             */
> 
>            pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> 
> 
>    +#ifdef CONFIG_PCI_MSI
> 
>    +       if (pci_enable_msi(pdev) == 0) {
> 
>    +               probe_ent->irq_flags &= ~IRQF_SHARED;
> 
>    +       }
> 
>    +       else {
> 
>    +               probe_ent->irq_flags = IRQF_SHARED;
> 
>    +       }
> 
>    +#else
> 
>    +       probe_ent->irq_flags = IRQF_SHARED;
> 
>    +#endif
> 
>    +
> 
>            probe_ent->sht = &vsc_sata_sht;
> 
>            probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> 
>                                    ATA_FLAG_MMIO;
> 
>            probe_ent->port_ops = &vsc_sata_ops;
> 
>            probe_ent->n_ports = 4;
> 
>            probe_ent->irq = pdev->irq;
> 
>    -       probe_ent->irq_flags = IRQF_SHARED;
> 
>            probe_ent->mmio_base = mmio_base;
> 
> 
>            /* We don't care much about the PIO/UDMA masks, but the core
>    won't like us

Re: [PATCH] Add MSI support to sata_vsc driver

[Jeff Garzik]

Jeremy Higdon wrote:
> On Tue, Aug 15, 2006 at 05:25:14PM -0700, Wolstenholme, Daniel E wrote:
>>    In the course of some work I'm doing with MSI interrupts, I had an
>>    Intel GD81344 card I wanted to try out with MSI interrupts, and I
>>    found that it does indeed support MSI.  Here's a patch to add support.
> 
> You sent the patch as html.  That's not going to work.  Even the text/plain
> part of it is quoted/printable with lots of extra blank lines, etc.  If
> you're using Outlook, the only way to send a patch is to first turn off
> html and then send the patch as a text/plain attachment (as far as I know).
> 
>>    I implemented this rather simple patch using #ifdef CONFIG_PCI_MSI.  I
>>    noticed some other drivers, such as sata_mv and LSI's fusion drivers,
>>    use a kernel command-line parameter to enable MSI support.  I honestly
>>    don't know the advantage to this; it seems to me to be easier if it's
>>    enabled by default, as long as MSI support is selected in the kernel
>>    configuration.  If not, someone please let me know.
> 
> It might be that some platforms that have MSI enabled won't support it
> on certain machines where these chips are installed.  So it's probably
> best to make it an option.  I'll defer to jgarzik on this, however.

There's no need for #ifdefs at all.  You call pci_enable_msi() 
unconditionally, and if it fails, don't configure/use MSI.  From the 
driver's perspective, everything is handled with runtime function calls.

	Jeff

RE: [PATCH] Add MSI support to sata_vsc driver

[Wolstenholme, Daniel E]

Unfortunately, I'm stuck using Lookout for now.  Hopefully this one will
get through.

I removed the #ifdef, and am calling pci_enable_msi() conditionally as
suggested.

Thanks!
Dan

--------------
diff -Naur linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c
linux-2.6.18-rc1/drivers/scsi/sata_vsc.c
--- linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c   2006-08-15
17:13:53.000000000 -0700
+++ linux-2.6.18-rc1/drivers/scsi/sata_vsc.c    2006-08-17
14:29:27.000000000 -0700
@@ -406,13 +406,17 @@
         */
        pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);

+       if (pci_enable_msi && pci_enable_msi(pdev) == 0)
+               probe_ent->irq_flags &= ~IRQF_SHARED;
+       else
+               probe_ent->irq_flags = IRQF_SHARED;
+
        probe_ent->sht = &vsc_sata_sht;
        probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
                                ATA_FLAG_MMIO;
        probe_ent->port_ops = &vsc_sata_ops;
        probe_ent->n_ports = 4;
        probe_ent->irq = pdev->irq;
-       probe_ent->irq_flags = IRQF_SHARED;
        probe_ent->mmio_base = mmio_base;

        /* We don't care much about the PIO/UDMA masks, but the core
won't like us
-----------------



-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: Thursday, August 17, 2006 5:31 AM
To: Jeremy Higdon
Cc: Wolstenholme, Daniel E; linux-ide@vger.kernel.org
Subject: Re: [PATCH] Add MSI support to sata_vsc driver

Jeremy Higdon wrote:
> On Tue, Aug 15, 2006 at 05:25:14PM -0700, Wolstenholme, Daniel E
wrote:
>>    In the course of some work I'm doing with MSI interrupts, I had an
>>    Intel GD81344 card I wanted to try out with MSI interrupts, and I
>>    found that it does indeed support MSI.  Here's a patch to add
support.
> 
> You sent the patch as html.  That's not going to work.  Even the
text/plain
> part of it is quoted/printable with lots of extra blank lines, etc.
If
> you're using Outlook, the only way to send a patch is to first turn
off
> html and then send the patch as a text/plain attachment (as far as I
know).
> 
>>    I implemented this rather simple patch using #ifdef
CONFIG_PCI_MSI.  I
>>    noticed some other drivers, such as sata_mv and LSI's fusion
drivers,
>>    use a kernel command-line parameter to enable MSI support.  I
honestly
>>    don't know the advantage to this; it seems to me to be easier if
it's
>>    enabled by default, as long as MSI support is selected in the
kernel
>>    configuration.  If not, someone please let me know.
> 
> It might be that some platforms that have MSI enabled won't support it
> on certain machines where these chips are installed.  So it's probably
> best to make it an option.  I'll defer to jgarzik on this, however.

There's no need for #ifdefs at all.  You call pci_enable_msi() 
unconditionally, and if it fails, don't configure/use MSI.  From the 
driver's perspective, everything is handled with runtime function calls.

	Jeff

Re: [PATCH] Add MSI support to sata_vsc driver

[Jeff Garzik]

Wolstenholme, Daniel E wrote:
> Unfortunately, I'm stuck using Lookout for now. 

That's OK, this one needs another iteration anyway :)


> --------------
> diff -Naur linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c
> linux-2.6.18-rc1/drivers/scsi/sata_vsc.c
> --- linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c   2006-08-15
> 17:13:53.000000000 -0700
> +++ linux-2.6.18-rc1/drivers/scsi/sata_vsc.c    2006-08-17
> 14:29:27.000000000 -0700
> @@ -406,13 +406,17 @@
>          */
>         pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> 
> +       if (pci_enable_msi && pci_enable_msi(pdev) == 0)

pci_enable_msi is not a variable, don't test it.  Just call it.

The function will always exist, even if conditionally implemented as a 
macro in the no-op case.


> +               probe_ent->irq_flags &= ~IRQF_SHARED;

We're doing an assignment (as you see below, from the code you 
modified), and the entire struct is memset(3) to zero, so there's no 
need to clear this flag.  Just assign it, for the non-MSI case.


> +               probe_ent->irq_flags = IRQF_SHARED;
> +
>         probe_ent->sht = &vsc_sata_sht;
>         probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>                                 ATA_FLAG_MMIO;
>         probe_ent->port_ops = &vsc_sata_ops;
>         probe_ent->n_ports = 4;
>         probe_ent->irq = pdev->irq;
> -       probe_ent->irq_flags = IRQF_SHARED;
>         probe_ent->mmio_base = mmio_base;

Re: [PATCH] Add MSI support to sata_vsc driver

[Jeremy Higdon]

On Thu, Aug 17, 2006 at 07:44:11PM -0400, Jeff Garzik wrote:
> Wolstenholme, Daniel E wrote:
> >Unfortunately, I'm stuck using Lookout for now. 
> 
> That's OK, this one needs another iteration anyway :)

Daniel, you'll need to send as a text attachment, too.
Your previous patch has some linewrap.  Unless some M$ expert
knows differently, I think the text attachment is the only way
to avoid that.

jeremy

RE: [PATCH] Add MSI support to sata_vsc driver

[Wolstenholme, Daniel E]

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

Ok, this patch should fix everything.

Thanks for the help!
Dan



-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: Thursday, August 17, 2006 4:44 PM
To: Wolstenholme, Daniel E
Cc: Jeremy Higdon; linux-ide@vger.kernel.org
Subject: Re: [PATCH] Add MSI support to sata_vsc driver

Wolstenholme, Daniel E wrote:
> Unfortunately, I'm stuck using Lookout for now. 

That's OK, this one needs another iteration anyway :)


> --------------
> diff -Naur linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c
> linux-2.6.18-rc1/drivers/scsi/sata_vsc.c
> --- linux-2.6.18-rc1-original/drivers/scsi/sata_vsc.c   2006-08-15
> 17:13:53.000000000 -0700
> +++ linux-2.6.18-rc1/drivers/scsi/sata_vsc.c    2006-08-17
> 14:29:27.000000000 -0700
> @@ -406,13 +406,17 @@
>          */
>         pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
> 
> +       if (pci_enable_msi && pci_enable_msi(pdev) == 0)

pci_enable_msi is not a variable, don't test it.  Just call it.

The function will always exist, even if conditionally implemented as a 
macro in the no-op case.


> +               probe_ent->irq_flags &= ~IRQF_SHARED;

We're doing an assignment (as you see below, from the code you 
modified), and the entire struct is memset(3) to zero, so there's no 
need to clear this flag.  Just assign it, for the non-MSI case.


> +               probe_ent->irq_flags = IRQF_SHARED;
> +
>         probe_ent->sht = &vsc_sata_sht;
>         probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
>                                 ATA_FLAG_MMIO;
>         probe_ent->port_ops = &vsc_sata_ops;
>         probe_ent->n_ports = 4;
>         probe_ent->irq = pdev->irq;
> -       probe_ent->irq_flags = IRQF_SHARED;
>         probe_ent->mmio_base = mmio_base;

[-- Attachment #2: diff.sata_vsc --]
[-- Type: application/octet-stream, Size: 834 bytes --]

diff -Naur -x .git linux-2.6.18-rc2/drivers/scsi/sata_vsc.c linux-2.6.18-rc2-MSI/drivers/scsi/sata_vsc.c
--- linux-2.6.18-rc2/drivers/scsi/sata_vsc.c	2006-08-15 17:13:53.000000000 -0700
+++ linux-2.6.18-rc2-MSI/drivers/scsi/sata_vsc.c	2006-08-18 10:42:52.000000000 -0700
@@ -406,13 +406,17 @@
 	 */
 	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
 
+	if (pci_enable_msi(pdev) == 0)
+		probe_ent->irq_flags = 0;
+	else
+		probe_ent->irq_flags = IRQF_SHARED;
+
 	probe_ent->sht = &vsc_sata_sht;
 	probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				ATA_FLAG_MMIO;
 	probe_ent->port_ops = &vsc_sata_ops;
 	probe_ent->n_ports = 4;
 	probe_ent->irq = pdev->irq;
-	probe_ent->irq_flags = IRQF_SHARED;
 	probe_ent->mmio_base = mmio_base;
 
 	/* We don't care much about the PIO/UDMA masks, but the core won't like us

Re: [PATCH] Add MSI support to sata_vsc driver

[Jeff Garzik]

two issues remain:

1) matching pci_disable_msi() is missing

2) as I mentioned previously, do not explicitly initialize irq_flags to 
zero, they are already zero via memset(3).  Only initialize irq_flags 
for the case where pci_enable_msi() fails.

RE: [PATCH] Add MSI support to sata_vsc driver

[Wolstenholme, Daniel E]

I noticed that several other drivers are calling "pci_intx(pdev, 0)"
when they enable MSI, but not all of them.  Is this something I should
also be doing?

Also, towards the end of the init_one function here, I noticed the
following:
	/* FIXME: check ata_device_add return value */
	ata_device_add(probe_ent);

In the sata_mv.c driver, this return value is checked, and if it's 0,
the driver exits with an error, and disables either msi or calls
pci_intx(pdev, 0) depending on if msi was set.  I checked the return
value of ata_device_add after disconnecting all my SATA drives, and I
still get a return value of 4 (4-port card), so it seems like this isn't
useful on this hardware.  Is ata_device_add supposed to return the
number of actual drives connected, or just the number of physical ports?

Thanks!
Dan

-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: Wednesday, August 23, 2006 11:56 PM
To: Wolstenholme, Daniel E
Cc: Jeremy Higdon; linux-ide@vger.kernel.org
Subject: Re: [PATCH] Add MSI support to sata_vsc driver

two issues remain:

1) matching pci_disable_msi() is missing

2) as I mentioned previously, do not explicitly initialize irq_flags to 
zero, they are already zero via memset(3).  Only initialize irq_flags 
for the case where pci_enable_msi() fails.

RE: [PATCH] Add MSI support to sata_vsc driver

[Wolstenholme, Daniel E]

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

Ok,
Here's my latest attempt.  I've added a new vsc_sata_host_priv data
structure (basically copied from the sata_mv driver) which has an
hp_flags bitfield with a bit to indicate whether MSI is being used.
This is used in the new vsc_sata_host_stop() routine to call
pci_disable_msi() if MSI is on, pci_intx() if not.

I'm still wondering if the pci_intx() stuff is needed; some SATA drivers
use it, some don't.  The intx disable bit was not set before when I had
MSI running, but it didn't seem to cause any problems, but it's still
probably good practice to set it when using MSI.  But is it needed when
in INTX mode?  Some drivers use it in the init_one() function, some
don't.

I've compiled this driver as a module and loaded and unloaded it to
verify that it indeed reuses the MSI interrupt instead of just
allocating a new one each time as it did with pci_disable_msi() missing.
However, one remaining problem (which I don't think has anything to do
with interrupts) is that when reloading the driver, it allocates all-new
port numbers:

ata5: SATA max UDMA/133 cmd 0x510A0200 ctl 0x510A0229 bmdma 0x510A0270
irq 131
ata6: SATA max UDMA/133 cmd 0x510A0400 ctl 0x510A0429 bmdma 0x510A0470
irq 131
ata7: SATA max UDMA/133 cmd 0x510A0600 ctl 0x510A0629 bmdma 0x510A0670
irq 131
ata8: SATA max UDMA/133 cmd 0x510A0800 ctl 0x510A0829 bmdma 0x510A0870
irq 131
scsi4 : sata_vsc
ata5: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata5.00: ATA-6, max UDMA/133, 72303840 sectors: LBA48
ata5.00: ata5: dev 0 multi count 0
ata5.00: applying bridge limits
ata5.00: configured for UDMA/100
scsi5 : sata_vsc
ata6: SATA link down (SStatus 0 SControl 300)
ATA: abnormal status 0x7F on port 0x510A041C
scsi6 : sata_vsc
ata7: SATA link down (SStatus 0 SControl 300)
ATA: abnormal status 0x7F on port 0x510A061C
scsi7 : sata_vsc
ata8: SATA link down (SStatus 0 SControl 300)
ATA: abnormal status 0x7F on port 0x510A081C
  Vendor: ATA       Model: WDC WD360GD-00FN  Rev: 35.0
  Type:   Direct-Access                      ANSI SCSI revision: 05

These were 1-4 when I first loaded the driver.  I've never seen this
before, as I've never used this driver as a module before.

Anyway, here's the new patch.

Thanks again for all the help!
Dan


-----Original Message-----
From: Jeff Garzik [mailto:jeff@garzik.org] 
Sent: Wednesday, August 23, 2006 11:56 PM
To: Wolstenholme, Daniel E
Cc: Jeremy Higdon; linux-ide@vger.kernel.org
Subject: Re: [PATCH] Add MSI support to sata_vsc driver

two issues remain:

1) matching pci_disable_msi() is missing

2) as I mentioned previously, do not explicitly initialize irq_flags to 
zero, they are already zero via memset(3).  Only initialize irq_flags 
for the case where pci_enable_msi() fails.

[-- Attachment #2: diff.sata_vsc.2 --]
[-- Type: application/octet-stream, Size: 3028 bytes --]

diff -Naur -x .git linux-2.6.18-rc2/drivers/scsi/sata_vsc.c linux-2.6.18-rc2-MSI/drivers/scsi/sata_vsc.c
--- linux-2.6.18-rc2/drivers/scsi/sata_vsc.c	2006-08-15 17:13:53.000000000 -0700
+++ linux-2.6.18-rc2-MSI/drivers/scsi/sata_vsc.c	2006-08-24 17:53:57.000000000 -0700
@@ -94,8 +94,14 @@
 			      VSC_SATA_INT_ERROR_P    | VSC_SATA_INT_ERROR_R | \
 			      VSC_SATA_INT_ERROR_E    | VSC_SATA_INT_ERROR_M | \
 			      VSC_SATA_INT_PHY_CHANGE),
+
+	/* Host private flags (hp_flags) */
+	VSC_SATA_HP_FLAG_MSI	= (1 << 0),
 };
 
+struct vsc_sata_host_priv {
+	u32			hp_flags;
+};
 
 #define is_vsc_sata_int_err(port_idx, int_status) \
 	 (int_status & (VSC_SATA_INT_ERROR << (8 * port_idx)))
@@ -118,6 +124,19 @@
 }
 
 
+static void vsc_sata_host_stop(struct ata_host_set *host_set)
+{
+	struct vsc_sata_host_priv *hpriv = host_set->private_data;
+	struct pci_dev *pdev = to_pci_dev(host_set->dev);
+
+	if (hpriv->hp_flags & VSC_SATA_HP_FLAG_MSI)
+		pci_disable_msi(pdev);
+	else
+		pci_intx(pdev, 0);
+	kfree (hpriv);
+	ata_pci_host_stop(host_set);
+}
+
 static void vsc_intr_mask_update(struct ata_port *ap, u8 ctl)
 {
 	void __iomem *mask_addr;
@@ -320,7 +339,7 @@
 	.scr_write		= vsc_sata_scr_write,
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
-	.host_stop		= ata_pci_host_stop,
+	.host_stop		= vsc_sata_host_stop,
 };
 
 static void __devinit vsc_sata_setup_port(struct ata_ioports *port, unsigned long base)
@@ -349,6 +368,7 @@
 {
 	static int printed_version;
 	struct ata_probe_ent *probe_ent = NULL;
+	struct vsc_sata_host_priv *hpriv;
 	unsigned long base;
 	int pci_dev_busy = 0;
 	void __iomem *mmio_base;
@@ -390,6 +410,7 @@
 		rc = -ENOMEM;
 		goto err_out_regions;
 	}
+
 	memset(probe_ent, 0, sizeof(*probe_ent));
 	probe_ent->dev = pci_dev_to_dev(pdev);
 	INIT_LIST_HEAD(&probe_ent->node);
@@ -401,19 +422,33 @@
 	}
 	base = (unsigned long) mmio_base;
 
+	hpriv = kmalloc(sizeof(*hpriv), GFP_KERNEL);
+	if (!hpriv) {
+		rc = -ENOMEM;
+		goto err_out_iounmap;
+	}
+	memset(hpriv, 0, sizeof(*hpriv));
+
 	/*
 	 * Due to a bug in the chip, the default cache line size can't be used
 	 */
 	pci_write_config_byte(pdev, PCI_CACHE_LINE_SIZE, 0x80);
 
+	if (pci_enable_msi(pdev) == 0) {
+		hpriv->hp_flags |= VSC_SATA_HP_FLAG_MSI;
+		pci_intx(pdev, 0);
+	}
+	else
+		probe_ent->irq_flags = IRQF_SHARED;
+
 	probe_ent->sht = &vsc_sata_sht;
 	probe_ent->host_flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
 				ATA_FLAG_MMIO;
 	probe_ent->port_ops = &vsc_sata_ops;
 	probe_ent->n_ports = 4;
 	probe_ent->irq = pdev->irq;
-	probe_ent->irq_flags = IRQF_SHARED;
 	probe_ent->mmio_base = mmio_base;
+	probe_ent->private_data = hpriv;
 
 	/* We don't care much about the PIO/UDMA masks, but the core won't like us
 	 * if we don't fill these
@@ -440,10 +475,12 @@
 
 	/* FIXME: check ata_device_add return value */
 	ata_device_add(probe_ent);
-	kfree(probe_ent);
 
+	kfree(probe_ent);
 	return 0;
 
+err_out_iounmap:
+	pci_iounmap(pdev, mmio_base);
 err_out_free_ent:
 	kfree(probe_ent);
 err_out_regions:

RE: [PATCH] Add MSI support to sata_vsc driver

[Wolstenholme, Daniel E]

Hi,

I'm just wondering if my latest patch was seen, and if there were any
more problems with it that I should address.

Thanks!
Dan