scsi: fix the build warning

PATCH In review [email protected] 6 participants 14 messages Aug 22, 2013 – Sep 20, 2013 SCSI

lore

Share & tools

lore

[email protected]

author

2013-08-22 00:46 UTC #0 lore raw

PATCH 1/1 scsi: fix the build warning

From: Zhi Yong Wu <[email protected]>

Initialize csum variable to fix the build warning.

drivers/scsi/scsi_debug.c: In function ‘dif_verify’:
drivers/scsi/scsi_debug.c:1755:3: warning: ‘csum’ may be used uninitialized in this function [-Wmaybe-uninitialized]
LD drivers/built-in.o
CHK include/generated/uapi/linux/version.h
make[2]: Nothing to be done for `all'.
make[2]: Nothing to be done for `relocs'.

Signed-off-by Zhi Yong Wu <[email protected]>

---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/scsi_debug.c +1 -1

index cb4fefa..50d70c3 100644
@@ -1733,7 +1733,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 
 static u16 dif_compute_csum(const void *buf, int len)
 {
-	u16 csum;
+	u16 csum = -1;
 
 	switch (scsi_debug_guard) {
 	case 1:

Signature · 6 lines

1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Joe Perches

2013-08-22 01:02 UTC #1 lore raw

Quoted 3 lines · On Thu, 2013-08-22 at 08:44 +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>
Initialize csum variable to fix the build warning.

Maybe it'd be better to change the variable
scsi_debug_guard type to bool?

Something like:

---
 drivers/scsi/scsi_debug.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

drivers/scsi/scsi_debug.c +7 ~~-15~~

index cb4fefa..58375c3 100644
@@ -97,7 +97,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_D_SENSE   0
 #define DEF_EVERY_NTH   0
 #define DEF_FAKE_RW	0
-#define DEF_GUARD 0
+#define DEF_GUARD false
 #define DEF_LBPU 0
 #define DEF_LBPWS 0
 #define DEF_LBPWS10 0

@@ -169,7 +169,7 @@ static int scsi_debug_dix = DEF_DIX;
 static int scsi_debug_dsense = DEF_D_SENSE;
 static int scsi_debug_every_nth = DEF_EVERY_NTH;
 static int scsi_debug_fake_rw = DEF_FAKE_RW;
-static int scsi_debug_guard = DEF_GUARD;
+static bool scsi_debug_guard = DEF_GUARD;
 static int scsi_debug_lowest_aligned = DEF_LOWEST_ALIGNED;
 static int scsi_debug_max_luns = DEF_MAX_LUNS;
 static int scsi_debug_max_queue = SCSI_DEBUG_CANQUEUE;

@@ -1735,14 +1735,11 @@ static u16 dif_compute_csum(const void *buf, int len)
 {
 	u16 csum;
 
-	switch (scsi_debug_guard) {
-	case 1:
+	if (scsi_debug_guard)
 		csum = ip_compute_csum(buf, len);
-		break;
-	case 0:
+	else
 		csum = cpu_to_be16(crc_t10dif(buf, len));
-		break;
-	}
+
 	return csum;
 }

@@ -2736,7 +2733,7 @@ module_param_named(dix, scsi_debug_dix, int, S_IRUGO);
 module_param_named(dsense, scsi_debug_dsense, int, S_IRUGO | S_IWUSR);
 module_param_named(every_nth, scsi_debug_every_nth, int, S_IRUGO | S_IWUSR);
 module_param_named(fake_rw, scsi_debug_fake_rw, int, S_IRUGO | S_IWUSR);
-module_param_named(guard, scsi_debug_guard, int, S_IRUGO);
+module_param_named(guard, scsi_debug_guard, bool, S_IRUGO);
 module_param_named(lbpu, scsi_debug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, scsi_debug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, scsi_debug_lbpws10, int, S_IRUGO);

@@ -3312,11 +3309,6 @@ static int __init scsi_debug_init(void)
 		return -EINVAL;
 	}
 
-	if (scsi_debug_guard > 1) {
-		printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n");
-		return -EINVAL;
-	}
-
 	if (scsi_debug_ato > 1) {
 		printk(KERN_ERR "scsi_debug_init: ato must be 0 or 1\n");
 		return -EINVAL;

@@ -4028,7 +4020,7 @@ static int sdebug_driver_probe(struct device * dev)
 	       (host_prot & SHOST_DIX_TYPE2_PROTECTION) ? " DIX2" : "",
 	       (host_prot & SHOST_DIX_TYPE3_PROTECTION) ? " DIX3" : "");
 
-	if (scsi_debug_guard == 1)
+	if (scsi_debug_guard)
 		scsi_host_set_guard(hpnt, SHOST_DIX_GUARD_IP);
 	else
 		scsi_host_set_guard(hpnt, SHOST_DIX_GUARD_CRC);

Zhi Yong Wu

author

2013-08-22 01:05 UTC #2 lore raw

HI,

If you'd like, you should draft one patch for this warning.

Quoted 85 lines · Joe Perches

Quoted 3 lines · On Thu, 2013-08-22 at 08:44 +0800, [email protected] wrote:

From: Zhi Yong Wu <[email protected]>
Initialize csum variable to fix the build warning.

Maybe it'd be better to change the variable
scsi_debug_guard type to bool?

Something like:

---
 drivers/scsi/scsi_debug.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

drivers/scsi/scsi_debug.c +7 ~~-15~~

index cb4fefa..58375c3 100644
@@ -97,7 +97,7 @@ static const char * scsi_debug_version_date = "20100324";
 #define DEF_D_SENSE   0
 #define DEF_EVERY_NTH   0
 #define DEF_FAKE_RW    0
-#define DEF_GUARD 0
+#define DEF_GUARD false
 #define DEF_LBPU 0
 #define DEF_LBPWS 0
 #define DEF_LBPWS10 0

@@ -169,7 +169,7 @@ static int scsi_debug_dix = DEF_DIX;
 static int scsi_debug_dsense = DEF_D_SENSE;
 static int scsi_debug_every_nth = DEF_EVERY_NTH;
 static int scsi_debug_fake_rw = DEF_FAKE_RW;
-static int scsi_debug_guard = DEF_GUARD;
+static bool scsi_debug_guard = DEF_GUARD;
 static int scsi_debug_lowest_aligned = DEF_LOWEST_ALIGNED;
 static int scsi_debug_max_luns = DEF_MAX_LUNS;
 static int scsi_debug_max_queue = SCSI_DEBUG_CANQUEUE;

@@ -1735,14 +1735,11 @@ static u16 dif_compute_csum(const void *buf, int len)
 {
        u16 csum;
 
-       switch (scsi_debug_guard) {
-       case 1:
+       if (scsi_debug_guard)
                csum = ip_compute_csum(buf, len);
-               break;
-       case 0:
+       else
                csum = cpu_to_be16(crc_t10dif(buf, len));
-               break;
-       }
+
        return csum;
 }

@@ -2736,7 +2733,7 @@ module_param_named(dix, scsi_debug_dix, int, S_IRUGO);
 module_param_named(dsense, scsi_debug_dsense, int, S_IRUGO | S_IWUSR);
 module_param_named(every_nth, scsi_debug_every_nth, int, S_IRUGO | S_IWUSR);
 module_param_named(fake_rw, scsi_debug_fake_rw, int, S_IRUGO | S_IWUSR);
-module_param_named(guard, scsi_debug_guard, int, S_IRUGO);
+module_param_named(guard, scsi_debug_guard, bool, S_IRUGO);
 module_param_named(lbpu, scsi_debug_lbpu, int, S_IRUGO);
 module_param_named(lbpws, scsi_debug_lbpws, int, S_IRUGO);
 module_param_named(lbpws10, scsi_debug_lbpws10, int, S_IRUGO);

@@ -3312,11 +3309,6 @@ static int __init scsi_debug_init(void)
                return -EINVAL;
        }
 
