[PATCH v2 0/4] ISH Fixes for module loading

[PATCH v2 0/4] ISH Fixes for module loading

[Srinivas Pandruvada]

Two issues are reported related to loading of intel-ishtp-hid:

1. Loading failure after rmmod and then modprobe
(Patch 1/4 to Patch 3/4) address this. Here first two patches are just
clean up patches which gets used in Patch 3/4 to fix the issue.

2. Loading failure due to request_irq failure on some platforms
(Patch 4/4)


v2
 -pci_set_power_state() used instead of changing directly CSR

Even Xu (3):
  hid: intel-ish-hid: ipc: consolidate ish wake up operation
  hid: intel-ish-hid: ipc: Move DMA disable code to new function
  hid: intel-ish-hid: ipc: Fix driver reinit failure

Srinivas Pandruvada (1):
  hid: intel-ish-hid: request_irq failure

 drivers/hid/intel-ish-hid/ipc/ipc.c     | 102 +++++++++++++++++++++++---------
 drivers/hid/intel-ish-hid/ipc/pci-ish.c |   2 +-
 2 files changed, 74 insertions(+), 30 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] hid: intel-ish-hid: ipc: consolidate ish wake up operation

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

Same operations are done in ish_hw_start() and _ish_hw_reset() to
wakeup ISH device. Consolidate them by introducing a new function
ish_wakeup() and move the code there.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 45 +++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index e2517c1..d4c5721 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -638,6 +638,28 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id)
 }
 
 /**
+ * ish_wakeup() - wakeup ishfw from waiting-for-host state
+ * @dev: ishtp device pointer
+ *
+ * Set the dma enable bit and send a void message to FW,
+ * it wil wakeup FW from waiting-for-host state.
+ */
+static void ish_wakeup(struct ishtp_device *dev)
+{
+	/* Set dma enable bit */
+	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
+
+	/*
+	 * Send 0 IPC message so that ISH FW wakes up if it was already
+	 * asleep.
+	 */
+	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
+
+	/* Flush writes to doorbell and REMAP2 */
+	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+}
+
+/**
  * _ish_hw_reset() - HW reset
  * @dev: ishtp device pointer
  *
@@ -690,16 +712,8 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 	csr |= PCI_D0;
 	pci_write_config_word(pdev, pdev->pm_cap + PCI_PM_CTRL, csr);
 
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
-
-	/*
-	 * Send 0 IPC message so that ISH FW wakes up if it was already
-	 * asleep
-	 */
-	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
-
-	/* Flush writes to doorbell and REMAP2 */
-	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+	/* Now we can enable ISH DMA operation and wakeup ISHFW */
+	ish_wakeup(dev);
 
 	return	0;
 }
@@ -758,16 +772,9 @@ static int _ish_ipc_reset(struct ishtp_device *dev)
 int ish_hw_start(struct ishtp_device *dev)
 {
 	ish_set_host_rdy(dev);
-	/* After that we can enable ISH DMA operation */
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, IPC_RMP2_DMA_ENABLED);
 
-	/*
-	 * Send 0 IPC message so that ISH FW wakes up if it was already
-	 * asleep
-	 */
-	ish_reg_write(dev, IPC_REG_HOST2ISH_DRBL, IPC_DRBL_BUSY_BIT);
-	/* Flush write to doorbell */
-	ish_reg_read(dev, IPC_REG_ISH_HOST_FWSTS);
+	/* After that we can enable ISH DMA operation and wakeup ISHFW */
+	ish_wakeup(dev);
 
 	set_host_ready(dev);
 
-- 
2.7.4

[PATCH v2 2/4] hid: intel-ish-hid: ipc: Move DMA disable code to new function

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

