Hi Himanshu,
Thanks for the review, comments inline..
> -----Original Message-----
> From: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> Sent: Wednesday, January 6, 2021 8:37 PM
> To: Nilesh Javali <njavali@xxxxxxxxxxx>; Saurav Kashyap
> <skashyap@xxxxxxxxxxx>
> Cc: Martin K. Petersen <martin.petersen@xxxxxxxxxx>; linux-
> scsi@xxxxxxxxxxxxxxx; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-
> Upstream@xxxxxxxxxxx>
> Subject: [EXT] Re: [PATCH v3 3/7] qla2xxx: Move some messages from debug to
> normal log level
>
> External Email
>
> ----------------------------------------------------------------------
> Hi Saurav,
>
> > On Jan 5, 2021, at 4:38 AM, Nilesh Javali <njavali@xxxxxxxxxxx> wrote:
> >
> > From: Saurav Kashyap <skashyap@xxxxxxxxxxx>
> >
> > This change will aid in debugging issues where debug level is not set.
> >
> > Signed-off-by: Saurav Kashyap <skashyap@xxxxxxxxxxx>
> > Signed-off-by: Nilesh Javali <njavali@xxxxxxxxxxx>
> > ---
> > drivers/scsi/qla2xxx/qla_init.c | 10 +++----
> > drivers/scsi/qla2xxx/qla_isr.c | 52 ++++++++++++++++-----------------
> > 2 files changed, 30 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> > index 410ff5534a59..221369cdf71f 100644
> > --- a/drivers/scsi/qla2xxx/qla_init.c
> > +++ b/drivers/scsi/qla2xxx/qla_init.c
> > @@ -347,11 +347,11 @@ qla2x00_async_login(struct scsi_qla_host *vha,
> fc_port_t *fcport,
> > if (NVME_TARGET(vha->hw, fcport))
> > lio->u.logio.flags |= SRB_LOGIN_SKIP_PRLI;
> >
> > - ql_dbg(ql_dbg_disc, vha, 0x2072,
> > - "Async-login - %8phC hdl=%x, loopid=%x portid=%02x%02x%02x "
> > - "retries=%d.\n", fcport->port_name, sp->handle, fcport-
> >loop_id,
> > - fcport->d_id.b.domain, fcport->d_id.b.area, fcport->d_id.b.al_pa,
> > - fcport->login_retry);
> > + ql_log(ql_log_warn, vha, 0x2072,
> > + "Async-login - %8phC hdl=%x, loopid=%x portid=%02x%02x%02x
> retries=%d.\n",
> > + fcport->port_name, sp->handle, fcport->loop_id,
> > + fcport->d_id.b.domain, fcport->d_id.b.area, fcport->d_id.b.al_pa,
> > + fcport->login_retry);
> >
> > rval = qla2x00_start_sp(sp);
> > if (rval != QLA_SUCCESS) {
> > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> > index 9cf8326ab9fc..bfc8bbaeea46 100644
> > --- a/drivers/scsi/qla2xxx/qla_isr.c
> > +++ b/drivers/scsi/qla2xxx/qla_isr.c
> > @@ -1455,9 +1455,9 @@ qla2x00_async_event(scsi_qla_host_t *vha, struct
> rsp_que *rsp, uint16_t *mb)
> > if (ha->flags.npiv_supported && vha->vp_idx != (mb[3] & 0xff))
> > break;
> >
> > - ql_dbg(ql_dbg_async, vha, 0x5013,
> > - "RSCN database changed -- %04x %04x %04x.\n",
> > - mb[1], mb[2], mb[3]);
> > + ql_log(ql_log_warn, vha, 0x5013,
> > + "RSCN database changed -- %04x %04x %04x.\n",
> > + mb[1], mb[2], mb[3]);
> >
> > rscn_entry = ((mb[1] & 0xff) << 16) | mb[2];
> > host_pid = (vha->d_id.b.domain << 16) | (vha->d_id.b.area <<
> 8)
> > @@ -2221,12 +2221,12 @@ qla24xx_logio_entry(scsi_qla_host_t *vha, struct
> req_que *req,
> > break;
> > }
> >
> > - ql_dbg(ql_dbg_async, sp->vha, 0x5037,
> > - "Async-%s failed: handle=%x pid=%06x wwpn=%8phC
> comp_status=%x iop0=%x iop1=%x\n",
> > - type, sp->handle, fcport->d_id.b24, fcport->port_name,
> > - le16_to_cpu(logio->comp_status),
> > - le32_to_cpu(logio->io_parameter[0]),
> > - le32_to_cpu(logio->io_parameter[1]));
> > + ql_log(ql_log_warn, sp->vha, 0x5037,
> > + "Async-%s failed: handle=%x pid=%06x wwpn=%8phC
> comp_status=%x iop0=%x iop1=%x\n",
> > + type, sp->handle, fcport->d_id.b24, fcport->port_name,
> > + le16_to_cpu(logio->comp_status),
> > + le32_to_cpu(logio->io_parameter[0]),
> > + le32_to_cpu(logio->io_parameter[1]));
> >
> > logio_done:
> > sp->done(sp, 0);
> > @@ -2389,9 +2389,9 @@ static void
> qla24xx_nvme_iocb_entry(scsi_qla_host_t *vha, struct req_que *req,
> >
> > tgt_xfer_len = be32_to_cpu(rsp_iu->xfrd_len);
> > if (fd->transferred_length != tgt_xfer_len) {
> > - ql_dbg(ql_dbg_io, fcport->vha, 0x3079,
> > - "Dropped frame(s) detected
> (sent/rcvd=%u/%u).\n",
> > - tgt_xfer_len, fd->transferred_length);
> > + ql_log(ql_log_warn, fcport->vha, 0x3079,
> > + "Dropped frame(s) detected
> (sent/rcvd=%u/%u).\n",
> > + tgt_xfer_len, fd->transferred_length);
> > logit = 1;
> > } else if (le16_to_cpu(comp_status) == CS_DATA_UNDERRUN) {
> > /*
> > @@ -3112,9 +3112,9 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct
> rsp_que *rsp, void *pkt)
> > scsi_set_resid(cp, resid);
> > if (scsi_status & SS_RESIDUAL_UNDER) {
> > if (IS_FWI2_CAPABLE(ha) && fw_resid_len !=
> resid_len) {
> > - ql_dbg(ql_dbg_io, fcport->vha, 0x301d,
> > - "Dropped frame(s) detected (0x%x of 0x%x
> bytes).\n",
> > - resid, scsi_bufflen(cp));
> > + ql_log(ql_log_warn, fcport->vha, 0x301d,
> > + "Dropped frame(s) detected (0x%x of 0x%x
> bytes).\n",
> > + resid, scsi_bufflen(cp));
> >
> > vha->interface_err_cnt++;
> >
> > @@ -3139,9 +3139,9 @@ qla2x00_status_entry(scsi_qla_host_t *vha, struct
> rsp_que *rsp, void *pkt)
> > * task not completed.
> > */
> >
> > - ql_dbg(ql_dbg_io, fcport->vha, 0x301f,
> > - "Dropped frame(s) detected (0x%x of 0x%x
> bytes).\n",
> > - resid, scsi_bufflen(cp));
> > + ql_log(ql_log_warn, fcport->vha, 0x301f,
> > + "Dropped frame(s) detected (0x%x of 0x%x
> bytes).\n",
> > + resid, scsi_bufflen(cp));
> >
> > vha->interface_err_cnt++;
> >
> > @@ -3257,15 +3257,13 @@ qla2x00_status_entry(scsi_qla_host_t *vha,
> struct rsp_que *rsp, void *pkt)
> >
> > out:
> > if (logit)
> > - ql_dbg(ql_dbg_io, fcport->vha, 0x3022,
> > - "FCP command status: 0x%x-0x%x (0x%x) nexus=%ld:%d:%llu
> "
> > - "portid=%02x%02x%02x oxid=0x%x cdb=%10phN len=0x%x "
> > - "rsp_info=0x%x resid=0x%x fw_resid=0x%x sp=%p cp=%p.\n",
> > - comp_status, scsi_status, res, vha->host_no,
> > - cp->device->id, cp->device->lun, fcport->d_id.b.domain,
> > - fcport->d_id.b.area, fcport->d_id.b.al_pa, ox_id,
> > - cp->cmnd, scsi_bufflen(cp), rsp_info_len,
> > - resid_len, fw_resid_len, sp, cp);
> > + ql_log(ql_log_warn, fcport->vha, 0x3022,
> > + "FCP command status: 0x%x-0x%x (0x%x)
> nexus=%ld:%d:%llu portid=%02x%02x%02x oxid=0x%x cdb=%10phN len=0x%x
> rsp_info=0x%x resid=0x%x fw_resid=0x%x sp=%p cp=%p.\n",
> > + comp_status, scsi_status, res, vha->host_no,
> > + cp->device->id, cp->device->lun, fcport->d_id.b.domain,
> > + fcport->d_id.b.area, fcport->d_id.b.al_pa, ox_id,
> > + cp->cmnd, scsi_bufflen(cp), rsp_info_len,
> > + resid_len, fw_resid_len, sp, cp);
> >
> > if (rsp->status_srb == NULL)
> > sp->done(sp, res);
> > --
> > 2.19.0.rc0
> >
>
> I like the direction of this patch.
>
> Can you consider removing "logit" variable. Since logit was designed to print
> messages only when a specific debug (IO bits in this case) was set.
<SK> logit is set under certain IO error conditions not based on any debug. If logit is removed,
this print will be come for each and every IO.
Thanks,
~Saurav
>
> --
> Himanshu Madhani Oracle Linux Engineering