-       if (scsi_debug_guard > 1) {
-               printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n");
-               return -EINVAL;
-       }
-
        if (scsi_debug_ato > 1) {
                printk(KERN_ERR "scsi_debug_init: ato must be 0 or 1\n");
                return -EINVAL;

@@ -4028,7 +4020,7 @@ static int sdebug_driver_probe(struct device * dev)
               (host_prot & SHOST_DIX_TYPE2_PROTECTION) ? " DIX2" : "",
               (host_prot & SHOST_DIX_TYPE3_PROTECTION) ? " DIX3" : "");
 
-       if (scsi_debug_guard == 1)
+       if (scsi_debug_guard)
                scsi_host_set_guard(hpnt, SHOST_DIX_GUARD_IP);
        else
                scsi_host_set_guard(hpnt, SHOST_DIX_GUARD_CRC);

Signature · 3 lines

Regards,

Zhi Yong Wu

Joe Perches

2013-08-22 01:17 UTC #3 lore raw

Quoted 1 line · Zhi Yong Wu

HI,

Hi.

Quoted 1 line

If you'd like, you should draft one patch for this warning.

Not for me.

I don't get this build warning in the first place and I think the scsi_debug file is quite old and probably doesn't need to be changed at all.

I think scsi_debug_guard could be set to a negative value and then the csum will always be -1.

