Re: [PATCH 2/2] firewire: ohci: add MSI support

Re: [PATCH 2/2] firewire: ohci: add MSI support

[Stefan Richter]

(adding Cc: lkml, quoting in full)

Clemens Ladisch wrote:
> This patch adds support for message-signaled interrupts.
> 
> Any native PCI-Express OHCI controller should support MSI, but most are
> just PCI cores behind a PCI-E/PCI bridge.  The only chips that are known
> to claim to support MSI are the Lucent/Agere/LSI FW643 and the VIA
> VT6315, none of which I have been able to test.

I got a FW643 card a few days ago and can test that one.  Also, I have a
JMicron JMB381 native PCIe 1394a controller.  The latter is a less ideal
test specimen since it is buggy already without MSI (soon stops to
operate if one dares to mix isochronous and asynchronous I/O; sometimes
also if there is a bus reset at an inconvenient time).

>From lspci:

04:00.0 FireWire (IEEE 1394): Agere Systems FW643 PCI Express1394b
Controller (PHY/Link) (rev 07) (prog-if 10 [OHCI])

        [...]
        Capabilities: [4c] MSI: Enable- Count=1/1 Maskable- 64bit+

                Address: 0000000000000000  Data: 0000

        Capabilities: [60] Express (v1) Endpoint, MSI 00

        [...]

0a:00.0 FireWire (IEEE 1394): JMicron Technology Corp. IEEE 1394 Host
Controller (prog-if 10 [OHCI])

        [...]
        Capabilities: [80] Express (v1) Endpoint, MSI 00

        [...]
        Capabilities: [94] MSI: Enable- Count=1/1 Maskable- 64bit-

                Address: fffffffc  Data: 0000

        [...]

Does "MSI 00", "MSI: Enable-" mean they do or don't support MSI?  Asks a
PCIe newbie.

The FW643 (actually two of them) sits behind a PCIe switch:

03:04.0 PCI bridge: PLX Technology, Inc. PEX 8505 5-lane, 5-port PCI
Express Switch (rev aa) (prog-if 00 [Normal decode])

        [...]
        Capabilities: [48] MSI: Enable+ Count=1/2 Maskable+ 64bit+

                Address: 00000000fee0f00c  Data: 41a1

                Masking: 00000003  Pending: 00000000

        Capabilities: [68] Express (v1) Downstream Port (Slot+), MSI 00

        [...]

I hope this switch does not make things any more interesting.

