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

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

[Brendan McGrath]

From: Brendan McGrath <redmcg@redmandi.dyndns.org>

[media] saa7164: use an MSI interrupt when available

Fixes a known crash which most commonly occurs when multiple saa7164 
chips are in use.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
  drivers/media/pci/saa7164/saa7164-core.c | 34 
++++++++++++++++++++++++++++++--
  drivers/media/pci/saa7164/saa7164.h      |  1 +
  2 files changed, 33 insertions(+), 2 deletions(-)


This patch falls back to the original code - a 'shared' interrupt - when 
MSI is not available (or encounters an error).

Many of today's cards that use the saa7164 chip operate on a PCI-E bus 
(thus MSI should be available). Examples: the Hauppage HVR-2200 and HVR-2250

This enhancement also fixes an issue that was causing the driver to crash:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948

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.




diff --git a/drivers/media/pci/saa7164/saa7164-core.c 
b/drivers/media/pci/saa7164/saa7164-core.c
index 4b0bec3..083bea4 100644
--- a/drivers/media/pci/saa7164/saa7164-core.c
+++ b/drivers/media/pci/saa7164/saa7164-core.c
@@ -1230,8 +1230,33 @@ 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 */
+    err = pci_enable_msi(pci_dev);
+
+    if (!err) {
+        /* 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 (err) {
+        dev->msi = false;
+        /* if we have an error (i.e. we don't have an interrupt)
+             - fallback to legacy 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 +1466,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

Re: [PATCH] [media] saa7164: use an MSI interrupt when available

[Steven Toth]

> 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

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] saa7164: use an MSI interrupt when available

[Kyle Sanderson]

The patch allows the card to support using both internal tuners
without crashing out. I've been recording significantly since the
23rd, it just works now in comparison to constant maintenance.

I am under the impression it was against the spec to have a PCI-E card
without MSI support. Wouldn't the fallback code as well work in this
regard?

Tested-by: Kyle Sanderson <kyle.leet@gmail.com>

Thanks a ton for the patch,
Kyle.

On Thu, Feb 26, 2015 at 7:12 AM, Steven Toth <stoth@kernellabs.com> 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
>
> --
> Steven Toth - Kernel Labs
> http://www.kernellabs.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] [media] saa7164: use an MSI interrupt when available

[Steven Toth]

> I am under the impression it was against the spec to have a PCI-E card
> without MSI support. Wouldn't the fallback code as well work in this
> regard?

Not if the motherboard MSI implementation is flakey, like we've seen
in the past with other PCIe bridges and their interaction with root
controllers. I've actually seen this with some commercial customers
using the 7164. With this new patch MSI gets enabled, the 'solution'
doesn't work properly and now we expect users to compile their own
kernels, just to get back to a previously working solution.

I'm happy to take the patch if the option can be disabled.

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] [media] saa7164: use an MSI interrupt when available

[Kyle Sanderson]

This isn't my patch, however shouldn't these bridges already exist in
drivers/pci/quirks.c?

Kyle.

On Thu, Feb 26, 2015 at 12:45 PM, Steven Toth <stoth@kernellabs.com> wrote:
>> I am under the impression it was against the spec to have a PCI-E card
>> without MSI support. Wouldn't the fallback code as well work in this
>> regard?
>
> Not if the motherboard MSI implementation is flakey, like we've seen
> in the past with other PCIe bridges and their interaction with root
> controllers. I've actually seen this with some commercial customers
> using the 7164. With this new patch MSI gets enabled, the 'solution'
> doesn't work properly and now we expect users to compile their own
> kernels, just to get back to a previously working solution.
>
> I'm happy to take the patch if the option can be disabled.
>
> --
> Steven Toth - Kernel Labs
> http://www.kernellabs.com

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

[Brendan McGrath]

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
>

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

[Steven Toth]

On Thu, Feb 26, 2015 at 6:29 PM, Brendan McGrath
<redmcg@redmandi.dyndns.org> wrote:
> 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>

Thank you Brendan.

Reviewed-by: Steven Toth <stoth@kernellabs.com>

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

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

[Brendan McGrath]

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. A reported example can
be found here:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948

Reviewed-by: Steven Toth <stoth@kernellabs.com>
Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---

Changes since v3:
 * Added link to reported incident in comments
 * Added Reviewed-by tag from Steven Toth
 * Used git send-mail (as my patch got mangled using Thunderbird)
 * Added the linux-kernel mailing list to the recipient list

Note I have not included the Tested-by tag from Kyle Sanderson as his test was on v1 (which did not include the module option 'enable_msi') - although much of the code in v3 was there is v1.

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

Re: [PATCHv3] [media] saa7164: use an MSI interrupt when available

[catchall]

On 02/28/2015 04:14 PM, Brendan McGrath wrote:
> 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. A reported example can
> be found here:
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948
>
> Reviewed-by: Steven Toth <stoth@kernellabs.com>
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
>
I wanted to report that I have been running this patch for about a week 
now and I have had no instances of the zero free sequences issue.

Thank you very much!

David

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

[Brendan McGrath]

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 both digital tuners of the saa7164 chip is in use. A reported example can
be found here:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948

Reviewed-by: Steven Toth <stoth@kernellabs.com>
Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
Changes since v3:
  - fixes a conflict with a commit (3f845f3c4cf4) made to the media_tree after v3 was created (only the unified context has been changed)
  - corrected comments to reflect that the reported incident occured more commonly when multiple tuners were in use (not multiple saa7164 chips as previously stated)


 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 9cf3c6c..7635598 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);
@@ -1439,6 +1470,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;
+	}
+
 	pci_disable_device(pci_dev);
 
 	mutex_lock(&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

Re: [PATCHv4] [media] saa7164: use an MSI interrupt when available

[Mauro Carvalho Chehab]

Hi Brendan,

The idea is good, 
Some comments bellow.

Em Sat, 14 Mar 2015 14:27:39 +1100
Brendan McGrath <redmcg@redmandi.dyndns.org> escreveu:

> 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 both digital tuners of the saa7164 chip is in use. A reported example can
> be found here:
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948
> 
> Reviewed-by: Steven Toth <stoth@kernellabs.com>
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> Changes since v3:
>   - fixes a conflict with a commit (3f845f3c4cf4) made to the media_tree after v3 was created (only the unified context has been changed)
>   - corrected comments to reflect that the reported incident occured more commonly when multiple tuners were in use (not multiple saa7164 chips as previously stated)
> 
> 
>  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 9cf3c6c..7635598 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);

It is worth printing a warning about that.

> +
> +	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;
> +		}
> +	}

It would be better to join this if with the next one, in order
to make clear that both belong to the enable_msi logic. Something like:

static bool saa7164_enable_msi(struct device *pci_dev)
{
	if (!enable_msi)
		return false;

	err = pci_enable_msi(pci_dev);
	if (err) {
		printf(KERN_ERR "%s() Failed to enable MSI"
		       " Falling back to a shared IRQ\n", __func__);
		return false;
	}
	err = request_irq(pci_dev->irq, saa7164_irq, 0,
			  dev->name, dev);
	if (err) {
		printk(KERN_ERR "%s() Failed to get an MSI interrupt."
		       " Falling back to a shared IRQ\n", __func__);
		pci_disable_msi(pci_dev);
		return false;
	}
	return true;
}

Then, at the probe function, you could simply do:

	if (saa7164_enable_msi(pci_dev)) {
		dev->msi = true;
	} else {
		/* SOME_FALLBACK_CODE */
	}