Dunno if that's a good thing.

Anyway, just a suggestion, do what you want with it.

cheers, Joe

Martin K. Petersen

2013-08-22 01:25 UTC #4 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:

Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.

guard isn't a boolean, it selects the checksum algorithm used.

Also, I believe Akinobu's recent reorganization of this code in question fixed the warning.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering

Joe Perches

2013-08-22 01:56 UTC #5 lore raw

Quoted 7 lines · Martin K. Petersen

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.
guard isn't a boolean, it selects the checksum algorithm used.

OK, maybe this then...

---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertions(+), 1 deletions(-)

drivers/scsi/scsi_debug.c +1 -1

index cb4fefa..6fc2831 100644
@@ -3312,7 +3312,7 @@ static int __init scsi_debug_init(void)
 		return -EINVAL;
 	}
 
-	if (scsi_debug_guard > 1) {
+	if (scsi_debug_guard < 0 || scsi_debug_guard > 1) {
 		printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n");
 		return -EINVAL;
 	}

Zhi Yong Wu

author

2013-08-22 01:59 UTC #6 lore raw

Quoted 19 lines · Joe Perches

Quoted 7 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.
guard isn't a boolean, it selects the checksum algorithm used.
OK, maybe this then...
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/scsi_debug.c +0 -0
index cb4fefa..6fc2831 100644

drivers/scsi/scsi_debug.c static int __init scsi_debug_init(void) · line 3315

Quoted 5 lines

drivers/scsi/scsi_debug.c +1 -1

@@ -3312,7 +3312,7 @@ static int __init scsi_debug_init(void)
                return -EINVAL;
        }
 
-       if (scsi_debug_guard > 1) {
+       if (scsi_debug_guard < 0 || scsi_debug_guard > 1) {

I don't think that it can fix that issue.

Quoted 5 lines

drivers/scsi/scsi_debug.c +0 -0

                printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n");
                return -EINVAL;
        }

Signature · 3 lines

Regards,

Zhi Yong Wu

Joe Perches

2013-08-22 02:25 UTC #7 lore raw

Quoted 23 lines · Zhi Yong Wu

Quoted 22 lines · Joe Perches
Quoted 7 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.
guard isn't a boolean, it selects the checksum algorithm used.
OK, maybe this then...
---
 drivers/scsi/scsi_debug.c | 2 +-
 1 file changed, 1 insertions(+), 1 deletions(-)
drivers/scsi/scsi_debug.c +0 -0
index cb4fefa..6fc2831 100644
@@ -3312,7 +3312,7 @@ static int __init scsi_debug_init(void)
                return -EINVAL;
        }
 

Quoted 3 lines

