On Mon, 2011-09-26 at 09:41 -0700, Roland Dreier wrote:
> Does this really have to be so hard?
>
> First of all I'm not sure I understand the changes about interrupt
> handling / IS_NOPOLLING_TYPE -- I don't see how we go back to legacy
> interrupt mode, since MSI-X is still enabled as far as I can tell. In
> any case what's wrong with getting interrupts during the restart?
>
So in the process to get a working setup, not having the extra code to
at least attempt to disable interrupts did have an negative effect
during restart testing that eventually cased port login to not occur.
Granted, this was before removing the bogus 'Disable LIP login' bit that
was causing the LIP hangs during reset, but at least before this change
it was easy to reproduce an problem w/o the extra interrupt changes.
> Also why do we need to move the "online=1" out of restart_isp? I
> haven't tried the stress test you did, but it seemed to work in my
> testing just to add a call to process the atio queue right after the
> existing "online=1" assignment. The patch that I came up with
> (independent of yours and probably not as rigorously tested) is the
> following, which is pretty small if you leave out the debugging check
> junk I put in:
>
Looks like a reasonable simplification..
Btw, the tests with the original patch ran overnight w/o issue and
reached 4500+ restarts.. I'm happy to give this simplified patch a shot
this afternoon..
--nab
> --- linux-2.6.git.orig/drivers/scsi/qla2xxx/qla_def.h 2011-09-25
> 22:08:59.026884599 -0700
> +++ linux-2.6.git/drivers/scsi/qla2xxx/qla_def.h 2011-09-25
> 22:09:32.996971614 -0700
> @@ -2886,6 +2886,7 @@ typedef struct scsi_qla_host {
> uint32_t online :1;
> uint32_t rscn_queue_overflow :1;
> uint32_t reset_active :1;
> + uint32_t atio_ignored :1;
>
> uint32_t management_server_logged_in :1;
> uint32_t process_response_queue :1;
> --- linux-2.6.git.orig/drivers/scsi/qla2xxx/qla_init.c 2011-09-25
> 22:10:17.357085241 -0700
> +++ linux-2.6.git/drivers/scsi/qla2xxx/qla_init.c 2011-09-25
> 22:13:46.467620871 -0700
> @@ -498,6 +498,7 @@ qla2x00_initialize_adapter(scsi_qla_host
>
> /* Clear adapter flags. */
> vha->flags.online = 0;
> + vha->flags.atio_ignored = 0;
> ha->flags.chip_reset_done = 0;
> vha->flags.reset_active = 0;
> ha->flags.pci_channel_io_perm_failure = 0;
> @@ -4248,6 +4249,7 @@ qla2x00_restart_isp(scsi_qla_host_t *vha
> struct qla_hw_data *ha = vha->hw;
> struct req_que *req = ha->req_q_map[0];
> struct rsp_que *rsp = ha->rsp_q_map[0];
> + unsigned long flags;
>
> /* If firmware needs to be loaded */
> if (qla2x00_isp_firmware(vha)) {
> @@ -4272,6 +4274,19 @@ qla2x00_restart_isp(scsi_qla_host_t *vha
> qla2x00_marker(vha, req, rsp, 0, 0, MK_SYNC_ALL);
>
> vha->flags.online = 1;
> +
> + /*
> + * Process any ATIO queue entries that came in
> + * while we weren't online.
> + */
> + spin_lock_irqsave(&ha->hardware_lock, flags);
> + if (vha->flags.atio_ignored) {
> + qla_printk(KERN_INFO, ha, "Processing ignored ATIO entries\n");
> + vha->flags.atio_ignored = 0;
> + }
> + qla_tgt_24xx_process_atio_queue(vha);
> + spin_unlock_irqrestore(&ha->hardware_lock, flags);
> +
> /* Wait at most MAX_TARGET RSCNs for a stable link. */
> wait_time = 256;
> do {
> --- linux-2.6.git.orig/drivers/scsi/qla2xxx/qla_target.c 2011-09-25
> 22:07:24.917072474 -0700
> +++ linux-2.6.git/drivers/scsi/qla2xxx/qla_target.c 2011-09-25
> 22:10:03.537049841 -0700
> @@ -5340,8 +5340,11 @@ qla_tgt_24xx_process_atio_queue(struct s
> atio_t *pkt;
> int cnt, i;
>
> - if (!vha->flags.online)
> + if (!vha->flags.online) {
> + vha->flags.atio_ignored = 1;
> + qla_printk(KERN_INFO, ha, "ATIO entry ignored because online == 0\n");
> return;
> + }
>
> while (ha->atio_ring_ptr->signature != ATIO_PROCESSED) {
> pkt = ha->atio_ring_ptr;
>
>
> On Sun, Sep 25, 2011 at 9:26 PM, Nicholas A. Bellinger
> <nab@xxxxxxxxxxxxxxx> wrote:
> > From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
> >
> > This patch addresses an issue between the assignment of vha->flags.online = 1
> > to signal operational status in process context via qla2x00_do_dpc() ->
> > qla2x00_abort_isp() -> qla2x00_restart_isp(), and processing of individual
> > ATIO packets in interrupt context within qla_tgt_24xx_process_atio_queue().
> > This was causing qla_tgt_24xx_process_atio_queue() to explictly ignore
> > new packets with non-operational status before completion of qla2x00_restart_isp()
> > -> target mode operation transition, and more specifically ELS packets
> > that are expected to drive the re-creation of active qla_tgt_sess nexuses in
> > qla_target.c + tcm_qla2xxx code.
> >
> > This v2 patch takes a different approach than v1, which originally tried to
> > allow ATIO ring processing in qla_tgt_24xx_process_atio_queue() during
> > qla2x00_abort_isp() reset operation. This patch:
> >
> > *) Keep !vha->flags.online check in qla_tgt_24xx_process_atio_queue() to
> > prevent all ATIO ring processing during qla2x00_abort_isp() reset.
> > *) Prevent vha->flags.online = 1 from being set during qla2x00_abort_isp()
> > and qla2x00_restart_isp() processing.
> > *) Set vha->flags.online = 1 upon the completion of qla2x00_abort_isp(),
> > and make a direct call into qla_tgt_24xx_process_atio_queue() from
> > qla2x00_do_dpc() process context to handle ATIO packets that have
> > accumulated during qla2x00_abort_isp().
> > *) Add extra paranoid qla_tgt_mode_enabled_base_vha() checks around
> > existing IS_NOPOLLING_TYPE() usage in qla2x00_mailbox_command(),
> > qla24xx_disable_intrs() and qla24xx_reset_risc(). This allows
> > target mode to run in legacy interrupt mode for mailbox commands
> > during qla2x00_abort_isp() reset, and allows qla24xx_disable_intrs()
> > to clear qla_hw_data->interrupts_on during reset.
> >
> > This patch also depends upon 'Disable initial LIP' being cleared within
> > qla_tgt_24xx_config_nvram_stage1() for firmware_options_1:
> >
> > /* Enable initial LIP */
> > nv->firmware_options_1 &= __constant_cpu_to_le32(~BIT_9);
> >
> > Enabling this bit to 'Disable initial LIP' ends up causing intermittent
> > reset hangs with target mode operation, eventually causing the port to
> > be taken offline.
> >
> > Reported-by: Roland Dreier <roland@xxxxxxxxxxxxxxx>
> > Cc: Roland Dreier <roland@xxxxxxxxxxxxxxx>
> > Reported-by: Patrick Lee <patrick@xxxxxxxxxxxxxxx>
> > Cc: Patrick Lee <patrick@xxxxxxxxxxxxxxx>
> > Cc: Andrew Vasquez <andrew.vasquez@xxxxxxxxxx>
> > Cc: Madhuranath Iyengar <mni@xxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/scsi/qla2xxx/qla_init.c | 20 +++++++++++++++++---
> > drivers/scsi/qla2xxx/qla_mbx.c | 7 ++++---
> > drivers/scsi/qla2xxx/qla_os.c | 2 +-
> > drivers/scsi/qla2xxx/qla_target.c | 8 +++++++-
> > drivers/scsi/qla2xxx/qla_target.h | 8 ++++++++
> > 5 files changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> > index fca50d5..de8810d 100644
> > --- a/drivers/scsi/qla2xxx/qla_init.c
> > +++ b/drivers/scsi/qla2xxx/qla_init.c
> > @@ -1068,7 +1068,7 @@ qla24xx_reset_risc(scsi_qla_host_t *vha)
> >
> > spin_unlock_irqrestore(&ha->hardware_lock, flags);
> >
> > - if (IS_NOPOLLING_TYPE(ha))
> > + if (IS_NOPOLLING_TYPE(ha) && !qla_tgt_mode_enabled_base_vha(ha))
> > ha->isp_ops->enable_intrs(ha);
> > }
> >
> > @@ -4133,7 +4133,8 @@ qla2x00_abort_isp(scsi_qla_host_t *vha)
> > vha->marker_needed = 1;
> > }
> >
> > - vha->flags.online = 1;
> > + if (!qla_tgt_mode_enabled(vha))
> > + vha->flags.online = 1;
> >
> > qla_tgt_abort_isp(vha);
> >
> > @@ -4229,6 +4230,17 @@ qla2x00_abort_isp(scsi_qla_host_t *vha)
> > }
> > }
> > spin_unlock_irqrestore(&ha->vport_slock, flags);
> > + /*
> > + * For target mode, wait until the last moment to set the
> > + * online flag and kick the ATIO ring to process new packets
> > + * that have accumulated during qla2x00_abort_isp().
> > + */
> > + if (qla_tgt_mode_enabled(vha)) {
> > + spin_lock_irqsave(&ha->hardware_lock, flags);
> > + vha->flags.online = 1;
> > + qla_tgt_24xx_process_atio_queue(vha);
> > + spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > + }
> >
> > } else {
> > qla_printk(KERN_INFO, ha,
> > @@ -4279,7 +4291,9 @@ qla2x00_restart_isp(scsi_qla_host_t *vha)
> > /* Issue a marker after FW becomes ready. */
> > qla2x00_marker(vha, req, rsp, 0, 0, MK_SYNC_ALL);
> >
> > - vha->flags.online = 1;
> > + if (!qla_tgt_mode_enabled(vha))
> > + vha->flags.online = 1;
> > +
> > /* Wait at most MAX_TARGET RSCNs for a stable link. */
> > wait_time = 256;
> > do {
> > diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> > index 087b2cc..3813306 100644
> > --- a/drivers/scsi/qla2xxx/qla_mbx.c
> > +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> > @@ -148,7 +148,8 @@ qla2x00_mailbox_command(scsi_qla_host_t *vha, mbx_cmd_t *mcp)
> >
> > /* Wait for mbx cmd completion until timeout */
> >
> > - if ((!abort_active && io_lock_on) || IS_NOPOLLING_TYPE(ha)) {
> > + if ((!abort_active && io_lock_on) ||
> > + (IS_NOPOLLING_TYPE(ha) && !qla_tgt_mode_enabled_base_vha(ha))) {
> > set_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags);
> >
> > if (IS_QLA82XX(ha)) {
> > @@ -278,8 +279,8 @@ qla2x00_mailbox_command(scsi_qla_host_t *vha, mbx_cmd_t *mcp)
> >
> > /* Clean up */
> > ha->mcp = NULL;
> > -
> > - if ((abort_active || !io_lock_on) && !IS_NOPOLLING_TYPE(ha)) {
> > + if ((abort_active || !io_lock_on) &&
> > + (!IS_NOPOLLING_TYPE(ha) || qla_tgt_mode_enabled_base_vha(ha))) {
> > DEBUG11(printk("%s(%ld): checking for additional resp "
> > "interrupt.\n", __func__, base_vha->host_no));
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> > index 2ab9564..0b61ca7 100644
> > --- a/drivers/scsi/qla2xxx/qla_os.c
> > +++ b/drivers/scsi/qla2xxx/qla_os.c
> > @@ -1486,7 +1486,7 @@ qla24xx_disable_intrs(struct qla_hw_data *ha)
> > unsigned long flags = 0;
> > struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
> >
> > - if (IS_NOPOLLING_TYPE(ha))
> > + if (IS_NOPOLLING_TYPE(ha) && !qla_tgt_mode_enabled_base_vha(ha))
> > return;
> > spin_lock_irqsave(&ha->hardware_lock, flags);
> > ha->interrupts_on = 0;
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> > index 757e971..f26b743 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -5328,7 +5328,13 @@ qla_tgt_24xx_process_atio_queue(struct scsi_qla_host *vha)
> > struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
> > atio_t *pkt;
> > int cnt, i;
> > -
> > + /*
> > + * During qla2x00_abort_isp() we expect to hit this check in order
> > + * to prevent processing during reset. Upon completion of reset,
> > + * qla2x00_abort_isp() will set vha->flags.online=1 and make a
> > + * direct call to qla_tgt_24xx_process_atio_queue() to kick off
> > + * ATIO ring processing from qla2x00_do_dpc() process context.
> > + */
> > if (!vha->flags.online)
> > return;
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> > index 42d5479..e89a7e3 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.h
> > +++ b/drivers/scsi/qla2xxx/qla_target.h
> > @@ -1023,6 +1023,14 @@ static inline bool qla_tgt_mode_enabled(struct scsi_qla_host *ha)
> > return ha->host->active_mode & MODE_TARGET;
> > }
> >
> > +static inline bool qla_tgt_mode_enabled_base_vha(struct qla_hw_data *hw)
> > +{
> > + if (!hw->qla_tgt || !hw->qla_tgt->vha)
> > + return false;
> > +
> > + return qla_tgt_mode_enabled(hw->qla_tgt->vha);
> > +}
> > +
> > static inline bool qla_ini_mode_enabled(struct scsi_qla_host *ha)
> > {
> > return ha->host->active_mode & MODE_INITIATOR;
> > --
> > 1.7.2.5
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
