[PATCH 0/3] Some fixes to aacraid

[PATCH 0/3] Some fixes to aacraid

[Guilherme G. Piccoli]

This series presents 3 small fixes for aacraid driver.
The most important is the crash prevention, IMHO.

Tested them against v4.14.

Guilherme G. Piccoli (3):
  scsi: aacraid: Check for PCI state of device in a generic way
  scsi: aacraid: Perform initialization reset only once
  scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path

 drivers/scsi/aacraid/aacraid.h |  1 +
 drivers/scsi/aacraid/commsup.c | 35 +++--------------------------------
 drivers/scsi/aacraid/linit.c   |  3 +++
 drivers/scsi/aacraid/rx.c      | 15 ++++++++++-----
 drivers/scsi/aacraid/src.c     | 20 ++++++++++++++------
 5 files changed, 31 insertions(+), 43 deletions(-)

-- 
2.15.0

[PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way

[Guilherme G. Piccoli]

Commit 16ae9dd35d37 ("scsi: aacraid: Fix for excessive prints on EEH")
introduced checks about the state of device before any PCI operations
in the driver. Basically, this prevents it to perform PCI accesses
when device is in the process of recover from a PCI error. In PowerPC,
such mechanism is called EEH, and the aforementioned commit introduced
checks that are based on EEH-specific primitives for that.

The potential problems with this approach are three: first, these checks
are "locked" to powerpc only - another archs could have error recovery
methods too, like AER in Intel. Also, the powerpc primitives perform
expensive FW accesses to validate the precise PCI state of a device.
Finally, code becomes more complicated and needs ifdef validation
based on arch config being set.

So, this patch makes use of generic PCI state checks, which are
lightweight and non-dependent of arch configs - also, it makes
the code cleaner.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/scsi/aacraid/commsup.c | 33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 525a652dab48..2abe8fd83494 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -467,35 +467,6 @@ int aac_queue_get(struct aac_dev * dev, u32 * index, u32 qid, struct hw_fib * hw
 	return 0;
 }
 
-#ifdef CONFIG_EEH
-static inline int aac_check_eeh_failure(struct aac_dev *dev)
-{
-	/* Check for an EEH failure for the given
-	 * device node. Function eeh_dev_check_failure()
-	 * returns 0 if there has not been an EEH error
-	 * otherwise returns a non-zero value.
-	 *
-	 * Need to be called before any PCI operation,
-	 * i.e.,before aac_adapter_check_health()
-	 */
-	struct eeh_dev *edev = pci_dev_to_eeh_dev(dev->pdev);
-
-	if (eeh_dev_check_failure(edev)) {
-		/* The EEH mechanisms will handle this
-		 * error and reset the device if
-		 * necessary.
-		 */
-		return 1;
-	}
-	return 0;
-}
-#else
-static inline int aac_check_eeh_failure(struct aac_dev *dev)
-{
-	return 0;
-}
-#endif
-
 /*
  *	Define the highest level of host to adapter communication routines.
  *	These routines will support host to adapter FS commuication. These
@@ -701,7 +672,7 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
 					return -ETIMEDOUT;
 				}
 
-				if (aac_check_eeh_failure(dev))
+				if (unlikely(pci_channel_offline(dev->pdev)))
 					return -EFAULT;
 
 				if ((blink = aac_adapter_check_health(dev)) > 0) {
@@ -801,7 +772,7 @@ int aac_hba_send(u8 command, struct fib *fibptr, fib_callback callback,
 
 		spin_unlock_irqrestore(&fibptr->event_lock, flags);
 
-		if (aac_check_eeh_failure(dev))
+		if (unlikely(pci_channel_offline(dev->pdev)))
 			return -EFAULT;
 
 		fibptr->flags |= FIB_CONTEXT_FLAG_WAIT;
-- 
2.15.0

[PATCH 2/3] scsi: aacraid: Perform initialization reset only once

[Guilherme G. Piccoli]

Currently the driver accepts two ways of requesting an initialization
reset on the adapter: by passing aac_reset_devices module parameter,
or the generic kernel parameter reset_devices.

It's working as intended...but if we end up reaching a scsi hang and
the scsi EH mechanism takes place, aacraid performs resets as part of
the scsi error recovery procedure. These EH routines might reinitialize
the device, and if we have provided some of the reset parameters in the
kernel command-line, we again perform an "initialization" reset.

So, to avoid this duplication of resets in case of scsi EH path, this
patch adds a field to aac_dev struct to keep per-adapter track of the
init reset request - once it's done, we set it to false and don't
proactively reset anymore in case of reinitializations.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/scsi/aacraid/aacraid.h |  1 +
 drivers/scsi/aacraid/linit.c   |  3 +++
 drivers/scsi/aacraid/rx.c      | 15 ++++++++++-----
 drivers/scsi/aacraid/src.c     | 20 ++++++++++++++------
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 403a639574e5..6e3d81969a77 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1673,6 +1673,7 @@ struct aac_dev
 	struct aac_hba_map_info	hba_map[AAC_MAX_BUSES][AAC_MAX_TARGETS];
 	u8			adapter_shutdown;
 	u32			handle_pci_error;
+	bool			init_reset;
 };
 
 #define aac_adapter_interrupt(dev) \
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index c9252b138c1f..bdf127aaab41 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1680,6 +1680,9 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	aac->cardtype = index;
 	INIT_LIST_HEAD(&aac->entry);
 
+	if (aac_reset_devices || reset_devices)
+		aac->init_reset = true;
+
 	aac->fibs = kzalloc(sizeof(struct fib) * (shost->can_queue + AAC_NUM_MGT_FIB), GFP_KERNEL);
 	if (!aac->fibs)
 		goto out_free_host;
diff --git a/drivers/scsi/aacraid/rx.c b/drivers/scsi/aacraid/rx.c
index 93ef7c37e568..ff2af06e7dd9 100644
--- a/drivers/scsi/aacraid/rx.c
+++ b/drivers/scsi/aacraid/rx.c
@@ -561,11 +561,16 @@ int _aac_rx_init(struct aac_dev *dev)
 	dev->a_ops.adapter_sync_cmd = rx_sync_cmd;
 	dev->a_ops.adapter_enable_int = aac_rx_disable_interrupt;
 	dev->OIMR = status = rx_readb (dev, MUnit.OIMR);
-	if ((((status & 0x0c) != 0x0c) || aac_reset_devices || reset_devices) &&
-	  !aac_rx_restart_adapter(dev, 0, IOP_HWSOFT_RESET))
-		/* Make sure the Hardware FIFO is empty */
-		while ((++restart < 512) &&
-		  (rx_readl(dev, MUnit.OutboundQueue) != 0xFFFFFFFFL));
+
+	if (((status & 0x0c) != 0x0c) || dev->init_reset) {
+		dev->init_reset = false;
+		if (!aac_rx_restart_adapter(dev, 0, IOP_HWSOFT_RESET)) {
+			/* Make sure the Hardware FIFO is empty */
+			while ((++restart < 512) &&
+			      (rx_readl(dev, MUnit.OutboundQueue) != 0xFFFFFFFFL));
+		}
+	}
+
 	/*
 	 *	Check to see if the board panic'd while booting.
 	 */
diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c
index 0c9361c87ec8..fde6b6aa86e3 100644
--- a/drivers/scsi/aacraid/src.c
+++ b/drivers/scsi/aacraid/src.c
@@ -868,9 +868,13 @@ int aac_src_init(struct aac_dev *dev)
 	/* Failure to reset here is an option ... */
 	dev->a_ops.adapter_sync_cmd = src_sync_cmd;
 	dev->a_ops.adapter_enable_int = aac_src_disable_interrupt;
-	if ((aac_reset_devices || reset_devices) &&
-		!aac_src_restart_adapter(dev, 0, IOP_HWSOFT_RESET))
-		++restart;
+
+	if (dev->init_reset) {
+		dev->init_reset = false;
+		if (!aac_src_restart_adapter(dev, 0, IOP_HWSOFT_RESET))
+			++restart;
+	}
+
 	/*
 	 *	Check to see if the board panic'd while booting.
 	 */
@@ -1014,9 +1018,13 @@ int aac_srcv_init(struct aac_dev *dev)
 	/* Failure to reset here is an option ... */
 	dev->a_ops.adapter_sync_cmd = src_sync_cmd;
 	dev->a_ops.adapter_enable_int = aac_src_disable_interrupt;
-	if ((aac_reset_devices || reset_devices) &&
-		!aac_src_restart_adapter(dev, 0, IOP_HWSOFT_RESET))
-		++restart;
+
+	if (dev->init_reset) {
+		dev->init_reset = false;
+		if (!aac_src_restart_adapter(dev, 0, IOP_HWSOFT_RESET))
+			++restart;
+	}
+
 	/*
 	 *	Check to see if flash update is running.
 	 *	Wait for the adapter to be up and running. Wait up to 5 minutes
-- 
2.15.0

[PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path

[Guilherme G. Piccoli]

As part of the scsi EH path, aacraid performs a reinitialization of
the adapter, which encompass freeing resources and IRQs, NULLifying
lots of pointers, and then initialize it all over again.
We've identified a problem during the free IRQ portion of this path
if CONFIG_DEBUG_SHIRQ is enabled on kernel config file.

Happens that, in case this flag was set, right after free_irq()
effectively clears the interrupt, it checks if it was requested
as IRQF_SHARED. In positive case, it performs another call to the
IRQ handler on driver. Problem is: since aacraid currently free
some resources *before* freeing the IRQ, once free_irq() path
calls the handler again (due to CONFIG_DEBUG_SHIRQ), aacraid
crashes due to NULL pointer dereference with the following trace:

  aac_src_intr_message+0xf8/0x740 [aacraid]
  __free_irq+0x33c/0x4a0
  free_irq+0x78/0xb0
  aac_free_irq+0x13c/0x150 [aacraid]
  aac_reset_adapter+0x2e8/0x970 [aacraid]
  aac_eh_reset+0x3a8/0x5d0 [aacraid]
  scsi_try_host_reset+0x74/0x180
  scsi_eh_ready_devs+0xc70/0x1510
  scsi_error_handler+0x624/0xa20

This patch prevents the crash by changing the order of the
deinitialization in this path of aacraid: first we clear the IRQ,
then we free other resources. No functional change intended.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
---
 drivers/scsi/aacraid/commsup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 2abe8fd83494..bec9f3193f60 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1554,6 +1554,7 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	 * will ensure that i/o is queisced and the card is flushed in that
 	 * case.
 	 */
+	aac_free_irq(aac);
 	aac_fib_map_free(aac);
 	dma_free_coherent(&aac->pdev->dev, aac->comm_size, aac->comm_addr,
 			  aac->comm_phys);
@@ -1561,7 +1562,6 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	aac->comm_phys = 0;
 	kfree(aac->queues);
 	aac->queues = NULL;
-	aac_free_irq(aac);
 	kfree(aac->fsa_dev);
 	aac->fsa_dev = NULL;
 
-- 
2.15.0

RE: [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path

[Raghava Aditya Renukunta]

> -----Original Message-----
> From: Guilherme G. Piccoli [mailto:gpiccoli@linux.vnet.ibm.com]
> Sent: Friday, November 17, 2017 1:15 PM
> To: dl-esc-Aacraid Linux Driver <aacraid@microsemi.com>; linux-
> scsi@vger.kernel.org
> Cc: gpiccoli@linux.vnet.ibm.com; Dave Carroll
> <david.carroll@microsemi.com>; Raghava Aditya Renukunta
> <RaghavaAditya.Renukunta@microsemi.com>; gpiccoli@protonmail.ch
> Subject: [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt
> during scsi EH path
> 
> EXTERNAL EMAIL
> 
> 
> As part of the scsi EH path, aacraid performs a reinitialization of
> the adapter, which encompass freeing resources and IRQs, NULLifying
> lots of pointers, and then initialize it all over again.
> We've identified a problem during the free IRQ portion of this path
> if CONFIG_DEBUG_SHIRQ is enabled on kernel config file.
> 
> Happens that, in case this flag was set, right after free_irq()
> effectively clears the interrupt, it checks if it was requested
> as IRQF_SHARED. In positive case, it performs another call to the
> IRQ handler on driver. Problem is: since aacraid currently free
> some resources *before* freeing the IRQ, once free_irq() path
> calls the handler again (due to CONFIG_DEBUG_SHIRQ), aacraid
> crashes due to NULL pointer dereference with the following trace:
> 
>   aac_src_intr_message+0xf8/0x740 [aacraid]
>   __free_irq+0x33c/0x4a0
>   free_irq+0x78/0xb0
>   aac_free_irq+0x13c/0x150 [aacraid]
>   aac_reset_adapter+0x2e8/0x970 [aacraid]
>   aac_eh_reset+0x3a8/0x5d0 [aacraid]
>   scsi_try_host_reset+0x74/0x180
>   scsi_eh_ready_devs+0xc70/0x1510
>   scsi_error_handler+0x624/0xa20
> 
> This patch prevents the crash by changing the order of the
> deinitialization in this path of aacraid: first we clear the IRQ,
> then we free other resources. No functional change intended.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> ---

Thank you for the fix. Much appreciated. 

Reviewed-by :Raghava Aditya Renukunta <RaghavaAditya.Renukunta@microsemi.com>

RE: [PATCH 2/3] scsi: aacraid: Perform initialization reset only once

[Raghava Aditya Renukunta]

> -----Original Message-----
> From: Guilherme G. Piccoli [mailto:gpiccoli@linux.vnet.ibm.com]
> Sent: Friday, November 17, 2017 1:15 PM
> To: dl-esc-Aacraid Linux Driver <aacraid@microsemi.com>; linux-
> scsi@vger.kernel.org
> Cc: gpiccoli@linux.vnet.ibm.com; Dave Carroll
> <david.carroll@microsemi.com>; Raghava Aditya Renukunta
> <RaghavaAditya.Renukunta@microsemi.com>; gpiccoli@protonmail.ch
> Subject: [PATCH 2/3] scsi: aacraid: Perform initialization reset only once
> 
> EXTERNAL EMAIL
> 
> 
> Currently the driver accepts two ways of requesting an initialization
> reset on the adapter: by passing aac_reset_devices module parameter,
> or the generic kernel parameter reset_devices.
> 
> It's working as intended...but if we end up reaching a scsi hang and
> the scsi EH mechanism takes place, aacraid performs resets as part of
> the scsi error recovery procedure. These EH routines might reinitialize
> the device, and if we have provided some of the reset parameters in the
> kernel command-line, we again perform an "initialization" reset.
> 
> So, to avoid this duplication of resets in case of scsi EH path, this
> patch adds a field to aac_dev struct to keep per-adapter track of the
> init reset request - once it's done, we set it to false and don't
> proactively reset anymore in case of reinitializations.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> ---

Reviewed-by :Raghava Aditya Renukunta <RaghavaAditya.Renukunta@microsemi.com>

RE: [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way

[Dave Carroll]

> Commit 16ae9dd35d37 ("scsi: aacraid: Fix for excessive prints on EEH")
> introduced checks about the state of device before any PCI operations
> in the driver. Basically, this prevents it to perform PCI accesses
> when device is in the process of recover from a PCI error. In PowerPC,
> such mechanism is called EEH, and the aforementioned commit introduced
> checks that are based on EEH-specific primitives for that.
> 
> The potential problems with this approach are three: first, these checks
> are "locked" to powerpc only - another archs could have error recovery
> methods too, like AER in Intel. Also, the powerpc primitives perform
> expensive FW accesses to validate the precise PCI state of a device.
> Finally, code becomes more complicated and needs ifdef validation
> based on arch config being set.
> 
> So, this patch makes use of generic PCI state checks, which are
> lightweight and non-dependent of arch configs - also, it makes
> the code cleaner.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
> ---

Reviewed-by: Dave Carroll <david.carroll@microsemi.com>

Re: [PATCH 0/3] Some fixes to aacraid

[Martin K. Petersen]

Guilherme,

> This series presents 3 small fixes for aacraid driver.  The most
> important is the crash prevention, IMHO.

Applied to 4.15/scsi-fixes. Thank you!

-- 
Martin K. Petersen	Oracle Linux Engineering