Re: [PATCH v2] qla2xxx: Fix for locking issue between driver ISR and mailbox routines

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Acked-by: Saurav Kashyap <saurav.kashyap@xxxxxxxxxx>

Thanks,
~Saurav

>The driver uses ha->mbx_cmd_flags variable to pass information between
>its ISR and mailbox routines, however, it does so without the protection
>of
>any locks.  Under certain conditions, this can lead to multiple mailbox
>command completions being signaled, which, in turn, leads to a false
>mailbox timeout error for the subsequently issued mailbox command.
>
>The issue occurs frequently but intermittenly with the Qlogic 8GFC mezz
>card during card initialization, resulting in card initialization failure.
>
>Signed-off-by: Gurinder (Sunny) Shergill <gurinder.shergill@xxxxxx>
>---
> drivers/scsi/qla2xxx/qla_inline.h | 11 +++++++++++
> drivers/scsi/qla2xxx/qla_isr.c    | 27 ++++-----------------------
> drivers/scsi/qla2xxx/qla_mbx.c    |  2 --
> drivers/scsi/qla2xxx/qla_mr.c     | 10 ++--------
> drivers/scsi/qla2xxx/qla_nx.c     | 26 ++++++++++----------------
> 5 files changed, 27 insertions(+), 49 deletions(-)
>
>diff --git a/drivers/scsi/qla2xxx/qla_inline.h
>b/drivers/scsi/qla2xxx/qla_inline.h
>index 98ab921..0a5c895 100644
>--- a/drivers/scsi/qla2xxx/qla_inline.h
>+++ b/drivers/scsi/qla2xxx/qla_inline.h
>@@ -278,3 +278,14 @@ qla2x00_do_host_ramp_up(scsi_qla_host_t *vha)
>
>       set_bit(HOST_RAMP_UP_QUEUE_DEPTH, &vha->dpc_flags);
> }
>+
>+static inline void
>+qla2x00_handle_mbx_completion(struct qla_hw_data *ha, int status)
>+{
>+      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>+          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>+              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>+              clear_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags);
>+              complete(&ha->mbx_intr_comp);
>+      }
>+}
>diff --git a/drivers/scsi/qla2xxx/qla_isr.c
>b/drivers/scsi/qla2xxx/qla_isr.c
>index 259d920..d2a4c75 100644
>--- a/drivers/scsi/qla2xxx/qla_isr.c
>+++ b/drivers/scsi/qla2xxx/qla_isr.c
>@@ -104,14 +104,9 @@ qla2100_intr_handler(int irq, void *dev_id)
>                       RD_REG_WORD(&reg->hccr);
>               }
>       }
>+      qla2x00_handle_mbx_completion(ha, status);
>       spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>-
>       return (IRQ_HANDLED);
> }
>
>@@ -221,14 +216,9 @@ qla2300_intr_handler(int irq, void *dev_id)
>               WRT_REG_WORD(&reg->hccr, HCCR_CLR_RISC_INT);
>               RD_REG_WORD_RELAXED(&reg->hccr);
>       }
>+      qla2x00_handle_mbx_completion(ha, status);
>       spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>-
>       return (IRQ_HANDLED);
> }
>
>@@ -2613,14 +2603,9 @@ qla24xx_intr_handler(int irq, void *dev_id)
>               if (unlikely(IS_QLA83XX(ha) && (ha->pdev->revision == 1)))
>                       ndelay(3500);
>       }
>+      qla2x00_handle_mbx_completion(ha, status);
>       spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>-
>       return IRQ_HANDLED;
> }
>
>@@ -2763,13 +2748,9 @@ qla24xx_msix_default(int irq, void *dev_id)
>               }
>               WRT_REG_DWORD(&reg->hccr, HCCRX_CLR_RISC_INT);
>       } while (0);
>+      qla2x00_handle_mbx_completion(ha, status);
>       spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>       return IRQ_HANDLED;
> }
>
>diff --git a/drivers/scsi/qla2xxx/qla_mbx.c
>b/drivers/scsi/qla2xxx/qla_mbx.c
>index 9e5d89d..3587ec2 100644
>--- a/drivers/scsi/qla2xxx/qla_mbx.c
>+++ b/drivers/scsi/qla2xxx/qla_mbx.c
>@@ -179,8 +179,6 @@ qla2x00_mailbox_command(scsi_qla_host_t *vha,
>mbx_cmd_t *mcp)
>
>               wait_for_completion_timeout(&ha->mbx_intr_comp, mcp->tov * HZ);
>
>-              clear_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags);
>-
>       } else {
>               ql_dbg(ql_dbg_mbx, vha, 0x1011,
>                   "Cmd=%x Polling Mode.\n", command);
>diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
>index 729b743..5483da8 100644
>--- a/drivers/scsi/qla2xxx/qla_mr.c
>+++ b/drivers/scsi/qla2xxx/qla_mr.c
>@@ -148,9 +148,6 @@ qlafx00_mailbox_command(scsi_qla_host_t *vha, struct
>mbx_cmd_32 *mcp)
>               spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>               wait_for_completion_timeout(&ha->mbx_intr_comp, mcp->tov * HZ);
>-
>-              clear_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags);
>-
>       } else {
>               ql_dbg(ql_dbg_mbx, vha, 0x112c,
>                   "Cmd=%x Polling Mode.\n", command);
>@@ -2934,13 +2931,10 @@ qlafx00_intr_handler(int irq, void *dev_id)
>               QLAFX00_CLR_INTR_REG(ha, clr_intr);
>               QLAFX00_RD_INTR_REG(ha);
>       }
>+
>+      qla2x00_handle_mbx_completion(ha, status);
>       spin_unlock_irqrestore(&ha->hardware_lock, flags);
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>       return IRQ_HANDLED;
> }
>
>diff --git a/drivers/scsi/qla2xxx/qla_nx.c b/drivers/scsi/qla2xxx/qla_nx.c
>index 10754f51..cce0cd0 100644
>--- a/drivers/scsi/qla2xxx/qla_nx.c
>+++ b/drivers/scsi/qla2xxx/qla_nx.c
>@@ -2074,9 +2074,6 @@ qla82xx_intr_handler(int irq, void *dev_id)
>               }
>               WRT_REG_DWORD(&reg->host_int, 0);
>       }
>-      spin_unlock_irqrestore(&ha->hardware_lock, flags);
>-      if (!ha->flags.msi_enabled)
>-              qla82xx_wr_32(ha, ha->nx_legacy_intr.tgt_mask_reg, 0xfbff);
>
> #ifdef QL_DEBUG_LEVEL_17
>       if (!irq && ha->flags.eeh_busy)
>@@ -2085,11 +2082,12 @@ qla82xx_intr_handler(int irq, void *dev_id)
>                   status, ha->mbx_cmd_flags, ha->flags.mbox_int, stat);
> #endif
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-          (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-              set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-              complete(&ha->mbx_intr_comp);
>-      }
>+      qla2x00_handle_mbx_completion(ha, status);
>+      spin_unlock_irqrestore(&ha->hardware_lock, flags);
>+
>+      if (!ha->flags.msi_enabled)
>+              qla82xx_wr_32(ha, ha->nx_legacy_intr.tgt_mask_reg, 0xfbff);
>+
>       return IRQ_HANDLED;
> }
>
>@@ -2149,8 +2147,6 @@ qla82xx_msix_default(int irq, void *dev_id)
>               WRT_REG_DWORD(&reg->host_int, 0);
>       } while (0);
>
>-      spin_unlock_irqrestore(&ha->hardware_lock, flags);
>-
> #ifdef QL_DEBUG_LEVEL_17
>       if (!irq && ha->flags.eeh_busy)
>               ql_log(ql_log_warn, vha, 0x5044,
>@@ -2158,11 +2154,9 @@ qla82xx_msix_default(int irq, void *dev_id)
>                   status, ha->mbx_cmd_flags, ha->flags.mbox_int, stat);
> #endif
>
>-      if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
>-              (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
>-                      set_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags);
>-                      complete(&ha->mbx_intr_comp);
>-      }
>+      qla2x00_handle_mbx_completion(ha, status);
>+      spin_unlock_irqrestore(&ha->hardware_lock, flags);
>+
>       return IRQ_HANDLED;
> }
>
>@@ -3345,7 +3339,7 @@ void qla82xx_clear_pending_mbx(scsi_qla_host_t *vha)
>               ha->flags.mbox_busy = 0;
>               ql_log(ql_log_warn, vha, 0x6010,
>                   "Doing premature completion of mbx command.\n");
>-              if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags))
>+              if (test_and_clear_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags))
>                       complete(&ha->mbx_intr_comp);
>       }
> }
>--
>1.7.11.7
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>the body of a message to majordomo@xxxxxxxxxxxxxxx
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux