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 [off-list ref] wrote:

Hi Himanshu,
Thanks for the review, comments inline..

quoted

-----Original Message-----
From: Himanshu Madhani <redacted>
Sent: Wednesday, January 6, 2021 8:37 PM
To: Nilesh Javali <njavali@marvell.com>; Saurav Kashyap
[off-list ref]
Cc: Martin K. Petersen <martin.petersen@oracle.com>; linux-
scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-
Upstream@marvell.com>
Subject: [EXT] Re: [PATCH v3 3/7] qla2xxx: Move some messages from debug to
normal log level

External Email

----------------------------------------------------------------------
Hi Saurav,

quoted

On Jan 5, 2021, at 4:38 AM, Nilesh Javali [off-list ref] wrote:

From: Saurav Kashyap <skashyap@marvell.com>

This change will aid in debugging issues where debug level is not set.

Signed-off-by: Saurav Kashyap <skashyap@marvell.com>
Signed-off-by: Nilesh Javali <njavali@marvell.com>
---
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,

quoted

	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",

quoted

+	       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)

quoted

		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)

quoted

@@ -2221,12 +2221,12 @@ qla24xx_logio_entry(scsi_qla_host_t *vha, struct

req_que *req,

quoted

		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",

quoted

-	    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",

quoted

+	       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,

quoted

		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",

quoted

-				tgt_xfer_len, fd->transferred_length);
+			ql_log(ql_log_warn, fcport->vha, 0x3079,
+			       "Dropped frame(s) detected

(sent/rcvd=%u/%u).\n",

quoted

+			       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)

quoted

		scsi_set_resid(cp, resid);
		if (scsi_status & SS_RESIDUAL_UNDER) {
			if (IS_FWI2_CAPABLE(ha) && fw_resid_len !=

resid_len) {

quoted

-				ql_dbg(ql_dbg_io, fcport->vha, 0x301d,
-				    "Dropped frame(s) detected (0x%x of 0x%x

bytes).\n",

quoted

-				    resid, scsi_bufflen(cp));
+				ql_log(ql_log_warn, fcport->vha, 0x301d,
+				       "Dropped frame(s) detected (0x%x of 0x%x

bytes).\n",

quoted

+				       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)

quoted

			 * task not completed.
			 */

-			ql_dbg(ql_dbg_io, fcport->vha, 0x301f,
-			    "Dropped frame(s) detected (0x%x of 0x%x

bytes).\n",

quoted

-			    resid, scsi_bufflen(cp));
+			ql_log(ql_log_warn, fcport->vha, 0x301f,
+			       "Dropped frame(s) detected (0x%x of 0x%x

bytes).\n",

quoted

+			       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)

quoted

out:
	if (logit)
-		ql_dbg(ql_dbg_io, fcport->vha, 0x3022,
-		    "FCP command status: 0x%x-0x%x (0x%x) nexus=%ld:%d:%llu

"

quoted

-		    "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",

quoted

+		       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

quoted

--
Himanshu Madhani	 Oracle Linux Engineering

--
Himanshu Madhani	 Oracle Linux Engineering