RE: [EXT] [PATCH v3 3/7] qla2xxx: Move some messages from debug to normal log level

Hi Himanshu,

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

quoted

On Jan 6, 2021, at 11:51 PM, Saurav Kashyap [off-list ref]

wrote:

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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,

quoted

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

quoted

Thanks,
~Saurav

quoted

--
Himanshu Madhani	 Oracle Linux Engineering

--
Himanshu Madhani	 Oracle Linux Engineering