Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Fri, Mar 16, 2007 at 09:27:26AM -0500, James Bottomley wrote:
On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
+ err = pci_enable_device(pdev);
+ if (err 0)
+ return err;
Traditionally, this should be
if (err)
return err;
The reason is that 0 is a signed comparison which can be slightly more
expensive on some architectures and it's unnecessary if zero is the only
successful return.
That isn't a tradition that I am familiar with, but it seems reasonable
to me.
Updated patch is below.
--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/
Honour the return value of pci_enable_device(), which
seems to be a desirable thing to do:
2.6.20-rc4
gcc (GCC) 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
CC [M] drivers/message/fusion/mptbase.o
drivers/message/fusion/mptbase.c: In function `mpt_resume':
drivers/message/fusion/mptbase.c:1541: warning: ignoring return value
of `pci_enable_device', declared with attribute warn_unused_result
It also in turn has mptscsih_resume() honour the return value of
mpt_resume()
I'm not sure about the handling of the other potential error cases
in mpt_resume(), of which there appear to be many. But this does
seem to be a good start.
Signed-off-by: Simon Horman [EMAIL PROTECTED]
Index: linux-2.6/drivers/message/fusion/mptbase.c
===
--- linux-2.6.orig/drivers/message/fusion/mptbase.c 2007-03-19
10:59:18.0 +0900
+++ linux-2.6/drivers/message/fusion/mptbase.c 2007-03-19 15:04:24.0
+0900
@@ -1531,6 +1531,7 @@
MPT_ADAPTER *ioc = pci_get_drvdata(pdev);
u32 device_state = pdev-current_state;
int recovery_state;
+ int err;
printk(MYIOC_s_INFO_FMT
pci-resume: pdev=0x%p, slot=%s, Previous operating state [D%d]\n,
@@ -1538,7 +1539,9 @@
pci_set_power_state(pdev, 0);
pci_restore_state(pdev);
- pci_enable_device(pdev);
+ err = pci_enable_device(pdev);
+ if (err)
+ return err;
/* enable interrupts */
CHIPREG_WRITE32(ioc-chip-IntMask, MPI_HIM_DIM);
Index: linux-2.6/drivers/message/fusion/mptscsih.c
===
--- linux-2.6.orig/drivers/message/fusion/mptscsih.c2007-03-19
15:03:22.0 +0900
+++ linux-2.6/drivers/message/fusion/mptscsih.c 2007-03-19 15:03:23.0
+0900
@@ -1188,8 +1188,7 @@
int
mptscsih_resume(struct pci_dev *pdev)
{
- mpt_resume(pdev);
- return 0;
+ return mpt_resume(pdev);
}
#endif
-
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
Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Fri, 2007-03-16 at 16:05 +0900, Horms wrote: + err = pci_enable_device(pdev); + if (err 0) + return err; Traditionally, this should be if (err) return err; The reason is that 0 is a signed comparison which can be slightly more expensive on some architectures and it's unnecessary if zero is the only successful return. James - 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
Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote:
On Fri, 2007-03-16 at 16:05 +0900, Horms wrote:
+ err = pci_enable_device(pdev);
+ if (err 0)
+ return err;
Traditionally, this should be
if (err)
return err;
The reason is that 0 is a signed comparison which can be slightly more
expensive on some architectures and it's unnecessary if zero is the only
successful return.
Tradition vs. Linus, eh? Linus wrote (2007-Mar-06, on lkml,
Message-ID: [EMAIL PROTECTED]):
- negative error values should preferably always be tested as such
if (tick_init_highres() 0) {
printk(Aieee! Couldn't init!\n);
return 0;
}
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
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
Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote: On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote: On Fri, 2007-03-16 at 16:05 +0900, Horms wrote: + err = pci_enable_device(pdev); + if (err 0) + return err; Traditionally, this should be if (err) return err; The reason is that 0 is a signed comparison which can be slightly more expensive on some architectures and it's unnecessary if zero is the only successful return. Tradition vs. Linus, eh? Linus wrote (2007-Mar-06, on lkml, Message-ID: [EMAIL PROTECTED]): Sure ... we can all maintain our own traditions .. what was the subject of this email? James - 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
Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Fri, 16 Mar 2007 11:14:51 -0500 James Bottomley wrote: On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote: On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote: On Fri, 2007-03-16 at 16:05 +0900, Horms wrote: + err = pci_enable_device(pdev); + if (err 0) + return err; Traditionally, this should be if (err) return err; The reason is that 0 is a signed comparison which can be slightly more expensive on some architectures and it's unnecessary if zero is the only successful return. Tradition vs. Linus, eh? Linus wrote (2007-Mar-06, on lkml, Message-ID: [EMAIL PROTECTED]): Sure ... we can all maintain our own traditions .. what was the subject of this email? The subject was coding style and return/error codes. The Subject: line was: Re: [5/6] 2.6.21-rc2: known regressions --- ~Randy *** Remember to use Documentation/SubmitChecklist when testing your code *** - 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
Re: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
Randy Dunlap wrote: On Fri, 16 Mar 2007 11:14:51 -0500 James Bottomley wrote: On Fri, 2007-03-16 at 08:06 -0700, Randy Dunlap wrote: On Fri, 16 Mar 2007 09:27:26 -0500 James Bottomley wrote: On Fri, 2007-03-16 at 16:05 +0900, Horms wrote: + err = pci_enable_device(pdev); + if (err 0) + return err; Traditionally, this should be if (err) return err; The reason is that 0 is a signed comparison which can be slightly more expensive on some architectures and it's unnecessary if zero is the only successful return. Tradition vs. Linus, eh? Linus wrote (2007-Mar-06, on lkml, Message-ID: [EMAIL PROTECTED]): Sure ... we can all maintain our own traditions .. what was the subject of this email? The subject was coding style and return/error codes. The Subject: line was: Re: [5/6] 2.6.21-rc2: known regressions Randy, While on the subject of traditions, how about the C90 and C99 ones? C identifiers starting with __ are reserved! Reference: ISO/IEC 9899:1999 (C99) section 7.1.3 All identifiers that start with an underscore and either an upper case letter or another underscore are always reserved for any use. It was the same in C90. Now we might start getting rid of __u32 and its friends first :-) Doug Gilbert - 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
RE: [PATCH 2/2] fusion - honour return value of pci_enable_device() in mpt_resume()
On Friday, March 16, 2007 1:06 AM, Horms wrote: Honour the return value of pci_enable_device(), which seems to be a desirable thing to do: Both patch's look good. Thanks, Eric - 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
