* [PATCH] remove redundant BUG_ON
@ 2009-06-22 17:54 Roel Kluin
2009-06-22 18:15 ` Abhijeet Joglekar (abjoglek)
2009-06-22 23:06 ` [PATCH] fnic: remove redundant BUG_ONs and fix checks on unsigned Roel Kluin
0 siblings, 2 replies; 5+ messages in thread
From: Roel Kluin @ 2009-06-22 17:54 UTC (permalink / raw)
To: abjoglek; +Cc: linux-scsi, Andrew Morton
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 <roel.kluin@gmail.com>
---
Please use this one instead.
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index eabf365..22a7cd5 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* RE: [PATCH] remove redundant BUG_ON
2009-06-22 17:54 [PATCH] remove redundant BUG_ON Roel Kluin
@ 2009-06-22 18:15 ` Abhijeet Joglekar (abjoglek)
2009-06-22 23:06 ` [PATCH] fnic: remove redundant BUG_ONs and fix checks on unsigned Roel Kluin
1 sibling, 0 replies; 5+ messages in thread
From: Abhijeet Joglekar (abjoglek) @ 2009-06-22 18:15 UTC (permalink / raw)
To: Roel Kluin; +Cc: linux-scsi, Andrew Morton
> -----Original Message-----
> From: Roel Kluin [mailto:roel.kluin@gmail.com]
> Sent: Monday, June 22, 2009 10:55 AM
> To: Abhijeet Joglekar (abjoglek)
> Cc: linux-scsi@vger.kernel.org; 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 <roel.kluin@gmail.com>
> ---
> Please use this one instead.
>
> diff --git a/drivers/scsi/fnic/fnic_scsi.c
> b/drivers/scsi/fnic/fnic_scsi.c index eabf365..22a7cd5 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -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;
>
>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] fnic: remove redundant BUG_ONs and fix checks on unsigned
2009-06-22 17:54 [PATCH] remove redundant BUG_ON Roel Kluin
2009-06-22 18:15 ` Abhijeet Joglekar (abjoglek)
@ 2009-06-22 23:06 ` Roel Kluin
1 sibling, 0 replies; 5+ messages in thread
From: Roel Kluin @ 2009-06-22 23:06 UTC (permalink / raw)
To: abjoglek; +Cc: linux-scsi, michaelc, Andrew Morton
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 <roel.kluin@gmail.com>
---
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index eabf365..bfc9969 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] remove redundant BUG_ON
@ 2009-06-22 17:43 Roel Kluin
2009-06-22 16:02 ` Mike Christie
0 siblings, 1 reply; 5+ messages in thread
From: Roel Kluin @ 2009-06-22 17:43 UTC (permalink / raw)
To: abjoglek; +Cc: linux-scsi, Andrew Morton
sg_count is unsigned so if negative a wrap occurs and the second BUG_ON
catches it.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
index eabf365..c57cff4 100644
--- a/drivers/scsi/fnic/fnic_scsi.c
+++ b/drivers/scsi/fnic/fnic_scsi.c
@@ -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 */
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] remove redundant BUG_ON
2009-06-22 17:43 [PATCH] remove redundant BUG_ON Roel Kluin
@ 2009-06-22 16:02 ` Mike Christie
0 siblings, 0 replies; 5+ messages in thread
From: Mike Christie @ 2009-06-22 16:02 UTC (permalink / raw)
To: Roel Kluin; +Cc: abjoglek, linux-scsi, Andrew Morton
Roel Kluin wrote:
> sg_count is unsigned so if negative a wrap occurs and the second BUG_ON
> catches it.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index eabf365..c57cff4 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -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);
>
I think they both could go. The shost sg tablesize is set to
FNIC_MAX_SG_DESC_CNT and fnic uses scsi_dma_map, so the block and scsi
layer should be doing the right thing. If we are worried about them,
then maybe we should add BUG_ONs in the common code to check for all
drivers.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-06-22 21:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-22 17:54 [PATCH] remove redundant BUG_ON Roel Kluin
2009-06-22 18:15 ` Abhijeet Joglekar (abjoglek)
2009-06-22 23:06 ` [PATCH] fnic: remove redundant BUG_ONs and fix checks on unsigned Roel Kluin
-- strict thread matches above, loose matches on Subject: below --
2009-06-22 17:43 [PATCH] remove redundant BUG_ON Roel Kluin
2009-06-22 16:02 ` Mike Christie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.