NULL check after starget_to_rport()?

PATCH In review Roel Kluin 2 participants 4 messages Jul 15, 2009 – Jul 17, 2009

Subsystems (2)

SCSI Cisco FCoE HBA driver

lore

Share & tools

lore

Roel Kluin

author

2009-07-15 21:10 UTC #0 lore raw

// vi include/scsi/scsi_transport_fc.h +396

#define starget_to_rport(s)			\
	scsi_is_fc_rport(s->dev.parent) ? dev_to_rport(s->dev.parent) : NULL

So shouldn't we test for this NULL value (something like the untested patch below?)

Also I had two more questions concerning this file:

In fnic_clean_pending_aborts(), rport isn't used after the starget_to_rport(). is the rport needed?

Should fnic_block_error_handler() return an error value if rport is NULL?

Roel

drivers/scsi/fnic/fnic_scsi.c +23 ~~-11~~

index bfc9969..8a6bb3e 100644
@@ -346,6 +346,9 @@ int fnic_queuecommand(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 	unsigned long ptr;
 
 	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport == NULL)
+		return 0;
+
 	ret = fc_remote_port_chkready(rport);
 	if (ret) {
 		sc->result = ret;

@@ -1230,6 +1233,9 @@ static void fnic_block_error_handler(struct scsi_cmnd *sc)
 	struct fc_rport *rport = starget_to_rport(scsi_target(sc->device));
 	unsigned long flags;
 
+	if (rport == NULL)
+		return;
+
 	spin_lock_irqsave(shost->host_lock, flags);
 	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
 		spin_unlock_irqrestore(shost->host_lock, flags);

@@ -1265,11 +1271,12 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
-		      (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun, sc->request->tag);
+	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport != NULL)
+		FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+				"Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
+				rport->port_id, sc->device->lun,
+				sc->request->tag);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up)) {
 		ret = FAILED;

@@ -1319,6 +1326,9 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	 * the IO. Else, just locally terminate the IO in the firmware
 	 */
 	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport == NULL)
+		goto fnic_abort_cmd_end;
+
 	if (fc_remote_port_chkready(rport) == 0)
 		task_req = FCPIO_ITMF_ABT_TASK;
 	else

@@ -1478,6 +1488,8 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
 		/* Now queue the abort command to firmware */
 		int_to_scsilun(sc->device->lun, &fc_lun);
 		rport = starget_to_rport(scsi_target(sc->device));
+		if (rport == NULL)
+			goto clean_pending_aborts_end;
 
 		if (fnic_queue_abort_io_req(fnic, tag,
 					    FCPIO_ITMF_ABT_TASK_TERM,

@@ -1547,19 +1559,19 @@ int fnic_device_reset(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Device reset called FCID 0x%x, LUN 0x%x\n",
-		      (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun);
 
+	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport != NULL)
+		FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+				"Device reset called FCID 0x%x, LUN 0x%x\n",
+				rport->port_id, sc->device->lun);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up))
 		goto fnic_device_reset_end;
 
 	/* Check if remote port up */
 	rport = starget_to_rport(scsi_target(sc->device));
-	if (fc_remote_port_chkready(rport))
+	if (rport == NULL || fc_remote_port_chkready(rport))
 		goto fnic_device_reset_end;
 
 	io_lock = fnic_io_lock_hash(fnic, sc);

Abhijeet Joglekar (abjoglek)

2009-07-16 08:28 UTC #1 lore raw

Quoted 7 lines

-----Original Message-----
From: Roel Kluin [mailto:[email protected]]
Sent: Wednesday, July 15, 2009 2:13 PM
To: Abhijeet Joglekar (abjoglek); Joe Eykholt (jeykholt); [email protected]
Subject: NULL check after starget_to_rport()?

Quoted 8 lines

// vi include/scsi/scsi_transport_fc.h +396
#define starget_to_rport(s)			\
	scsi_is_fc_rport(s->dev.parent) ? 
dev_to_rport(s->dev.parent) : NULL
So shouldn't we test for this NULL value (something like the untested patch
below?)

I think scsi_is_fc_rport(s->dev.parent) should always be true since the discovered scsi targets are all fc remote ports created by fc_remote_port_add(). So, NULL check should not be required on the rport.

Quoted 5 lines

Also I had two more questions concerning this file:
In fnic_clean_pending_aborts(), rport isn't used after the starget_to_rport().
is the rport needed?

No, it can be removed.

Quoted 2 lines

Should fnic_block_error_handler() return an error value if rport is NULL?

Same as above, it should not be NULL, since the target dev's parent should be an fc rport.

thanks
-- abhijeet

Quoted 99 lines

Roel

(quoted diff) +0 -0

b/drivers/scsi/fnic/fnic_scsi.c index bfc9969..8a6bb3e 100644

drivers/scsi/fnic/fnic_scsi.c +3 -0

@@ -346,6 +346,9 @@ int fnic_queuecommand(struct scsi_cmnd 
 *sc, void (*done)(struct scsi_cmnd *))
 	unsigned long ptr;
 
 	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport == NULL)
+		return 0;
+
 	ret = fc_remote_port_chkready(rport);
 	if (ret) {

drivers/scsi/fnic/fnic_scsi.c +3 -0

 		sc->result = ret;

@@ -1230,6 +1233,9 @@ static void 
 fnic_block_error_handler(struct scsi_cmnd *sc)
 	struct fc_rport *rport = 
 starget_to_rport(scsi_target(sc->device));
 	unsigned long flags;
 
+	if (rport == NULL)
+		return;
+
 	spin_lock_irqsave(shost->host_lock, flags);

drivers/scsi/fnic/fnic_scsi.c +0 -0

 	while (rport->port_state == FC_PORTSTATE_BLOCKED) {
 		spin_unlock_irqrestore(shost->host_lock,

flags); @@ -1265,11 +1271,12 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)

drivers/scsi/fnic/fnic_scsi.c +0 -4

 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
-

(starget_to_rport(scsi_target(sc->device)))->port_id,

drivers/scsi/fnic/fnic_scsi.c +4 -1

-		      sc->device->lun, sc->request->tag);
+	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport != NULL)
+		FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+				"Abort Cmd called FCID 0x%x,

LUN 0x%x TAG %d\n",

drivers/scsi/fnic/fnic_scsi.c +7 -0

+				rport->port_id, sc->device->lun,
+				sc->request->tag);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up)) {
 		ret = FAILED;

@@ -1319,6 +1326,9 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	 * the IO. Else, just locally terminate the IO in the firmware
 	 */
 	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport == NULL)
+		goto fnic_abort_cmd_end;
+
 	if (fc_remote_port_chkready(rport) == 0)
 		task_req = FCPIO_ITMF_ABT_TASK;
 	else

@@ -1478,6 +1488,8 @@ static int 
 fnic_clean_pending_aborts(struct fnic *fnic,
 		/* Now queue the abort command to firmware */
 		int_to_scsilun(sc->device->lun, &fc_lun);
 		rport = starget_to_rport(scsi_target(sc->device));
+		if (rport == NULL)
+			goto clean_pending_aborts_end;
 
 		if (fnic_queue_abort_io_req(fnic, tag,

drivers/scsi/fnic/fnic_scsi.c +6 -6

 					    FCPIO_ITMF_ABT_TASK_TERM,

@@ -1547,19 +1559,19 @@ int fnic_device_reset(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Device reset called FCID 0x%x, LUN 0x%x\n",
-		      
 (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun);
 
+	rport = starget_to_rport(scsi_target(sc->device));
+	if (rport != NULL)
+		FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+				"Device reset called FCID 0x%x, 
 LUN 0x%x\n",
+				rport->port_id, sc->device->lun);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up))
 		goto fnic_device_reset_end;
 
 	/* Check if remote port up */
 	rport = starget_to_rport(scsi_target(sc->device));
-	if (fc_remote_port_chkready(rport))
+	if (rport == NULL || fc_remote_port_chkready(rport))
 		goto fnic_device_reset_end;

io_lock = fnic_io_lock_hash(fnic, sc);

Roel Kluin

author

2009-07-16 11:21 UTC #2 lore raw

PATCH 1/1 fnic: clean up

In reply to Roel Kluin

In fnic_abort_cmd() and fnic_device_reset() assign `rport' earlier to make FNIC_SCSI_DBG() calls cleaner.

In fnic_clean_pending_aborts() `rport' is not used.

Signed-off-by Roel Kluin <[email protected]>

---
thanks abhijeet, is this ackable?

drivers/scsi/fnic/fnic_scsi.c +8 ~~-14~~

index bfc9969..c04b7e8 100644
@@ -1265,11 +1265,10 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
-		      (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun, sc->request->tag);
+	rport = starget_to_rport(scsi_target(sc->device));
+	FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+			"Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
+			rport->port_id, sc->device->lun, sc->request->tag);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up)) {
 		ret = FAILED;

@@ -1318,7 +1317,6 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	 * port is up, then send abts to the remote port to terminate
 	 * the IO. Else, just locally terminate the IO in the firmware
 	 */
-	rport = starget_to_rport(scsi_target(sc->device));
 	if (fc_remote_port_chkready(rport) == 0)
 		task_req = FCPIO_ITMF_ABT_TASK;
 	else

@@ -1437,7 +1435,6 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
 	unsigned long flags;
 	int ret = 0;
 	struct scsi_cmnd *sc;
-	struct fc_rport *rport;
 	struct scsi_lun fc_lun;
 	struct scsi_device *lun_dev = lr_sc->device;
 	DECLARE_COMPLETION_ONSTACK(tm_done);

@@ -1477,7 +1474,6 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
 
 		/* Now queue the abort command to firmware */
 		int_to_scsilun(sc->device->lun, &fc_lun);
-		rport = starget_to_rport(scsi_target(sc->device));
 
 		if (fnic_queue_abort_io_req(fnic, tag,
 					    FCPIO_ITMF_ABT_TASK_TERM,

@@ -1547,18 +1543,16 @@ int fnic_device_reset(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Device reset called FCID 0x%x, LUN 0x%x\n",
-		      (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun);
 
+	rport = starget_to_rport(scsi_target(sc->device));
+	FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+			"Device reset called FCID 0x%x, LUN 0x%x\n",
+			rport->port_id, sc->device->lun);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up))
 		goto fnic_device_reset_end;
 
 	/* Check if remote port up */
-	rport = starget_to_rport(scsi_target(sc->device));
 	if (fc_remote_port_chkready(rport))
 		goto fnic_device_reset_end;

Abhijeet Joglekar (abjoglek)

2009-07-17 17:14 UTC #3 lore raw

RE: [PATCH] fnic: clean up

Quoted 7 lines

-----Original Message-----
From: Roel Kluin [mailto:[email protected]]
Sent: Thursday, July 16, 2009 4:23 AM
To: Abhijeet Joglekar (abjoglek)
Cc: Joe Eykholt (jeykholt); [email protected]; Andrew Morton
Subject: [PATCH] fnic: clean up

Quoted 8 lines

In fnic_abort_cmd() and fnic_device_reset() assign `rport' earlier to make FNIC_SCSI_DBG() calls cleaner.
In fnic_clean_pending_aborts() `rport' is not used.
Signed-off-by Roel Kluin <[email protected]>
---
thanks abhijeet, is this ackable?

Yes, this looks good.

thanks
-- abhijeet

Quoted 73 lines

(quoted diff) +0 -0

b/drivers/scsi/fnic/fnic_scsi.c index bfc9969..c04b7e8 100644

drivers/scsi/fnic/fnic_scsi.c +4 -5

@@ -1265,11 +1265,10 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
-		      
 (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun, sc->request->tag);
+	rport = starget_to_rport(scsi_target(sc->device));
+	FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+			"Abort Cmd called FCID 0x%x, LUN 0x%x TAG %d\n",
+			rport->port_id, sc->device->lun, 
 sc->request->tag);

