Nic,
I noticed this yesterday and took care of it already in my 3rd batch
which I'll send by end of this week (hopefully).
I don't know how I missed the warning when I compiled everything before
sending it to target-devel. But thanks anyways for pointing it out.
Another set of eyes is always good to have (:-).
With my patch to fix this yesterday, I only made a 1 line change which I
point out below. So, I have just minor comments on your patch below,
since it's a variation with the patch I made yesterday.
So, let me know which way you would rather lean.
Cheers,
Madhu
On Wed, 2011-10-19 at 23:36 -0700, Nicholas A. Bellinger wrote:
> On Sat, 2011-10-15 at 16:57 -0500, Madhuranath N Iyengar wrote:
> > From: Madhuranath Iyengar <mni@xxxxxxxxxxxxxxx>
> >
> > The two functions, qla_tgt_2xxx_handle_srr() and qla_tgt_24xx_handle_srr()
> > are merged into a single function called, qla_tgt_handle_srr().
> >
> > Signed-off-by: Madhuranath Iyengar <mni@xxxxxxxxxxxxxxx>
> > ---
> > drivers/scsi/qla2xxx/qla_target.c | 166 ++++++++----------------------------
> > 1 files changed, 37 insertions(+), 129 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> > index 4f9123d..585fbb52 100644
> > --- a/drivers/scsi/qla2xxx/qla_target.c
> > +++ b/drivers/scsi/qla2xxx/qla_target.c
> > @@ -3600,25 +3600,42 @@ static inline int qla_tgt_srr_adjust_data(struct qla_tgt_cmd *cmd,
> > }
> >
> > /* No locks, thread context */
> > -static void qla_tgt_24xx_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_ctio *sctio,
> > +static void qla_tgt_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_ctio *sctio,
> > struct qla_tgt_srr_imm *imm)
> > {
> > - imm_ntfy_from_24xx_entry_t *ntfy = &imm->imm.imm_ntfy24;
> > + void *ntfy, *atio;
> > + imm_ntfy_from_24xx_entry_t *ntfy24 = &imm->imm.imm_ntfy24;
> > + imm_ntfy_from_2xxx_entry_t *ntfy2x = &imm->imm.imm_ntfy;
> > struct qla_hw_data *ha = vha->hw;
> > struct qla_tgt_cmd *cmd = sctio->cmd;
> > struct se_cmd *se_cmd = &cmd->se_cmd;
> > unsigned long flags;
> > + int xmit_type, resp=0;
> > + uint32_t offset;
> > + uint16_t srr_ui;
> > +
> > + if (IS_FWI2_CAPABLE(ha)) {
> > + ntfy = (void *)ntfy24;
> > + atio = (void *)&cmd->atio.atio7;
> > + offset = le32_to_cpu(ntfy24->srr_rel_offs);
> > + srr_ui = ntfy24->srr_ui;
> > + } else {
> > + ntfy = (void *)ntfy2x;
> > + atio = (void *)&cmd->atio.atio2x;
> > + offset = le32_to_cpu(ntfy2x->srr_rel_offs);
> > + srr_ui = ntfy2x->srr_ui;
> > + }
> >
> > ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12c, "SRR cmd %p, srr_ui %x\n",
> > - cmd, ntfy->srr_ui);
> > + cmd, srr_ui);
> >
> > - switch (ntfy->srr_ui) {
> > + switch (srr_ui) {
> > case SRR_IU_STATUS:
> > spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > + qla_tgt_send_notify_ack(vha, ntfy,
> > 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > - __qla_tgt_24xx_xmit_response(cmd, QLA_TGT_XMIT_STATUS, se_cmd->scsi_status);
> > + resp = 1;
> > break;
> > case SRR_IU_DATA_IN:
> > if (!cmd->sg || !cmd->sg_cnt) {
> > @@ -3635,16 +3652,13 @@ static void qla_tgt_24xx_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_sr
> > cmd->bufflen = se_cmd->data_length;
> >
> > if (qla_tgt_has_data(cmd)) {
> > - uint32_t offset;
> > - int xmit_type;
> > - offset = le32_to_cpu(imm->imm.imm_ntfy24.srr_rel_offs);
> > if (qla_tgt_srr_adjust_data(cmd, offset, &xmit_type) != 0)
> > goto out_reject;
> > spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > + qla_tgt_send_notify_ack(vha, ntfy,
> > 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > - __qla_tgt_24xx_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
> > + resp = 1;
> > } else {
> > printk(KERN_ERR "qla_target(%d): SRR for in data for cmd "
> > "without them (tag %d, SCSI status %d), "
> > @@ -3668,13 +3682,10 @@ static void qla_tgt_24xx_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_sr
> > cmd->bufflen = se_cmd->data_length;
> >
> > if (qla_tgt_has_data(cmd)) {
> > - uint32_t offset;
> > - int xmit_type;
> > - offset = le32_to_cpu(imm->imm.imm_ntfy24.srr_rel_offs);
> > if (qla_tgt_srr_adjust_data(cmd, offset, &xmit_type) != 0)
> > goto out_reject;
> > spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > + qla_tgt_send_notify_ack(vha, ntfy,
> > 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > if (xmit_type & QLA_TGT_XMIT_DATA)
> > @@ -3689,124 +3700,25 @@ static void qla_tgt_24xx_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_sr
> > break;
> > default:
> > printk(KERN_ERR "qla_target(%d): Unknown srr_ui value %x",
> > - vha->vp_idx, ntfy->srr_ui);
> > + vha->vp_idx, srr_ui);
> > goto out_reject;
> > }
> >
> > - return;
> > -
> > -out_reject:
> > - spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy, 0, 0, 0,
> > - NOTIFY_ACK_SRR_FLAGS_REJECT,
> > - NOTIFY_ACK_SRR_REJECT_REASON_UNABLE_TO_PERFORM,
> > - NOTIFY_ACK_SRR_FLAGS_REJECT_EXPL_NO_EXPL);
> > - if (cmd->state == QLA_TGT_STATE_NEED_DATA) {
> > - cmd->state = QLA_TGT_STATE_DATA_IN;
> > - dump_stack();
> > - } else
> > - qla_tgt_send_term_exchange(vha, cmd, (void *)&cmd->atio.atio7, 1);
> > - spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > -}
> > -
> > -/* No locks, thread context */
> > -static void qla_tgt_2xxx_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_ctio *sctio,
> > - struct qla_tgt_srr_imm *imm)
> > -{
> > - imm_ntfy_from_2xxx_entry_t *ntfy = &imm->imm.imm_ntfy;
> > - struct qla_hw_data *ha = vha->hw;
> > - struct qla_tgt_cmd *cmd = sctio->cmd;
> > - struct se_cmd *se_cmd = &cmd->se_cmd;
> > - unsigned long flags;
> > -
> > - ql_dbg(ql_dbg_tgt_mgt, vha, 0xe12d, "SRR cmd %p, srr_ui %x\n",
> > - cmd, ntfy->srr_ui);
> > -
> > - switch (ntfy->srr_ui) {
> > - case SRR_IU_STATUS:
> > - spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > - 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > - spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > - __qla_tgt_2xxx_xmit_response(cmd, QLA_TGT_XMIT_STATUS, se_cmd->scsi_status);
> > - break;
> > - case SRR_IU_DATA_IN:
> > - if (!cmd->sg || !cmd->sg_cnt) {
> > - printk(KERN_ERR "Unable to process SRR_IU_DATA_IN due to"
> > - " missing cmd->sg, state: %d\n", cmd->state);
> > - dump_stack();
> > - goto out_reject;
> > - }
> > - if (se_cmd->scsi_status != 0) {
> > - ql_dbg(ql_dbg_tgt, vha, 0xe024, "Rejecting SRR_IU_DATA_IN"
> > - " with non GOOD scsi_status\n");
> > - goto out_reject;
> > - }
> > - cmd->bufflen = se_cmd->data_length;
> > -
> > - if (qla_tgt_has_data(cmd)) {
> > - uint32_t offset;
> > - int xmit_type;
> > - offset = le32_to_cpu(imm->imm.imm_ntfy.srr_rel_offs);
> > - if (qla_tgt_srr_adjust_data(cmd, offset, &xmit_type) != 0)
> > - goto out_reject;
> > - spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > - 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > - spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > - __qla_tgt_2xxx_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
> > - } else {
> > - printk(KERN_ERR "qla_target(%d): SRR for in data for cmd "
> > - "without them (tag %d, SCSI status %d), "
> > - "reject", vha->vp_idx, cmd->tag,
> > - cmd->se_cmd.scsi_status);
> > - goto out_reject;
> > - }
> > - break;
> > - case SRR_IU_DATA_OUT:
> > - if (!cmd->sg || !cmd->sg_cnt) {
> > - printk(KERN_ERR "Unable to process SRR_IU_DATA_OUT due to"
> > - " missing cmd->sg\n");
> > - goto out_reject;
> > - }
> > - if (se_cmd->scsi_status != 0) {
> > - ql_dbg(ql_dbg_tgt, vha, 0xe025, "Rejecting SRR_IU_DATA_OUT"
> > - " with non GOOD scsi_status\n");
> > - goto out_reject;
> > - }
> > - cmd->bufflen = se_cmd->data_length;
> > -
> > - if (qla_tgt_has_data(cmd)) {
> > - uint32_t offset;
> > - int xmit_type;
> > - offset = le32_to_cpu(imm->imm.imm_ntfy.srr_rel_offs);
> > - if (qla_tgt_srr_adjust_data(cmd, offset, &xmit_type) != 0)
> > - goto out_reject;
> > - spin_lock_irqsave(&ha->hardware_lock, flags);
> > - qla_tgt_send_notify_ack(vha, (void *)ntfy,
> > - 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> > - spin_unlock_irqrestore(&ha->hardware_lock, flags);
> > - if (xmit_type & QLA_TGT_XMIT_DATA)
> > - qla_tgt_rdy_to_xfer(cmd);
> > - } else {
> > - printk(KERN_ERR "qla_target(%d): SRR for out data for cmd "
> > - "without them (tag %d, SCSI status %d), "
> > - "reject", vha->vp_idx, cmd->tag,
> > - cmd->se_cmd.scsi_status);
> > - goto out_reject;
> > + /* Transmit response in case of status and data-in cases */
> > + if (resp) {
> > + if (IS_FWI2_CAPABLE(ha))
> > + __qla_tgt_24xx_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
> > + else {
> > + __qla_tgt_2xxx_xmit_response(cmd, QLA_TGT_XMIT_STATUS,
> > + se_cmd->scsi_status);
>
> Hey Madhu,
>
> So the usage of __qla_tgt_2[4,x]xx_xmit_response() is not quite right
> here, as xmit_type needs to be set based on SRR_IU_STATUS or
> SRR_IU_DATA_IN usage for both cases. That is:
>
> *) SRR_IU_STATUS: Set QLA_TGT_XMIT_STATUS
> *) SRR_IU_DATA_IN: Have qla_tgt_srr_adjust_data() set this for us to
> either QLA_TGT_XMIT_ALL or QLA_TGT_XMIT_STATUS.
>
> Here is the change i'll go ahead and commit as an incremental patch to
> fix this up..
>
> Thanks,
>
> --nab
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 1acab3f..ec9fab4 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -3610,7 +3610,7 @@ static void qla_tgt_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_cti
> struct qla_tgt_cmd *cmd = sctio->cmd;
> struct se_cmd *se_cmd = &cmd->se_cmd;
> unsigned long flags;
> - int xmit_type, resp=0;
> + int xmit_type = 0, resp = 0;
I assigned xmit_type = QLA_TGT_XMIT_STATUS above in my patch from
yesterday (instead of the 0 you have assigned above), and no other
changes below, and that fixed the compiler warning.
> uint32_t offset;
> uint16_t srr_ui;
>
> @@ -3635,6 +3635,7 @@ static void qla_tgt_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_cti
> qla_tgt_send_notify_ack(vha, ntfy,
> 0, 0, 0, NOTIFY_ACK_SRR_FLAGS_ACCEPT, 0, 0);
> spin_unlock_irqrestore(&ha->hardware_lock, flags);
> + xmit_type = QLA_TGT_XMIT_STATUS;
This change is not needed if I make my change above, don't you think?
> resp = 1;
> break;
> case SRR_IU_DATA_IN:
> @@ -3708,10 +3709,8 @@ static void qla_tgt_handle_srr(struct scsi_qla_host *vha, struct qla_tgt_srr_cti
> if (resp) {
> if (IS_FWI2_CAPABLE(ha))
> __qla_tgt_24xx_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
> - else {
> - __qla_tgt_2xxx_xmit_response(cmd, QLA_TGT_XMIT_STATUS,
> - se_cmd->scsi_status);
> - }
> + else
> + __qla_tgt_2xxx_xmit_response(cmd, xmit_type, se_cmd->scsi_status);
> }
>
This change above is also probably not needed with my change, but
doesn't hurt.
> return;
>
>
--
--
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
