* [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way
2017-11-17 21:14 [PATCH 0/3] Some fixes to aacraid Guilherme G. Piccoli
@ 2017-11-17 21:14 ` Guilherme G. Piccoli
2017-11-17 22:54 ` Dave Carroll
2017-11-17 21:14 ` [PATCH 2/3] scsi: aacraid: Perform initialization reset only once Guilherme G. Piccoli
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Guilherme G. Piccoli @ 2017-11-17 21:14 UTC (permalink / raw)
To: aacraid, linux-scsi
Cc: gpiccoli, david.carroll, RaghavaAditya.Renukunta, gpiccoli
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* RE: [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way
2017-11-17 21:14 ` [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way Guilherme G. Piccoli
@ 2017-11-17 22:54 ` Dave Carroll
0 siblings, 0 replies; 8+ messages in thread
From: Dave Carroll @ 2017-11-17 22:54 UTC (permalink / raw)
To: Guilherme G. Piccoli, dl-esc-Aacraid Linux Driver,
linux-scsi@vger.kernel.org
Cc: Raghava Aditya Renukunta, gpiccoli@protonmail.ch
> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] scsi: aacraid: Perform initialization reset only once
2017-11-17 21:14 [PATCH 0/3] Some fixes to aacraid Guilherme G. Piccoli
2017-11-17 21:14 ` [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way Guilherme G. Piccoli
@ 2017-11-17 21:14 ` Guilherme G. Piccoli
2017-11-17 22:11 ` Raghava Aditya Renukunta
2017-11-17 21:14 ` [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path Guilherme G. Piccoli
2017-11-21 3:34 ` [PATCH 0/3] Some fixes to aacraid Martin K. Petersen
3 siblings, 1 reply; 8+ messages in thread
From: Guilherme G. Piccoli @ 2017-11-17 21:14 UTC (permalink / raw)
To: aacraid, linux-scsi
Cc: gpiccoli, david.carroll, RaghavaAditya.Renukunta, gpiccoli
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* RE: [PATCH 2/3] scsi: aacraid: Perform initialization reset only once
2017-11-17 21:14 ` [PATCH 2/3] scsi: aacraid: Perform initialization reset only once Guilherme G. Piccoli
@ 2017-11-17 22:11 ` Raghava Aditya Renukunta
0 siblings, 0 replies; 8+ messages in thread
From: Raghava Aditya Renukunta @ 2017-11-17 22:11 UTC (permalink / raw)
To: Guilherme G. Piccoli, dl-esc-Aacraid Linux Driver,
linux-scsi@vger.kernel.org
Cc: Dave Carroll, gpiccoli@protonmail.ch
> -----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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path
2017-11-17 21:14 [PATCH 0/3] Some fixes to aacraid Guilherme G. Piccoli
2017-11-17 21:14 ` [PATCH 1/3] scsi: aacraid: Check for PCI state of device in a generic way Guilherme G. Piccoli
2017-11-17 21:14 ` [PATCH 2/3] scsi: aacraid: Perform initialization reset only once Guilherme G. Piccoli
@ 2017-11-17 21:14 ` Guilherme G. Piccoli
2017-11-17 22:09 ` Raghava Aditya Renukunta
2017-11-21 3:34 ` [PATCH 0/3] Some fixes to aacraid Martin K. Petersen
3 siblings, 1 reply; 8+ messages in thread
From: Guilherme G. Piccoli @ 2017-11-17 21:14 UTC (permalink / raw)
To: aacraid, linux-scsi
Cc: gpiccoli, david.carroll, RaghavaAditya.Renukunta, gpiccoli
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path
2017-11-17 21:14 ` [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path Guilherme G. Piccoli
@ 2017-11-17 22:09 ` Raghava Aditya Renukunta
0 siblings, 0 replies; 8+ messages in thread
From: Raghava Aditya Renukunta @ 2017-11-17 22:09 UTC (permalink / raw)
To: Guilherme G. Piccoli, dl-esc-Aacraid Linux Driver,
linux-scsi@vger.kernel.org
Cc: Dave Carroll, gpiccoli@protonmail.ch
> -----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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Some fixes to aacraid
2017-11-17 21:14 [PATCH 0/3] Some fixes to aacraid Guilherme G. Piccoli
` (2 preceding siblings ...)
2017-11-17 21:14 ` [PATCH 3/3] scsi: aacraid: Prevent crash in case of free interrupt during scsi EH path Guilherme G. Piccoli
@ 2017-11-21 3:34 ` Martin K. Petersen
3 siblings, 0 replies; 8+ messages in thread
From: Martin K. Petersen @ 2017-11-21 3:34 UTC (permalink / raw)
To: Guilherme G. Piccoli
Cc: aacraid, linux-scsi, david.carroll, RaghavaAditya.Renukunta,
gpiccoli
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
^ permalink raw reply [flat|nested] 8+ messages in thread