Hi Himanshu,
> -----Original Message-----
> From: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> Sent: Thursday, January 7, 2021 11:22 PM
> To: Saurav Kashyap <skashyap@xxxxxxxxxxx>
> Cc: Nilesh Javali <njavali@xxxxxxxxxxx>; Martin K. Petersen
> <martin.petersen@xxxxxxxxxx>; linux-scsi@xxxxxxxxxxxxxxx; GR-QLogic-
> Storage-Upstream <GR-QLogic-Storage-Upstream@xxxxxxxxxxx>
> Subject: Re: [EXT] [PATCH v3 3/7] qla2xxx: Move some messages from debug to
> normal log level
>
>
>
> > On Jan 6, 2021, at 11:51 PM, Saurav Kashyap <skashyap@xxxxxxxxxxx>
> wrote:
> >
> > 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.
> >
>
> Yeah. I see that and so it was more of a suggestion while you are optimizing
> debugging effort without having to enable extended logging. if you can
> optimize logit logic while at it would be nice. I’ll leave it upto you. No objection
> to patch itself.
I will look into it, I am not going to changes this patch and series.
If I end up in making changes then it will be new patch.
>
> However, when you submit v2 of this patchset, I do want more descriptive
> commit message. Usually small things like this can escape scrutiny under
> “debug level change” description.
Sure.
Thanks,
~Saurav
>
> > Thanks,
> > ~Saurav
> >>
> >> --
> >> Himanshu Madhani Oracle Linux Engineering
>
> --
> Himanshu Madhani Oracle Linux Engineering
