Skip to contentSkip to conversation

remove redundant BUG_ON

PATCH In review Roel Kluin 2 participants 3 messages Jun 22, 2009

Subsystems (2)
SCSI Cisco FCoE HBA driver
lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 remove redundant BUG_ON

sg_count is unsigned. If negative, a wrap causes the second BUG_ON to trigger.

scsi_dma_map may return -ENOMEM, sg_count should be int to catch that.

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

---
Please use this one instead.

drivers/scsi/fnic/fnic_scsi.c +1 -2
index eabf365..22a7cd5 100644
@@ -260,7 +260,6 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
 	char msg[2];
 
 	if (sg_count) {
-		BUG_ON(sg_count < 0);
 		BUG_ON(sg_count > FNIC_MAX_SG_DESC_CNT);
 
 		/* For each SGE, create a device desc entry */

@@ -344,7 +343,7 @@ int fnic_queuecommand(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 	struct fnic *fnic;
 	struct vnic_wq_copy *wq;
 	int ret;
-	u32 sg_count;
+	int sg_count;
 	unsigned long flags;
 	unsigned long ptr;
#1 lore raw
Quoted 32 lines

-----Original Message-----
From: Roel Kluin [mailto:[email protected]]
Sent: Monday, June 22, 2009 10:55 AM
To: Abhijeet Joglekar (abjoglek)
Cc: [email protected]; Andrew Morton
Subject: [PATCH] remove redundant BUG_ON

sg_count is unsigned. If negative, a wrap causes the second BUG_ON to trigger.

scsi_dma_map may return -ENOMEM, sg_count should be int to catch that.

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

---
Please use this one instead.

(quoted diff) +0 -0

b/drivers/scsi/fnic/fnic_scsi.c index eabf365..22a7cd5 100644

drivers/scsi/fnic/fnic_scsi.c +0 -1
@@ -260,7 +260,6 @@ static inline int 
 fnic_queue_wq_copy_desc(struct fnic *fnic,
 	char msg[2];
 
 	if (sg_count) {
-		BUG_ON(sg_count < 0);
 		BUG_ON(sg_count > FNIC_MAX_SG_DESC_CNT);
drivers/scsi/fnic/fnic_scsi.c +0 -0
 		/* For each SGE, create a device desc entry */

drivers/scsi/fnic/fnic_scsi.c int fnic_queuecommand(struct scsi_cmnd · line 349

Quoted 8 lines
drivers/scsi/fnic/fnic_scsi.c +1 -1
@@ -344,7 +343,7 @@ int fnic_queuecommand(struct scsi_cmnd 
 *sc, void (*done)(struct scsi_cmnd *))
 	struct fnic *fnic;
 	struct vnic_wq_copy *wq;
 	int ret;
-	u32 sg_count;
+	int sg_count;
 	unsigned long flags;
 	unsigned long ptr;

Roel,

The fix looks good. If dma_map_sg() would have failed, bad things would happen since sg_count was u32. Good catch!

It will be better to change the sg_count parameter passed to fnic_queue_wq_copy_desc to int too, so that it is consistent between the 2 functions. Do you want to add that to your patch or you want me to send a patch for that?

The BUG_ON(sg_count < 0) can be dropped even though sg_count is an int now, since fnic_queuecommand() checks for sg_count less than zero after call to dma_map_sg.

Thanks
-- abhijeet

author
#2 lore raw

PATCH 1/1 fnic: remove redundant BUG_ONs and fix checks on unsigned

In reply to Roel Kluin

The shost sg tablesize is set to FNIC_MAX_SG_DESC_CNT and fnic uses scsi_dma_map, so both BUG_ONs can be removed.

scsi_dma_map may return -ENOMEM, sg_count should be int to catch that.

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

---

drivers/scsi/fnic/fnic_scsi.c +2 -5
index eabf365..bfc9969 100644
@@ -245,7 +245,7 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
 					  struct vnic_wq_copy *wq,
 					  struct fnic_io_req *io_req,
 					  struct scsi_cmnd *sc,
-					  u32 sg_count)
+					  int sg_count)
 {
 	struct scatterlist *sg;
 	struct fc_rport *rport = starget_to_rport(scsi_target(sc->device));

@@ -260,9 +260,6 @@ static inline int fnic_queue_wq_copy_desc(struct fnic *fnic,
 	char msg[2];
 
 	if (sg_count) {
-		BUG_ON(sg_count < 0);
-		BUG_ON(sg_count > FNIC_MAX_SG_DESC_CNT);
-
 		/* For each SGE, create a device desc entry */
 		desc = io_req->sgl_list;
 		for_each_sg(scsi_sglist(sc), sg, sg_count, i) {

@@ -344,7 +341,7 @@ int fnic_queuecommand(struct scsi_cmnd *sc, void (*done)(struct scsi_cmnd *))
 	struct fnic *fnic;
 	struct vnic_wq_copy *wq;
 	int ret;
-	u32 sg_count;
+	int sg_count;
 	unsigned long flags;
 	unsigned long ptr;