Mailing List Archive

On Thu, 2013-08-22 at 08:44 +0800, zwu.kernel@gmail.com wrote:
> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>
> 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(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cb4fefa..58375c3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -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);


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

HI,

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

On Thu, Aug 22, 2013 at 9:02 AM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2013-08-22 at 08:44 +0800, zwu.kernel@gmail.com wrote:
>> From: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
>>
>> 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(-)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index cb4fefa..58375c3 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -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);
>
>



--
Regards,

Zhi Yong Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, 2013-08-22 at 09:05 +0800, Zhi Yong Wu wrote:
> HI,

Hi.

> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

>>>>> "Joe" == Joe Perches <joe@perches.com> 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.

--
Martin K. Petersen Oracle Linux Engineering
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Wed, 2013-08-21 at 21:25 -0400, Martin K. Petersen wrote:
> >>>>> "Joe" == Joe Perches <joe@perches.com> 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(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index cb4fefa..6fc2831 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -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;
}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, Aug 22, 2013 at 9:55 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2013-08-21 at 21:25 -0400, Martin K. Petersen wrote:
>> >>>>> "Joe" == Joe Perches <joe@perches.com> 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(-)
>
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index cb4fefa..6fc2831 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -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.
> printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n");
> return -EINVAL;
> }
>
>



--
Regards,

Zhi Yong Wu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, 2013-08-22 at 09:59 +0800, Zhi Yong Wu wrote:
> On Thu, Aug 22, 2013 at 9:55 AM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2013-08-21 at 21:25 -0400, Martin K. Petersen wrote:
> >> >>>>> "Joe" == Joe Perches <joe@perches.com> 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(-)
> >
> > diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> > index cb4fefa..6fc2831 100644
> > --- a/drivers/scsi/scsi_debug.c
> > +++ b/drivers/scsi/scsi_debug.c
> > @@ -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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

2013/8/22 Martin K. Petersen <martin.petersen@oracle.com>:
>>>>>> "Joe" == Joe Perches <joe@perches.com> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, 2013-08-22 at 21:42 +0900, Akinobu Mita wrote:
> 2013/8/22 Martin K. Petersen <martin.petersen@oracle.com>:
> >>>>>> "Joe" == Joe Perches <joe@perches.com> 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

2013/8/22 James Bottomley <jbottomley@parallels.com>:
> On Thu, 2013-08-22 at 21:42 +0900, Akinobu Mita wrote:
>> 2013/8/22 Martin K. Petersen <martin.petersen@oracle.com>:
>> >>>>>> "Joe" == Joe Perches <joe@perches.com> 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.
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Thu, 2013-08-22 at 23:49 +0900, Akinobu Mita wrote:
> 2013/8/22 James Bottomley <jbottomley@parallels.com>:
> > On Thu, 2013-08-22 at 21:42 +0900, Akinobu Mita wrote:
> >> 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.

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

2013/9/20 Paul Bolle <pebolle@tiscali.nl>:
> On Thu, 2013-08-22 at 23:49 +0900, Akinobu Mita wrote:
>> 2013/8/22 James Bottomley <jbottomley@parallels.com>:
>> > On Thu, 2013-08-22 at 21:42 +0900, Akinobu Mita wrote:
>> >> 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.
>
> 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?

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

On Fri, 2013-09-20 at 23:32 +0900, Akinobu Mita wrote:
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/