> 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.
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.
> Thanks,
> ~Saurav
>>
>> --
>> Himanshu Madhani Oracle Linux Engineering
--
Himanshu Madhani Oracle Linux Engineering