Add a new function ish_disable_dma() and move DMA disable operations
here, so that this functionality can be reused.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 42 ++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index d4c5721..0e0dfa6 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -638,6 +638,36 @@ irqreturn_t ish_irq_handler(int irq, void *dev_id)
 }
 
 /**
+ * ish_disable_dma() - disable dma communication between host and ISHFW
+ * @dev: ishtp device pointer
+ *
+ * Clear the dma enable bit and wait for dma inactive.
+ *
+ * Return: 0 for success else error code.
+ */
+static int ish_disable_dma(struct ishtp_device *dev)
+{
+	unsigned int	dma_delay;
+
+	/* Clear the dma enable bit */
+	ish_reg_write(dev, IPC_REG_ISH_RMP2, 0);
+
+	/* wait for dma inactive */
+	for (dma_delay = 0; dma_delay < MAX_DMA_DELAY &&
+		_ish_read_fw_sts_reg(dev) & (IPC_ISH_IN_DMA);
+		dma_delay += 5)
+		mdelay(5);
+
+	if (dma_delay >= MAX_DMA_DELAY) {
+		dev_err(dev->devc,
+			"Wait for DMA inactive timeout\n");
+		return	-EBUSY;
+	}
+
+	return 0;
+}
+
+/**
  * ish_wakeup() - wakeup ishfw from waiting-for-host state
  * @dev: ishtp device pointer
  *
@@ -671,7 +701,6 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 {
 	struct pci_dev *pdev = dev->pdev;
 	int	rv;
-	unsigned int	dma_delay;
 	uint16_t csr;
 
 	if (!pdev)
@@ -686,15 +715,8 @@ static int _ish_hw_reset(struct ishtp_device *dev)
 		return	-EINVAL;
 	}
 
-	/* Now trigger reset to FW */
-	ish_reg_write(dev, IPC_REG_ISH_RMP2, 0);
-
-	for (dma_delay = 0; dma_delay < MAX_DMA_DELAY &&
-		_ish_read_fw_sts_reg(dev) & (IPC_ISH_IN_DMA);
-		dma_delay += 5)
-		mdelay(5);
-
-	if (dma_delay >= MAX_DMA_DELAY) {
+	/* Disable dma communication between FW and host */
+	if (ish_disable_dma(dev)) {
 		dev_err(&pdev->dev,
 			"Can't reset - stuck with DMA in-progress\n");
 		return	-EBUSY;
-- 
2.7.4

[PATCH v2 3/4] hid: intel-ish-hid: ipc: Fix driver reinit failure

[Srinivas Pandruvada]

From: Even Xu <even.xu@intel.com>

When built as a module, modprobe followed by rmmod can fail because
DMA was still active. So to fix this, DMA needs to be disabled during
module exit.

This change disables DMA during modules exit and change the ISH PCI
device status to D3.

Signed-off-by: Even Xu <even.xu@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/ipc.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 0e0dfa6..0c9ac4d 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -905,6 +905,21 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
  */
 void	ish_device_disable(struct ishtp_device *dev)
 {
+	struct pci_dev *pdev = dev->pdev;
+
+	if (!pdev)
+		return;
+
+	/* Disable dma communication between FW and host */
+	if (ish_disable_dma(dev)) {
+		dev_err(&pdev->dev,
+			"Can't reset - stuck with DMA in-progress\n");
+		return;
+	}
+
+	/* Put ISH to D3hot state for power saving */
+	pci_set_power_state(pdev, PCI_D3hot);
+
 	dev->dev_state = ISHTP_DEV_DISABLED;
 	ish_clr_host_rdy(dev);
 }
-- 
2.7.4

[PATCH v2 4/4] hid: intel-ish-hid: request_irq failure

[Srinivas Pandruvada]

On some platforms ISH interrupt is shared, which causes request_irq to
fail. This requires IRQF_SHARED irq flag.

But IRQF_NO_SUSPEND and IRQF_SHARED should not be used together, so
removed IRQF_NO_SUSPEND flag. Anyway this driver doesn't require
IRQF_NO_SUSPEND, as this interrupt is not required during "noirq" phases
of suspending and resuming devices as well as during the time when
nonboot CPUs are taken offline and brought back online.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ipc/pci-ish.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
index 42f0bee..6fda0f9 100644
--- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
+++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
@@ -146,7 +146,7 @@ static int ish_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	pdev->dev_flags |= PCI_DEV_FLAGS_NO_D3;
 
 	/* request and enable interrupt */
-	ret = request_irq(pdev->irq, ish_irq_handler, IRQF_NO_SUSPEND,
+	ret = request_irq(pdev->irq, ish_irq_handler, IRQF_SHARED,
 			  KBUILD_MODNAME, dev);
 	if (ret) {
 		dev_err(&pdev->dev, "ISH: request IRQ failure (%d)\n",
-- 
2.7.4

Re: [PATCH v2 0/4] ISH Fixes for module loading

[Jiri Kosina]

On Fri, 21 Oct 2016, Srinivas Pandruvada wrote:

> Two issues are reported related to loading of intel-ishtp-hid:
> 
> 1. Loading failure after rmmod and then modprobe
> (Patch 1/4 to Patch 3/4) address this. Here first two patches are just
> clean up patches which gets used in Patch 3/4 to fix the issue.
> 
> 2. Loading failure due to request_irq failure on some platforms
> (Patch 4/4)

This actually looks like 4.9-rc material ... could you please confirm that 
you agree with that, or you'd rather have this for the next merge window?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2 0/4] ISH Fixes for module loading

[Srinivas Pandruvada]

On Thu, 2016-11-03 at 12:05 -0600, Jiri Kosina wrote:
> On Fri, 21 Oct 2016, Srinivas Pandruvada wrote:
> 
> > Two issues are reported related to loading of intel-ishtp-hid:
> > 
> > 1. Loading failure after rmmod and then modprobe
> > (Patch 1/4 to Patch 3/4) address this. Here first two patches are
> just
> > clean up patches which gets used in Patch 3/4 to fix the issue.
> > 
> > 2. Loading failure due to request_irq failure on some platforms
> > (Patch 4/4)
> 
> This actually looks like 4.9-rc material ... could you please confirm
> that 
> you agree with that, or you'd rather have this for the next merge
> window?
I am fine with 4.9-rc.

Thanks,
Srinivas

> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs
> 

Re: [PATCH v2 0/4] ISH Fixes for module loading

[Jiri Kosina]

On Fri, 21 Oct 2016, Srinivas Pandruvada wrote:

> Two issues are reported related to loading of intel-ishtp-hid:
> 
> 1. Loading failure after rmmod and then modprobe
> (Patch 1/4 to Patch 3/4) address this. Here first two patches are just
> clean up patches which gets used in Patch 3/4 to fix the issue.
> 
> 2. Loading failure due to request_irq failure on some platforms
> (Patch 4/4)
> 
> 
> v2
>  -pci_set_power_state() used instead of changing directly CSR

Applied to hid.git#for-4.9/upstream-fixes

-- 
Jiri Kosina
SUSE Labs