> Due to the high level of trust I have in the competence of these and any
> future chip makers, I thought it a good idea to add a disable-MSI quirk.
> 
> Signed-off-by: Clemens Ladisch <clemens@ladisch.de>
> ---
>  drivers/firewire/ohci.c |   13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> --- a/drivers/firewire/ohci.c
> +++ b/drivers/firewire/ohci.c
> @@ -237,6 +237,7 @@ static char ohci_driver_name[] = KBUILD_
>  #define QUIRK_RESET_PACKET		2
>  #define QUIRK_BE_HEADERS		4
>  #define QUIRK_NO_1394A			8
> +#define QUIRK_NO_MSI			16
>  
>  /* In case of multiple matches in ohci_quirks[], only the first one is used. */
>  static const struct {
> @@ -260,6 +261,7 @@ MODULE_PARM_DESC(quirks, "Chip quirks (d
>  	", reset packet generation = "	__stringify(QUIRK_RESET_PACKET)
>  	", AR/selfID endianess = "	__stringify(QUIRK_BE_HEADERS)
>  	", no 1394a enhancements = "	__stringify(QUIRK_NO_1394A)
> +	", disable MSI = "		__stringify(QUIRK_NO_MSI)
>  	")");
>  
>  #define OHCI_PARAM_DEBUG_AT_AR		1
> @@ -1718,10 +1720,13 @@ static int ohci_enable(struct fw_card *c
>  
>  	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
>  
> +	if (!(ohci->quirks & QUIRK_NO_MSI))
> +		pci_enable_msi(dev);
>  	if (request_irq(dev->irq, irq_handler,
> -			IRQF_SHARED, ohci_driver_name, ohci)) {
> +			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
> +			ohci_driver_name, ohci)) {
> -		fw_error("Failed to allocate shared interrupt %d.\n",
> -			 dev->irq);
> +		fw_error("Failed to allocate interrupt %d.\n", dev->irq);
> +		pci_disable_msi(dev);
>  		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
>  				  ohci->config_rom, ohci->config_rom_bus);
>  		return -EIO;
> @@ -2625,6 +2630,7 @@ static void pci_remove(struct pci_dev *d
>  	context_release(&ohci->at_response_ctx);
>  	kfree(ohci->it_context_list);
>  	kfree(ohci->ir_context_list);
> +	pci_disable_msi(dev);
>  	pci_iounmap(dev, ohci->registers);
>  	pci_release_region(dev, 0);
>  	pci_disable_device(dev);
> @@ -2642,6 +2648,7 @@ static int pci_suspend(struct pci_dev *d
>  
>  	software_reset(ohci);
>  	free_irq(dev->irq, ohci);
> +	pci_disable_msi(dev);
>  	err = pci_save_state(dev);
>  	if (err) {
>  		fw_error("pci_save_state failed\n");

Looks right, but what do I know?
-- 
Stefan Richter
-=====-==-=- -=-- ==-=-
http://arcgraph.de/sr/

Re: [PATCH 2/2] firewire: ohci: add MSI support

[Stefan Richter]

Stefan Richter wrote:
> Does "MSI 00", "MSI: Enable-" mean they do or don't support MSI?  Asks a
> PCIe newbie.

OK, an online encyclopedia informs me that MSI or MSI-X is required by PCIe.
-- 
Stefan Richter
-=====-==-=- -=-- ==-=-
http://arcgraph.de/sr/

Re: [PATCH 2/2] firewire: ohci: add MSI support

[Clemens Ladisch]

Stefan Richter wrote:
> Clemens Ladisch wrote:
> > This patch adds support for message-signaled interrupts.
> > 
> > Any native PCI-Express OHCI controller should support MSI, but most are
> > just PCI cores behind a PCI-E/PCI bridge.  The only chips that are known
> > to claim to support MSI are the Lucent/Agere/LSI FW643 and the VIA
> > VT6315, none of which I have been able to test.
> 
> I got a FW643 card a few days ago and can test that one.  Also, I have a
> JMicron JMB381 native PCIe 1394a controller.  The latter is a less ideal
> test specimen since it is buggy already without MSI (soon stops to
> operate if one dares to mix isochronous and asynchronous I/O; sometimes
> also if there is a bus reset at an inconvenient time).

Problematic MSI implementations usually deliver no interrupt or forget
to disable the INTx interrupt (so that the interrupt never gets
deasserted); these bugs should be visible immediately.

> From lspci:
> 
> 04:00.0 FireWire (IEEE 1394): Agere Systems FW643 PCI Express1394b Controller (PHY/Link) (rev 07) (prog-if 10 [OHCI])
>         Capabilities: [4c] MSI: Enable- Count=1/1 Maskable- 64bit+
> 
> 0a:00.0 FireWire (IEEE 1394): JMicron Technology Corp. IEEE 1394 Host Controller (prog-if 10 [OHCI])
>         Capabilities: [94] MSI: Enable- Count=1/1 Maskable- 64bit-
> 
> Does "MSI 00", "MSI: Enable-" mean they do or don't support MSI?  Asks a
> PCIe newbie.

The "MSI: Enable-" means that it's supported but not enabled at the
moment.

> OK, an online encyclopedia informs me that MSI or MSI-X is required by PCIe.

That requirement doesn't help us if there is an old PCI chip behind a
PCIe/PCI bridge (even if only virtually inside one chip, such as the TI
XIO2xxx).

> The FW643 (actually two of them) sits behind a PCIe switch:
> 
> 03:04.0 PCI bridge: PLX Technology, Inc. PEX 8505 5-lane, 5-port PCI Express Switch (rev aa) (prog-if 00 [Normal decode])
>         Capabilities: [48] MSI: Enable+ Count=1/2 Maskable+ 64bit+
> 
> I hope this switch does not make things any more interesting.

Interrupt messages should be passed through like just any other messages
(such as memory or I/O reads or writes), so it shouldn't be able to
introduce MSI-related problems.

(The PEX8505's MSI registers are for interrupts that come from the
switch itself, probably for power management or error reporting; have
a look into /proc/interrupts.)


Regards,
Clemens

Re: [PATCH 2/2] firewire: ohci: add MSI support

[Roland Dreier]

 > Does "MSI 00", "MSI: Enable-" mean they do or don't support MSI?  Asks a
 > PCIe newbie.

Yes, the fact that you see an MSI capability structure at all in the
lspci output says that the device supports MSI.  The "Enable-" means
that that MSI has not been enabled.
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

[PATCH update] firewire: ohci: add MSI support

[Stefan Richter]

From: Clemens Ladisch <clemens@ladisch.de>

This patch adds support for message-signaled interrupts.

Any native PCI-Express OHCI controller should support MSI, but most are
just PCI cores behind a PCI-E/PCI bridge.  The only chips that are known
to claim to support MSI are the Lucent/Agere/LSI FW643 and the VIA
VT6315, none of which I have been able to test.

Due to the high level of trust I have in the competence of these and any
future chip makers, I thought it a good idea to add a disable-MSI quirk.

Signed-off-by: Clemens Ladisch <clemens@ladisch.de>

Tested Agere FW643 rev 07 [11c1:5901] and JMicron JMB381 [197b:2380].
Added a quirks list entry for the one of them which kept its count of
MSI events consistently at zero.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/ohci.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Index: b/drivers/firewire/ohci.c
===================================================================
--- a/drivers/firewire/ohci.c
+++ b/drivers/firewire/ohci.c
@@ -231,12 +231,14 @@ static inline struct fw_ohci *fw_ohci(st
 
 static char ohci_driver_name[] = KBUILD_MODNAME;
 
+#define PCI_DEVICE_ID_JMICRON_JMB38X_FW	0x2380
 #define PCI_DEVICE_ID_TI_TSB12LV22	0x8009
 
 #define QUIRK_CYCLE_TIMER		1
 #define QUIRK_RESET_PACKET		2
 #define QUIRK_BE_HEADERS		4
 #define QUIRK_NO_1394A			8
+#define QUIRK_NO_MSI			16
 
 /* In case of multiple matches in ohci_quirks[], only the first one is used. */
 static const struct {
@@ -247,6 +249,7 @@ static const struct {
 							    QUIRK_NO_1394A},
 	{PCI_VENDOR_ID_TI,	PCI_ANY_ID,	QUIRK_RESET_PACKET},
 	{PCI_VENDOR_ID_AL,	PCI_ANY_ID,	QUIRK_CYCLE_TIMER},
+	{PCI_VENDOR_ID_JMICRON,	PCI_DEVICE_ID_JMICRON_JMB38X_FW, QUIRK_NO_MSI},
 	{PCI_VENDOR_ID_NEC,	PCI_ANY_ID,	QUIRK_CYCLE_TIMER},
 	{PCI_VENDOR_ID_VIA,	PCI_ANY_ID,	QUIRK_CYCLE_TIMER},
 	{PCI_VENDOR_ID_APPLE,	PCI_DEVICE_ID_APPLE_UNI_N_FW, QUIRK_BE_HEADERS},
@@ -260,6 +263,7 @@ MODULE_PARM_DESC(quirks, "Chip quirks (d
 	", reset packet generation = "	__stringify(QUIRK_RESET_PACKET)
 	", AR/selfID endianess = "	__stringify(QUIRK_BE_HEADERS)
 	", no 1394a enhancements = "	__stringify(QUIRK_NO_1394A)
+	", disable MSI = "		__stringify(QUIRK_NO_MSI)
 	")");
 
 #define OHCI_PARAM_DEBUG_AT_AR		1
@@ -1709,10 +1713,13 @@ static int ohci_enable(struct fw_card *c
 
 	reg_write(ohci, OHCI1394_AsReqFilterHiSet, 0x80000000);
 
+	if (!(ohci->quirks & QUIRK_NO_MSI))
+		pci_enable_msi(dev);
 	if (request_irq(dev->irq, irq_handler,
-			IRQF_SHARED, ohci_driver_name, ohci)) {
-		fw_error("Failed to allocate shared interrupt %d.\n",
-			 dev->irq);
+			pci_dev_msi_enabled(dev) ? 0 : IRQF_SHARED,
+			ohci_driver_name, ohci)) {
+		fw_error("Failed to allocate interrupt %d.\n", dev->irq);
+		pci_disable_msi(dev);
 		dma_free_coherent(ohci->card.device, CONFIG_ROM_SIZE,
 				  ohci->config_rom, ohci->config_rom_bus);
 		return -EIO;
@@ -2627,6 +2634,7 @@ static void pci_remove(struct pci_dev *d
 	context_release(&ohci->at_response_ctx);
 	kfree(ohci->it_context_list);
 	kfree(ohci->ir_context_list);
+	pci_disable_msi(dev);
 	pci_iounmap(dev, ohci->registers);
 	pci_release_region(dev, 0);
 	pci_disable_device(dev);
@@ -2644,6 +2652,7 @@ static int pci_suspend(struct pci_dev *d
 
 	software_reset(ohci);
 	free_irq(dev->irq, ohci);
+	pci_disable_msi(dev);
 	err = pci_save_state(dev);
 	if (err) {
 		fw_error("pci_save_state failed\n");

-- 
Stefan Richter
-=====-==-=- -==- --=-=
http://arcgraph.de/sr/