drivers/scsi/fnic/fnic_scsi.c +0 -2

 	if (lp->state != LPORT_ST_READY || !(lp->link_up)) {
 		ret = FAILED;

@@ -1318,7 +1317,6 @@ int fnic_abort_cmd(struct scsi_cmnd *sc)
 	 * port is up, then send abts to the remote port to terminate
 	 * the IO. Else, just locally terminate the IO in the firmware
 	 */
-	rport = starget_to_rport(scsi_target(sc->device));
 	if (fc_remote_port_chkready(rport) == 0)
 		task_req = FCPIO_ITMF_ABT_TASK;
 	else

@@ -1437,7 +1435,6 @@ static int 
 fnic_clean_pending_aborts(struct fnic *fnic,
 	unsigned long flags;
 	int ret = 0;
 	struct scsi_cmnd *sc;
-	struct fc_rport *rport;
 	struct scsi_lun fc_lun;
 	struct scsi_device *lun_dev = lr_sc->device;

drivers/scsi/fnic/fnic_scsi.c +0 -1

 	DECLARE_COMPLETION_ONSTACK(tm_done);

@@ -1477,7 +1474,6 @@ static int 
 fnic_clean_pending_aborts(struct fnic *fnic,
 
 		/* Now queue the abort command to firmware */
 		int_to_scsilun(sc->device->lun, &fc_lun);
-		rport = starget_to_rport(scsi_target(sc->device));
 
 		if (fnic_queue_abort_io_req(fnic, tag,

drivers/scsi/fnic/fnic_scsi.c +4 -6

 					    FCPIO_ITMF_ABT_TASK_TERM,

@@ -1547,18 +1543,16 @@ int fnic_device_reset(struct scsi_cmnd *sc)
 	lp = shost_priv(sc->device->host);
 
 	fnic = lport_priv(lp);
-	FNIC_SCSI_DBG(KERN_DEBUG,
-		      fnic->lport->host,
-		      "Device reset called FCID 0x%x, LUN 0x%x\n",
-		      
 (starget_to_rport(scsi_target(sc->device)))->port_id,
-		      sc->device->lun);
 
+	rport = starget_to_rport(scsi_target(sc->device));
+	FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
+			"Device reset called FCID 0x%x, LUN 0x%x\n",
+			rport->port_id, sc->device->lun);
 
 	if (lp->state != LPORT_ST_READY || !(lp->link_up))
 		goto fnic_device_reset_end;
 
 	/* Check if remote port up */
-	rport = starget_to_rport(scsi_target(sc->device));
 	if (fc_remote_port_chkready(rport))
 		goto fnic_device_reset_end;