drivers/scsi/scsi_debug.c static int __init scsi_debug_init(void) · line 3315
Quoted 2 lines
drivers/scsi/scsi_debug.c +1 -1
-       if (scsi_debug_guard > 1) {
+       if (scsi_debug_guard < 0 || scsi_debug_guard > 1) {
I don't think that it can fix that issue.

No, it doesn't fix any compile warning. Your patch, if it's needed, would still apply.

This patch suggestion fixes an issue where debug_guard could be set to a negative value.

I didn't sign it, it's up to Martin or I suppose James to actually want it done.

cheers, Joe

Akinobu Mita

2013-08-22 12:42 UTC #8 lore raw

In reply to Martin K. Petersen

2013/8/22 Martin K. Petersen <[email protected]>:

Quoted 10 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.
guard isn't a boolean, it selects the checksum algorithm used.
Also, I believe Akinobu's recent reorganization of this code in question fixed the warning.

Unfortunately, this warning isn't fixed in linux-next, either. Paul Bolle also sent a patch that fixes the same warning in a little bit different way.

http://marc.info/?l=linux-scsi&m=137404660520109&w=2

James Bottomley

2013-08-22 14:27 UTC #9 lore raw

Quoted 7 lines · Akinobu Mita

2013/8/22 Martin K. Petersen <[email protected]>:
Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.

Quoted 8 lines

Quoted 4 lines
guard isn't a boolean, it selects the checksum algorithm used.
Also, I believe Akinobu's recent reorganization of this code in question fixed the warning.
Unfortunately, this warning isn't fixed in linux-next, either. Paul Bolle also sent a patch that fixes the same warning in a little bit different way.

Well, it is and it isn't. Whether you see the warning seems to depend on how gcc was built. My take is that an impossible default case just to keep some versions of gcc quiet is a bit pointless.

James

Akinobu Mita

2013-08-22 14:49 UTC #10 lore raw

2013/8/22 James Bottomley <[email protected]>:

Quoted 13 lines · Akinobu Mita

Quoted 12 lines
2013/8/22 Martin K. Petersen <[email protected]>:
Quoted 10 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Joe" == Joe Perches <[email protected]> writes:
Joe> I don't get this build warning in the first place and I think the Joe> scsi_debug file is quite old and probably doesn't need to be Joe> changed at all.
guard isn't a boolean, it selects the checksum algorithm used.
Also, I believe Akinobu's recent reorganization of this code in question fixed the warning.

Quoted 7 lines

Quoted 3 lines
Unfortunately, this warning isn't fixed in linux-next, either. Paul Bolle also sent a patch that fixes the same warning in a little bit different way.
Well, it is and it isn't. Whether you see the warning seems to depend on how gcc was built. My take is that an impossible default case just to keep some versions of gcc quiet is a bit pointless.

As Joe said in the other reply, scsi_debug_guard could be a negative value (scsi_debug_guard > 1 is only prohibited). So this warning does not seem a false positive.

Paul Bolle

2013-09-20 09:53 UTC #11 lore raw

Quoted 6 lines · Akinobu Mita

2013/8/22 James Bottomley <[email protected]>:
Quoted 5 lines · Akinobu Mita
Quoted 3 lines
Unfortunately, this warning isn't fixed in linux-next, either. Paul Bolle also sent a patch that fixes the same warning in a little bit different way.

Quoted 7 lines

Quoted 3 lines
Well, it is and it isn't. Whether you see the warning seems to depend on how gcc was built. My take is that an impossible default case just to keep some versions of gcc quiet is a bit pointless.
As Joe said in the other reply, scsi_debug_guard could be a negative value (scsi_debug_guard > 1 is only prohibited). So this warning does not seem a false positive.

I too think that GCC is correct here. Perhaps the people not seeing this warning don't have CONFIG_SCSI_DEBUG set.

A week ago Antonia also submitted a patch to silence this warning ( https://lkml.org/lkml/2013/9/13/649 ). That's at least the third time someone tried to silence it since it got introduced in the v3.11 cycle.

Akinobu, could you please say how you'd like this warning to be silenced? Or is an actual fix queued somewhere?

Paul Bolle

Akinobu Mita

2013-09-20 14:32 UTC #12 lore raw

2013/9/20 Paul Bolle <[email protected]>:

Quoted 18 lines · Akinobu Mita

Quoted 6 lines
2013/8/22 James Bottomley <[email protected]>:
Quoted 5 lines · Akinobu Mita
Quoted 3 lines
Unfortunately, this warning isn't fixed in linux-next, either. Paul Bolle also sent a patch that fixes the same warning in a little bit different way.
Quoted 7 lines
Quoted 3 lines
Well, it is and it isn't. Whether you see the warning seems to depend on how gcc was built. My take is that an impossible default case just to keep some versions of gcc quiet is a bit pointless.
As Joe said in the other reply, scsi_debug_guard could be a negative value (scsi_debug_guard > 1 is only prohibited). So this warning does not seem a false positive.
I too think that GCC is correct here. Perhaps the people not seeing this warning don't have CONFIG_SCSI_DEBUG set.

Quoted 6 lines

A week ago Antonia also submitted a patch to silence this warning ( https://lkml.org/lkml/2013/9/13/649 ). That's at least the third time someone tried to silence it since it got introduced in the v3.11 cycle.
Akinobu, could you please say how you'd like this warning to be silenced? Or is an actual fix queued somewhere?

Yesterday, I sent a patch set which includes two fixes for this issue. I wish this to be merged and I'll do my best.

http://marc.info/?l=linux-scsi&m=137950732530325&w=2
http://marc.info/?l=linux-scsi&m=137950732530326&w=2

The first one prevents scsi_debug_guard from being a negative value by changing the type of scsi_debug_guard to 'unsigned int'.

The second one is actually titled sparse warning fix, but it also silences this GCC warning by chaning from switch statement to if/else statements.

Paul Bolle

2013-09-20 16:14 UTC #13 lore raw

Quoted 2 lines · Akinobu Mita

Yesterday, I sent a patch set which includes two fixes for this issue. I wish this to be merged and I'll do my best.

I hadn't yet stumbled onto these patches. Thanks!

Paul Bolle