[PATCHv2] [media] saa7164: use an MSI interrupt when available

[Brendan McGrath <redmcg@redmandi.dyndns.org>]

From: Brendan McGrath <redmcg@redmandi.dyndns.org>
[media] saa7164: use an MSI interrupt when available

Enhances driver to use an MSI interrupt when available.

Adds the module option 'enable_msi' (type bool) which by default is 
enabled. Can be set to 'N' to disable.

Fixes (or can reduce the occurrence of) a crash which is most commonly 
reported when multiple saa7164 chips are in use.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---

Thanks for the feedback Steven (and thanks for all your hard work on 
this driver!).

And thank-you Kyle for taking the time to test this patch. Much appreciated.


  drivers/media/pci/saa7164/saa7164-core.c | 40 
++++++++++++++++++++++++++++++--
  drivers/media/pci/saa7164/saa7164.h      |  1 +
  2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/saa7164/saa7164-core.c 
b/drivers/media/pci/saa7164/saa7164-core.c
index 4b0bec3..1aff2ee 100644
--- a/drivers/media/pci/saa7164/saa7164-core.c
+++ b/drivers/media/pci/saa7164/saa7164-core.c
@@ -85,6 +85,11 @@ module_param(guard_checking, int, 0644);
  MODULE_PARM_DESC(guard_checking,
     "enable dma sanity checking for buffer overruns");

+static bool enable_msi = true;
+module_param(enable_msi, bool, 0444);
+MODULE_PARM_DESC(enable_msi,
+     "enable the use of an msi interrupt if available");
+
  static unsigned int saa7164_devcount;

  static DEFINE_MUTEX(devlist);
@@ -1230,8 +1235,34 @@ static int saa7164_initdev(struct pci_dev *pci_dev,
         goto fail_irq;
     }

-    err = request_irq(pci_dev->irq, saa7164_irq,
-        IRQF_SHARED, dev->name, dev);
+    /* irq bit */
+    if (enable_msi)
+        err = pci_enable_msi(pci_dev);
+
+    if (!err && enable_msi) {
+        /* no error - so request an msi interrupt */
+        err = request_irq(pci_dev->irq, saa7164_irq, 0,
+                  dev->name, dev);
+
+        if (err) {
+            /* fall back to legacy interrupt */
+            printk(KERN_ERR "%s() Failed to get an MSI interrupt."
+                " Falling back to a shared IRQ\n", __func__);
+            pci_disable_msi(pci_dev);
+        } else {
+            dev->msi = true;
+        }
+    }
+
+    if ((!enable_msi) || err) {
+        dev->msi = false;
+        /* if we have an error (i.e. we don't have an interrupt)
+             or msi is not enabled - fallback to shared interrupt */
+
+        err = request_irq(pci_dev->irq, saa7164_irq,
+                    IRQF_SHARED, dev->name, dev);
+    }
+
     if (err < 0) {
         printk(KERN_ERR "%s: can't get IRQ %d\n", dev->name,
             pci_dev->irq);
@@ -1441,6 +1472,11 @@ static void saa7164_finidev(struct pci_dev *pci_dev)
     /* unregister stuff */
     free_irq(pci_dev->irq, dev);

+    if (dev->msi) {
+        pci_disable_msi(pci_dev);
+        dev->msi = false;
+    }
+
     mutex_lock(&devlist);
     list_del(&dev->devlist);
     mutex_unlock(&devlist);
diff --git a/drivers/media/pci/saa7164/saa7164.h 
b/drivers/media/pci/saa7164/saa7164.h
index cd1a07c..6df4b252 100644
--- a/drivers/media/pci/saa7164/saa7164.h
+++ b/drivers/media/pci/saa7164/saa7164.h
@@ -459,6 +459,7 @@ struct saa7164_dev {
     /* Interrupt status and ack registers */
     u32 int_status;
     u32 int_ack;
+    u32 msi;

     struct cmd            cmds[SAA_CMD_MAX_MSG_UNITS];
     struct mutex            lock;
--
1.9.1



On 27/02/15 02:12, Steven Toth wrote:
>> I believe the root cause of the crash is due to a DMA/IRQ race condition. It
>> most commonly occurs when the saa7164 driver is dealing with more than one
>> saa7164 chip (the HVR-2200 and HVR-2250 for example have two - one for each
>> tuner). Given MSI avoids DMA/IRQ race conditions - this would explain why
>> the patch works as a fix.
> Brendan, thanks.
>
> With MSI I've had some people report complete success, others still
> have the issues.
>
> In my experience this does help with i2c timeout issues but not
> completely in every case. I've also seen it with single card instances
> so you descripton above is close - but not quiet accurate in all
> cases.
>
> While I'm generally OK with changing the driver behaviour to enable
> MSI by default, please add a module option to allow the behaviour to
> be disabled, reverting the driver back to existing behaviour.
>
> Once this is done, I'll be happy to Ack it.
>
> Thanks again.
>
> - Steve
>