The probe function is already complex enough. Breaking it into small
inlined functions makes easier to review. The removal of the if's
is an extra bonus, as the code size will likely be a little bit smaller.

> +
> +	if ((!enable_msi) || err) {
> +		dev->msi = false;

No need, as dev was initialized with kzalloc(), with zeroes all fields.

Also, you can simplify the "if" clause to:

	if (!dev->msi) {

That makes clearer that the code below is to be used when MSI is not
enabled or not initialized properly.

> +		/* 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);
> @@ -1439,6 +1470,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;
> +	}
> +
>  	pci_disable_device(pci_dev);
>  
>  	mutex_lock(&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;

Should be bool instead of u32.

>  
>  	struct cmd			cmds[SAA_CMD_MAX_MSG_UNITS];
>  	struct mutex			lock;

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

[Brendan McGrath]

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 both digital tuners of the saa7164 chip is in use. A
reported example can be found here:
http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948

Reviewed-by: Steven Toth <stoth@kernellabs.com>
Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
Changes since v4:
  - improved readability by taking on suggestions made by Mauro
  - the msi variable in the saa7164_dev structure is now a bool

Thanks Mauro - good suggestions and I think I've taken on board all of them.

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

diff --git a/drivers/media/pci/saa7164/saa7164-core.c b/drivers/media/pci/saa7164/saa7164-core.c
index 9cf3c6c..5e4a9f0 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);
@@ -1184,6 +1189,39 @@ static int saa7164_thread_function(void *data)
 	return 0;
 }
 
+static bool saa7164_enable_msi(struct pci_dev *pci_dev, struct saa7164_dev *dev)
+{
+	int err;
+
+	if (!enable_msi) {
+		printk(KERN_WARNING "%s() MSI disabled by module parameter 'enable_msi'"
+		       , __func__);
+		return false;
+	}
+
+	err = pci_enable_msi(pci_dev);
+
+	if (err) {
+		printk(KERN_ERR "%s() Failed to enable MSI interrupt."
+			" Falling back to a shared IRQ\n", __func__);
+		return false;
+	}
+
+	/* 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);
+		return false;
+	}
+
+	return true;
+}
+
 static int saa7164_initdev(struct pci_dev *pci_dev,
 			   const struct pci_device_id *pci_id)
 {
@@ -1230,13 +1268,22 @@ 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);
-	if (err < 0) {
-		printk(KERN_ERR "%s: can't get IRQ %d\n", dev->name,
-			pci_dev->irq);
-		err = -EIO;
-		goto fail_irq;
+	/* irq bit */
+	if (saa7164_enable_msi(pci_dev, dev)) {
+		dev->msi = true;
+	} else {
+		/* 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);
+			err = -EIO;
+			goto fail_irq;
+		}
 	}
 
 	pci_set_drvdata(pci_dev, dev);
@@ -1439,6 +1486,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;
+	}
+
 	pci_disable_device(pci_dev);
 
 	mutex_lock(&devlist);
diff --git a/drivers/media/pci/saa7164/saa7164.h b/drivers/media/pci/saa7164/saa7164.h
index cd1a07c..75a3f51 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;
+	bool msi;
 
 	struct cmd			cmds[SAA_CMD_MAX_MSG_UNITS];
 	struct mutex			lock;
-- 
1.9.1

Re: [PATCHv5] [media] saa7164: use an MSI interrupt when available

[Kyle Sanderson]

This has been plaguing users for years (there's a number of threads on
the Ubuntu board). I've been using revision 1 of the patch without
issue since early February. This is from having to constantly reboot
the system to flawless recording. If something has been outstanding
from Brendan, please let me know and I'll happily make the requested
changes.

Can we please merge this? There are at-least three consumers in this
thread alone that have confirmed this fixes the saa7164 driver for the
HVR-2250 device.
Kyle.

PS: I can't seem to source out who owns this in the MAINTAINERS file?

On Thu, Apr 9, 2015 at 11:39 PM, Brendan McGrath
<redmcg@redmandi.dyndns.org> wrote:
> 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 both digital tuners of the saa7164 chip is in use. A
> reported example can be found here:
> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948
>
> Reviewed-by: Steven Toth <stoth@kernellabs.com>
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
> Changes since v4:
>   - improved readability by taking on suggestions made by Mauro
>   - the msi variable in the saa7164_dev structure is now a bool
>
> Thanks Mauro - good suggestions and I think I've taken on board all of them.
>
>  drivers/media/pci/saa7164/saa7164-core.c | 66 ++++++++++++++++++++++++++++----
>  drivers/media/pci/saa7164/saa7164.h      |  1 +
>  2 files changed, 60 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/pci/saa7164/saa7164-core.c b/drivers/media/pci/saa7164/saa7164-core.c
> index 9cf3c6c..5e4a9f0 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);
> @@ -1184,6 +1189,39 @@ static int saa7164_thread_function(void *data)
>         return 0;
>  }
>
> +static bool saa7164_enable_msi(struct pci_dev *pci_dev, struct saa7164_dev *dev)
> +{
> +       int err;
> +
> +       if (!enable_msi) {
> +               printk(KERN_WARNING "%s() MSI disabled by module parameter 'enable_msi'"
> +                      , __func__);
> +               return false;
> +       }
> +
> +       err = pci_enable_msi(pci_dev);
> +
> +       if (err) {
> +               printk(KERN_ERR "%s() Failed to enable MSI interrupt."
> +                       " Falling back to a shared IRQ\n", __func__);
> +               return false;
> +       }
> +
> +       /* 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);
> +               return false;
> +       }
> +
> +       return true;
> +}
> +
>  static int saa7164_initdev(struct pci_dev *pci_dev,
>                            const struct pci_device_id *pci_id)
>  {
> @@ -1230,13 +1268,22 @@ 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);
> -       if (err < 0) {
> -               printk(KERN_ERR "%s: can't get IRQ %d\n", dev->name,
> -                       pci_dev->irq);
> -               err = -EIO;
> -               goto fail_irq;
> +       /* irq bit */
> +       if (saa7164_enable_msi(pci_dev, dev)) {
> +               dev->msi = true;
> +       } else {
> +               /* 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);
> +                       err = -EIO;
> +                       goto fail_irq;
> +               }
>         }
>
>         pci_set_drvdata(pci_dev, dev);
> @@ -1439,6 +1486,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;
> +       }
> +
>         pci_disable_device(pci_dev);
>
>         mutex_lock(&devlist);
> diff --git a/drivers/media/pci/saa7164/saa7164.h b/drivers/media/pci/saa7164/saa7164.h
> index cd1a07c..75a3f51 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;
> +       bool msi;
>
>         struct cmd                      cmds[SAA_CMD_MAX_MSG_UNITS];
>         struct mutex                    lock;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5] [media] saa7164: use an MSI interrupt when available

[Brendan McGrath]

Hi Kyle,

Great to hear you haven't had any problems since applying this patch! 
I'm looking forward to seeing it in the Linux master branch too.

Version 5 of the patch has been accepted and committed to the media tree 
by Mauro:
http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=77978089ddc90347644cc057e6b6cd169ac9abd4

I'm guessing it will therefore go in to the main Linux tree with the 
release of kernel version v4.2? (or it can be submitted as a fix for 
v4.1 - but I have no idea how a patch is selected on that criteria).

Hopefully someone can confirm or elaborate.

Regards,
Brendan McGrath


On 05/06/15 14:42, Kyle Sanderson wrote:
> This has been plaguing users for years (there's a number of threads on
> the Ubuntu board). I've been using revision 1 of the patch without
> issue since early February. This is from having to constantly reboot
> the system to flawless recording. If something has been outstanding
> from Brendan, please let me know and I'll happily make the requested
> changes.
>
> Can we please merge this? There are at-least three consumers in this
> thread alone that have confirmed this fixes the saa7164 driver for the
> HVR-2250 device.
> Kyle.
>
> PS: I can't seem to source out who owns this in the MAINTAINERS file?
>
> On Thu, Apr 9, 2015 at 11:39 PM, Brendan McGrath
> <redmcg@redmandi.dyndns.org> wrote:
>> 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 both digital tuners of the saa7164 chip is in use. A
>> reported example can be found here:
>> http://permalink.gmane.org/gmane.linux.drivers.video-input-infrastructure/83948
>>
>> Reviewed-by: Steven Toth <stoth@kernellabs.com>
>> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
>> ---
>> Changes since v4:
>>    - improved readability by taking on suggestions made by Mauro
>>    - the msi variable in the saa7164_dev structure is now a bool
>>
>> Thanks Mauro - good suggestions and I think I've taken on board all of them.
>>
>>   drivers/media/pci/saa7164/saa7164-core.c | 66 ++++++++++++++++++++++++++++----
>>   drivers/media/pci/saa7164/saa7164.h      |  1 +
>>   2 files changed, 60 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/pci/saa7164/saa7164-core.c b/drivers/media/pci/saa7164/saa7164-core.c
>> index 9cf3c6c..5e4a9f0 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);
>> @@ -1184,6 +1189,39 @@ static int saa7164_thread_function(void *data)
>>          return 0;
>>   }
>>
>> +static bool saa7164_enable_msi(struct pci_dev *pci_dev, struct saa7164_dev *dev)
>> +{
>> +       int err;
>> +
>> +       if (!enable_msi) {
>> +               printk(KERN_WARNING "%s() MSI disabled by module parameter 'enable_msi'"
>> +                      , __func__);
>> +               return false;
>> +       }
>> +
>> +       err = pci_enable_msi(pci_dev);
>> +
>> +       if (err) {
>> +               printk(KERN_ERR "%s() Failed to enable MSI interrupt."
>> +                       " Falling back to a shared IRQ\n", __func__);
>> +               return false;
>> +       }
>> +
>> +       /* 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);
>> +               return false;
>> +       }
>> +
>> +       return true;
>> +}
>> +
>>   static int saa7164_initdev(struct pci_dev *pci_dev,
>>                             const struct pci_device_id *pci_id)
>>   {
>> @@ -1230,13 +1268,22 @@ 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);
>> -       if (err < 0) {
>> -               printk(KERN_ERR "%s: can't get IRQ %d\n", dev->name,
>> -                       pci_dev->irq);
>> -               err = -EIO;
>> -               goto fail_irq;
>> +       /* irq bit */
>> +       if (saa7164_enable_msi(pci_dev, dev)) {
>> +               dev->msi = true;
>> +       } else {
>> +               /* 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);
>> +                       err = -EIO;
>> +                       goto fail_irq;
>> +               }
>>          }
>>
>>          pci_set_drvdata(pci_dev, dev);
>> @@ -1439,6 +1486,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;
>> +       }
>> +
>>          pci_disable_device(pci_dev);
>>
>>          mutex_lock(&devlist);
>> diff --git a/drivers/media/pci/saa7164/saa7164.h b/drivers/media/pci/saa7164/saa7164.h
>> index cd1a07c..75a3f51 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;
>> +       bool msi;
>>
>>          struct cmd                      cmds[SAA_CMD_MAX_MSG_UNITS];
>>          struct mutex                    lock;
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/