* [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
@ 2012-01-10 23:42 akpm
2012-01-11 1:18 ` adam radford
0 siblings, 1 reply; 6+ messages in thread
From: akpm @ 2012-01-10 23:42 UTC (permalink / raw)
To: James.Bottomley; +Cc: linux-scsi, akpm, rdunlap, megaraidlinux, viro
From: Randy Dunlap <rdunlap@xenotime.net>
Subject: drivers/scsi/megaraid.c: fix sparse warnings
Fix sparse warnings of right shift bigger than source value size:
drivers/scsi/megaraid.c:311:65: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:313:65: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:317:67: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:319:67: warning: right shift by bigger than source value
Patch suggestion from email by Al Viro:
"Since both are claimed to be strings, I really suspect that this >> 8 is
misspelled >> 4 and they have a character followed by pair of two-digit
packed decimals in there..."
Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/scsi/megaraid.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff -puN drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings drivers/scsi/megaraid.c
--- a/drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings
+++ a/drivers/scsi/megaraid.c
@@ -310,15 +310,15 @@ mega_query_adapter(adapter_t *adapter)
if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
sprintf (adapter->fw_version, "%c%d%d.%d%d",
adapter->product_info.fw_version[2],
- adapter->product_info.fw_version[1] >> 8,
+ adapter->product_info.fw_version[1] >> 4,
adapter->product_info.fw_version[1] & 0x0f,
- adapter->product_info.fw_version[0] >> 8,
+ adapter->product_info.fw_version[0] >> 4,
adapter->product_info.fw_version[0] & 0x0f);
sprintf (adapter->bios_version, "%c%d%d.%d%d",
adapter->product_info.bios_version[2],
- adapter->product_info.bios_version[1] >> 8,
+ adapter->product_info.bios_version[1] >> 4,
adapter->product_info.bios_version[1] & 0x0f,
- adapter->product_info.bios_version[0] >> 8,
+ adapter->product_info.bios_version[0] >> 4,
adapter->product_info.bios_version[0] & 0x0f);
} else {
memcpy(adapter->fw_version,
_
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
2012-01-10 23:42 [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings akpm
@ 2012-01-11 1:18 ` adam radford
2012-01-11 1:25 ` James Bottomley
0 siblings, 1 reply; 6+ messages in thread
From: adam radford @ 2012-01-11 1:18 UTC (permalink / raw)
To: akpm; +Cc: James.Bottomley, linux-scsi, rdunlap, megaraidlinux, viro
On Tue, Jan 10, 2012 at 3:42 PM, <akpm@linux-foundation.org> wrote:
> From: Randy Dunlap <rdunlap@xenotime.net>
> Subject: drivers/scsi/megaraid.c: fix sparse warnings
>
> Fix sparse warnings of right shift bigger than source value size:
>
> drivers/scsi/megaraid.c:311:65: warning: right shift by bigger than source value
> drivers/scsi/megaraid.c:313:65: warning: right shift by bigger than source value
> drivers/scsi/megaraid.c:317:67: warning: right shift by bigger than source value
> drivers/scsi/megaraid.c:319:67: warning: right shift by bigger than source value
>
> Patch suggestion from email by Al Viro:
>
> "Since both are claimed to be strings, I really suspect that this >> 8 is
> misspelled >> 4 and they have a character followed by pair of two-digit
> packed decimals in there..."
>
> Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> drivers/scsi/megaraid.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff -puN drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings drivers/scsi/megaraid.c
> --- a/drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings
> +++ a/drivers/scsi/megaraid.c
> @@ -310,15 +310,15 @@ mega_query_adapter(adapter_t *adapter)
> if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
> sprintf (adapter->fw_version, "%c%d%d.%d%d",
> adapter->product_info.fw_version[2],
> - adapter->product_info.fw_version[1] >> 8,
> + adapter->product_info.fw_version[1] >> 4,
> adapter->product_info.fw_version[1] & 0x0f,
> - adapter->product_info.fw_version[0] >> 8,
> + adapter->product_info.fw_version[0] >> 4,
> adapter->product_info.fw_version[0] & 0x0f);
> sprintf (adapter->bios_version, "%c%d%d.%d%d",
> adapter->product_info.bios_version[2],
> - adapter->product_info.bios_version[1] >> 8,
> + adapter->product_info.bios_version[1] >> 4,
> adapter->product_info.bios_version[1] & 0x0f,
> - adapter->product_info.bios_version[0] >> 8,
> + adapter->product_info.bios_version[0] >> 4,
> adapter->product_info.bios_version[0] & 0x0f);
> } else {
> memcpy(adapter->fw_version,
After a huge hunt, I was able to locate a single parallel scsi board
that this driver works with, however it has LSI firmware on it. I am
unable to find the HP firmware for my board, which this code change
affects. Additionally, checking hp.com for firmware for my controller
was unsuccessful.
Changing the driver version string for HP firmware on this board in
_any way_ other than what was intended will affect the following code
in the driver that does firmware string matching:
if ((subsysvid == HP_SUBSYS_VID) &&
((subsysid == 0x60E7) || (subsysid == 0x60E8))) {
/*
* which firmware
*/
if (!strcmp(adapter->fw_version, "H01.07") ||
!strcmp(adapter->fw_version, "H01.08") ||
!strcmp(adapter->fw_version, "H01.09") ) {
printk(KERN_WARNING
"megaraid: Firmware H.01.07, "
"H.01.08, and H.01.09 on 1M/2M "
"controllers\n"
"megaraid: do not support 64 bit "
"addressing.\nmegaraid: DISABLING "
"64 bit support.\n");
adapter->flag &= ~BOARD_64BIT;
}
}
Since I have no HP firmware on one of these boards, and I can see that
altering the firmware string will alter the behvior of the above code
WRT setting/clearing BOARD_64BIT in the adapter->flag field, I have to
NACK this patch. I hope we can live with the sparse warning since I
have no way to test this.
-Adam
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
2012-01-11 1:18 ` adam radford
@ 2012-01-11 1:25 ` James Bottomley
2012-01-11 1:48 ` Andrew Morton
0 siblings, 1 reply; 6+ messages in thread
From: James Bottomley @ 2012-01-11 1:25 UTC (permalink / raw)
To: adam radford; +Cc: akpm, linux-scsi, rdunlap, megaraidlinux, viro
On Tue, 2012-01-10 at 17:18 -0800, adam radford wrote:
> On Tue, Jan 10, 2012 at 3:42 PM, <akpm@linux-foundation.org> wrote:
> > From: Randy Dunlap <rdunlap@xenotime.net>
> > Subject: drivers/scsi/megaraid.c: fix sparse warnings
> >
> > Fix sparse warnings of right shift bigger than source value size:
> >
> > drivers/scsi/megaraid.c:311:65: warning: right shift by bigger than source value
> > drivers/scsi/megaraid.c:313:65: warning: right shift by bigger than source value
> > drivers/scsi/megaraid.c:317:67: warning: right shift by bigger than source value
> > drivers/scsi/megaraid.c:319:67: warning: right shift by bigger than source value
> >
> > Patch suggestion from email by Al Viro:
> >
> > "Since both are claimed to be strings, I really suspect that this >> 8 is
> > misspelled >> 4 and they have a character followed by pair of two-digit
> > packed decimals in there..."
> >
> > Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
> > Cc: Al Viro <viro@ZenIV.linux.org.uk>
> > Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > ---
> >
> > drivers/scsi/megaraid.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff -puN drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings drivers/scsi/megaraid.c
> > --- a/drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings
> > +++ a/drivers/scsi/megaraid.c
> > @@ -310,15 +310,15 @@ mega_query_adapter(adapter_t *adapter)
> > if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
> > sprintf (adapter->fw_version, "%c%d%d.%d%d",
> > adapter->product_info.fw_version[2],
> > - adapter->product_info.fw_version[1] >> 8,
> > + adapter->product_info.fw_version[1] >> 4,
> > adapter->product_info.fw_version[1] & 0x0f,
> > - adapter->product_info.fw_version[0] >> 8,
> > + adapter->product_info.fw_version[0] >> 4,
> > adapter->product_info.fw_version[0] & 0x0f);
> > sprintf (adapter->bios_version, "%c%d%d.%d%d",
> > adapter->product_info.bios_version[2],
> > - adapter->product_info.bios_version[1] >> 8,
> > + adapter->product_info.bios_version[1] >> 4,
> > adapter->product_info.bios_version[1] & 0x0f,
> > - adapter->product_info.bios_version[0] >> 8,
> > + adapter->product_info.bios_version[0] >> 4,
> > adapter->product_info.bios_version[0] & 0x0f);
> > } else {
> > memcpy(adapter->fw_version,
>
> After a huge hunt, I was able to locate a single parallel scsi board
> that this driver works with, however it has LSI firmware on it. I am
> unable to find the HP firmware for my board, which this code change
> affects. Additionally, checking hp.com for firmware for my controller
> was unsuccessful.
>
> Changing the driver version string for HP firmware on this board in
> _any way_ other than what was intended will affect the following code
> in the driver that does firmware string matching:
>
> if ((subsysvid == HP_SUBSYS_VID) &&
> ((subsysid == 0x60E7) || (subsysid == 0x60E8))) {
> /*
> * which firmware
> */
> if (!strcmp(adapter->fw_version, "H01.07") ||
> !strcmp(adapter->fw_version, "H01.08") ||
> !strcmp(adapter->fw_version, "H01.09") ) {
> printk(KERN_WARNING
> "megaraid: Firmware H.01.07, "
> "H.01.08, and H.01.09 on 1M/2M "
> "controllers\n"
> "megaraid: do not support 64 bit "
> "addressing.\nmegaraid: DISABLING "
> "64 bit support.\n");
> adapter->flag &= ~BOARD_64BIT;
> }
> }
>
> Since I have no HP firmware on one of these boards, and I can see that
> altering the firmware string will alter the behvior of the above code
> WRT setting/clearing BOARD_64BIT in the adapter->flag field, I have to
> NACK this patch. I hope we can live with the sparse warning since I
> have no way to test this.
So I think the title of the patch is misleading. The point is that
adapter->product_info.fw_version[] >> 8
Always produces zero since adapter->product_info.fw_version is a char[].
It seems that the idea was
If you want to supply an update that just making the zero explicit for
the firmware version (and thus fixes the sparse warning) is fine too ...
Al just thought that you meant to take the firmware version as BCD
nybbles.
James
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
2012-01-11 1:25 ` James Bottomley
@ 2012-01-11 1:48 ` Andrew Morton
2012-01-11 2:07 ` adam radford
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Morton @ 2012-01-11 1:48 UTC (permalink / raw)
To: James Bottomley; +Cc: adam radford, linux-scsi, rdunlap, megaraidlinux, viro
On Tue, 10 Jan 2012 20:25:38 -0500 James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > Since I have no HP firmware on one of these boards, and I can see that
> > altering the firmware string will alter the behvior of the above code
> > WRT setting/clearing BOARD_64BIT in the adapter->flag field, I have to
> > NACK this patch. I hope we can live with the sparse warning since I
> > have no way to test this.
>
> So I think the title of the patch is misleading.
"fix sparse warning" is almost always a bad or wrong patch title.
Sparse warns about "X", and what we're fixing is "X", not the warning.
> The point is that
>
> adapter->product_info.fw_version[] >> 8
>
> Always produces zero since adapter->product_info.fw_version is a char[].
> It seems that the idea was
>
> If you want to supply an update that just making the zero explicit for
> the firmware version (and thus fixes the sparse warning) is fine too ...
> Al just thought that you meant to take the firmware version as BCD
> nybbles.
Yes, it sounds like this is a day-one bug which we can't really fix
now, with an acceptable level of effort or risk. I agree that replacing
it with literal zero and a decent code comment is appropriate.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
2012-01-11 1:48 ` Andrew Morton
@ 2012-01-11 2:07 ` adam radford
0 siblings, 0 replies; 6+ messages in thread
From: adam radford @ 2012-01-11 2:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: James Bottomley, linux-scsi, rdunlap, megaraidlinux, viro
[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]
On Tue, Jan 10, 2012 at 5:48 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> Yes, it sounds like this is a day-one bug which we can't really fix
> now, with an acceptable level of effort or risk. I agree that replacing
> it with literal zero and a decent code comment is appropriate.
>
James/Andrew/linux-scsi,
The following patch for the older megaraid driver fixes some sparse warnings.
Signed-off-by: Adam Radford <aradford@gmail.com>
diff -Naur scsi-misc-2.6/drivers/scsi/megaraid.c
scsi-misc-2.6.new/drivers/scsi/megaraid.c
--- scsi-misc-2.6/drivers/scsi/megaraid.c 2012-02-06 12:50:20.000000000 -0800
+++ scsi-misc-2.6.new/drivers/scsi/megaraid.c 2012-01-10
18:00:23.570267019 -0800
@@ -306,19 +306,22 @@
adapter->host->sg_tablesize = adapter->sglen;
- /* use HP firmware and bios version encoding */
+ /* use HP firmware and bios version encoding
+ Note: fw_version[0|1] and bios_version[0|1] were originally shifted
+ right 8 bits making them zero. This 0 value was hardcoded to fix
+ sparse warnings. */
if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
sprintf (adapter->fw_version, "%c%d%d.%d%d",
adapter->product_info.fw_version[2],
- adapter->product_info.fw_version[1] >> 8,
+ 0,
adapter->product_info.fw_version[1] & 0x0f,
- adapter->product_info.fw_version[0] >> 8,
+ 0,
adapter->product_info.fw_version[0] & 0x0f);
sprintf (adapter->bios_version, "%c%d%d.%d%d",
adapter->product_info.bios_version[2],
- adapter->product_info.bios_version[1] >> 8,
+ 0,
adapter->product_info.bios_version[1] & 0x0f,
- adapter->product_info.bios_version[0] >> 8,
+ 0,
adapter->product_info.bios_version[0] & 0x0f);
} else {
memcpy(adapter->fw_version,
[-- Attachment #2: megaraid.patch --]
[-- Type: text/x-patch, Size: 1301 bytes --]
diff -Naur scsi-misc-2.6/drivers/scsi/megaraid.c scsi-misc-2.6.new/drivers/scsi/megaraid.c
--- scsi-misc-2.6/drivers/scsi/megaraid.c 2012-02-06 12:50:20.000000000 -0800
+++ scsi-misc-2.6.new/drivers/scsi/megaraid.c 2012-01-10 18:00:23.570267019 -0800
@@ -306,19 +306,22 @@
adapter->host->sg_tablesize = adapter->sglen;
- /* use HP firmware and bios version encoding */
+ /* use HP firmware and bios version encoding
+ Note: fw_version[0|1] and bios_version[0|1] were originally shifted
+ right 8 bits making them zero. This 0 value was hardcoded to fix
+ sparse warnings. */
if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
sprintf (adapter->fw_version, "%c%d%d.%d%d",
adapter->product_info.fw_version[2],
- adapter->product_info.fw_version[1] >> 8,
+ 0,
adapter->product_info.fw_version[1] & 0x0f,
- adapter->product_info.fw_version[0] >> 8,
+ 0,
adapter->product_info.fw_version[0] & 0x0f);
sprintf (adapter->bios_version, "%c%d%d.%d%d",
adapter->product_info.bios_version[2],
- adapter->product_info.bios_version[1] >> 8,
+ 0,
adapter->product_info.bios_version[1] & 0x0f,
- adapter->product_info.bios_version[0] >> 8,
+ 0,
adapter->product_info.bios_version[0] & 0x0f);
} else {
memcpy(adapter->fw_version,
^ permalink raw reply [flat|nested] 6+ messages in thread
* [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings
@ 2011-11-15 22:58 akpm
0 siblings, 0 replies; 6+ messages in thread
From: akpm @ 2011-11-15 22:58 UTC (permalink / raw)
To: James.Bottomley; +Cc: linux-scsi, akpm, rdunlap, megaraidlinux, viro
From: Randy Dunlap <rdunlap@xenotime.net>
Subject: drivers/scsi/megaraid.c: fix sparse warnings
Fix sparse warnings of right shift bigger than source value size:
drivers/scsi/megaraid.c:311:65: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:313:65: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:317:67: warning: right shift by bigger than source value
drivers/scsi/megaraid.c:319:67: warning: right shift by bigger than source value
Patch suggestion from email by Al Viro:
"Since both are claimed to be strings, I really suspect that this >> 8 is
misspelled >> 4 and they have a character followed by pair of two-digit
packed decimals in there..."
Signed-off-by: Randy Dunlap <rdunlap@xenotime.net>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Neela Syam Kolli <megaraidlinux@lsi.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
drivers/scsi/megaraid.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff -puN drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings drivers/scsi/megaraid.c
--- a/drivers/scsi/megaraid.c~drivers-scsi-megaraidc-fix-sparse-warnings
+++ a/drivers/scsi/megaraid.c
@@ -310,15 +310,15 @@ mega_query_adapter(adapter_t *adapter)
if (adapter->product_info.subsysvid == HP_SUBSYS_VID) {
sprintf (adapter->fw_version, "%c%d%d.%d%d",
adapter->product_info.fw_version[2],
- adapter->product_info.fw_version[1] >> 8,
+ adapter->product_info.fw_version[1] >> 4,
adapter->product_info.fw_version[1] & 0x0f,
- adapter->product_info.fw_version[0] >> 8,
+ adapter->product_info.fw_version[0] >> 4,
adapter->product_info.fw_version[0] & 0x0f);
sprintf (adapter->bios_version, "%c%d%d.%d%d",
adapter->product_info.bios_version[2],
- adapter->product_info.bios_version[1] >> 8,
+ adapter->product_info.bios_version[1] >> 4,
adapter->product_info.bios_version[1] & 0x0f,
- adapter->product_info.bios_version[0] >> 8,
+ adapter->product_info.bios_version[0] >> 4,
adapter->product_info.bios_version[0] & 0x0f);
} else {
memcpy(adapter->fw_version,
_
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-01-11 2:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-10 23:42 [patch 2/6] drivers/scsi/megaraid.c: fix sparse warnings akpm
2012-01-11 1:18 ` adam radford
2012-01-11 1:25 ` James Bottomley
2012-01-11 1:48 ` Andrew Morton
2012-01-11 2:07 ` adam radford
-- strict thread matches above, loose matches on Subject: below --
2011-11-15 22:58 akpm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox