All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 9/12] IPMI: add pigeonpoint poweroff
@ 2006-12-02  4:37 Corey Minyard
  2006-12-03 21:26 ` Andrew Morton
  0 siblings, 1 reply; 13+ messages in thread
From: Corey Minyard @ 2006-12-02  4:37 UTC (permalink / raw)
  To: Andrew Morton, Linux Kernel; +Cc: OpenIPMI Developers, Joseph Barnett


X86 boards generally use ACPI for the power management interactions
with the BMC.  However, non-x86 boards need some help.  This patch
adds the help for the Motorola PigeonPoint-based IPMCs.

Signed-off-by: Joseph Barnett <jbarnett@motorola.com>
Signed-off-by: Corey Minyard <minyard@acm.org>

Index: linux-2.6.19-rc6/drivers/char/ipmi/ipmi_poweroff.c
===================================================================
--- linux-2.6.19-rc6.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.19-rc6/drivers/char/ipmi/ipmi_poweroff.c
@@ -176,6 +176,42 @@ static int ipmi_request_in_rc_mode(ipmi_
 #define IPMI_ATCA_GET_ADDR_INFO_CMD	0x01
 #define IPMI_PICMG_ID			0
 
+#define IPMI_NETFN_OEM				0x2e
+#define IPMI_ATCA_PPS_GRACEFUL_RESTART		0x11
+#define IPMI_ATCA_PPS_IANA			"\x00\x40\x0A"
+#define IPMI_MOTOROLA_MANUFACTURER_ID		0x0000A1
+#define IPMI_MOTOROLA_PPS_IPMC_PRODUCT_ID	0x0051
+
+static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;
+
+static void pps_poweroff_atca (ipmi_user_t user)
+{
+        struct ipmi_system_interface_addr smi_addr;
+        struct kernel_ipmi_msg            send_msg;
+        int                               rv;
+        /*
+         * Configure IPMI address for local access
+         */
+        smi_addr.addr_type = IPMI_SYSTEM_INTERFACE_ADDR_TYPE;
+        smi_addr.channel = IPMI_BMC_CHANNEL;
+        smi_addr.lun = 0;
+
+        printk(KERN_INFO PFX "PPS powerdown hook used");
+
+        send_msg.netfn = IPMI_NETFN_OEM;
+        send_msg.cmd = IPMI_ATCA_PPS_GRACEFUL_RESTART;
+        send_msg.data = IPMI_ATCA_PPS_IANA;
+        send_msg.data_len = 3;
+        rv = ipmi_request_in_rc_mode(user,
+                                  (struct ipmi_addr *) &smi_addr,
+                                   &send_msg);
+        if (rv && rv != IPMI_UNKNOWN_ERR_COMPLETION_CODE) {
+                printk(KERN_ERR PFX "Unable to send ATCA ,"
+                       " IPMI error 0x%x\n", rv);
+        }
+	return;
+}
+
 static int ipmi_atca_detect (ipmi_user_t user)
 {
 	struct ipmi_system_interface_addr smi_addr;
@@ -201,6 +237,13 @@ static int ipmi_atca_detect (ipmi_user_t
 	rv = ipmi_request_wait_for_response(user,
 					    (struct ipmi_addr *) &smi_addr,
 					    &send_msg);
+
+        printk(KERN_INFO PFX "ATCA Detect mfg 0x%X prod 0x%X\n", mfg_id, prod_id);
+        if((mfg_id == IPMI_MOTOROLA_MANUFACTURER_ID)
+            && (prod_id == IPMI_MOTOROLA_PPS_IPMC_PRODUCT_ID)) {
+		printk(KERN_INFO PFX "Installing Pigeon Point Systems Poweroff Hook\n");
+		atca_oem_poweroff_hook = pps_poweroff_atca;
+	}
 	return !rv;
 }
 
@@ -234,12 +277,19 @@ static void ipmi_poweroff_atca (ipmi_use
 	rv = ipmi_request_in_rc_mode(user,
 				     (struct ipmi_addr *) &smi_addr,
 				     &send_msg);
-	if (rv) {
+        /** At this point, the system may be shutting down, and most
+         ** serial drivers (if used) will have interrupts turned off
+         ** it may be better to ignore IPMI_UNKNOWN_ERR_COMPLETION_CODE
+         ** return code
+         **/
+        if (rv && rv != IPMI_UNKNOWN_ERR_COMPLETION_CODE) {
 		printk(KERN_ERR PFX "Unable to send ATCA powerdown message,"
 		       " IPMI error 0x%x\n", rv);
 		goto out;
 	}
 
+	if(atca_oem_poweroff_hook)
+		return atca_oem_poweroff_hook(user);
  out:
 	return;
 }

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-02  4:37 [PATCH 9/12] IPMI: add pigeonpoint poweroff Corey Minyard
@ 2006-12-03 21:26 ` Andrew Morton
  2006-12-04  2:35   ` Corey Minyard
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Morton @ 2006-12-03 21:26 UTC (permalink / raw)
  To: minyard; +Cc: Linux Kernel, OpenIPMI Developers, Joseph Barnett

On Fri, 1 Dec 2006 22:37:46 -0600
Corey Minyard <minyard@acm.org> wrote:

> +static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;

Sometime, please go through the IPMI code looking for all these
statically-allocated things which are initialised to 0 or NULL and remove
all those intialisations?  They're unneeded, they increase the vmlinux
image size and there are quite a number of them.  Thanks.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-03 21:26 ` Andrew Morton
@ 2006-12-04  2:35   ` Corey Minyard
  2006-12-04  2:54     ` Randy Dunlap
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Corey Minyard @ 2006-12-04  2:35 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Linux Kernel, OpenIPMI Developers, Joseph Barnett

Andrew Morton wrote:
> On Fri, 1 Dec 2006 22:37:46 -0600
> Corey Minyard <minyard@acm.org> wrote:
>
>   
>> +static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;
>>     
>
> Sometime, please go through the IPMI code looking for all these
> statically-allocated things which are initialised to 0 or NULL and remove
> all those intialisations?  They're unneeded, they increase the vmlinux
> image size and there are quite a number of them.  Thanks.
>   
I'll do that, thanks, and I'll work on the other changes you suggest.

Do you prefer patches to fold into the existing patches or new versions?

-Corey

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  2:35   ` Corey Minyard
@ 2006-12-04  2:54     ` Randy Dunlap
  2006-12-04  3:53       ` [Openipmi-developer] " Bela Lubkin
  2006-12-04  3:00     ` Randy Dunlap
  2006-12-04  5:03     ` Andrew Morton
  2 siblings, 1 reply; 13+ messages in thread
From: Randy Dunlap @ 2006-12-04  2:54 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Andrew Morton, Linux Kernel, OpenIPMI Developers, Joseph Barnett

On Sun, 03 Dec 2006 20:35:05 -0600 Corey Minyard wrote:

> Andrew Morton wrote:
> > On Fri, 1 Dec 2006 22:37:46 -0600
> > Corey Minyard <minyard@acm.org> wrote:
> >
> >   
> >> +static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;
> >>     
> >
> > Sometime, please go through the IPMI code looking for all these
> > statically-allocated things which are initialised to 0 or NULL and remove
> > all those intialisations?  They're unneeded, they increase the vmlinux
> > image size and there are quite a number of them.  Thanks.
> >   
> I'll do that, thanks, and I'll work on the other changes you suggest.
> 
> Do you prefer patches to fold into the existing patches or new versions?

I was just about to send that patch.  Here it is,
on top of the series-of-12.

---
From: Randy Dunlap <randy.dunlap@oracle.com>

Remove all =0 and =NULL from static initializers.
They are not needed and removing them saves space in the object files.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/char/ipmi/ipmi_bt_sm.c      |    2 +-
 drivers/char/ipmi/ipmi_devintf.c    |    2 +-
 drivers/char/ipmi/ipmi_msghandler.c |    6 +++---
 drivers/char/ipmi/ipmi_poweroff.c   |    6 +++---
 drivers/char/ipmi/ipmi_si_intf.c    |   12 ++++++------
 drivers/char/ipmi/ipmi_watchdog.c   |   18 +++++++++---------
 6 files changed, 23 insertions(+), 23 deletions(-)

--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_bt_sm.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_bt_sm.c
@@ -38,7 +38,7 @@
 #define BT_DEBUG_MSG	2	/* Prints all request/response buffers */
 #define BT_DEBUG_STATES	4	/* Verbose look at state changes */
 
-static int bt_debug = BT_DEBUG_OFF;
+static int bt_debug;
 
 module_param(bt_debug, int, 0644);
 MODULE_PARM_DESC(bt_debug, "debug bitmask, 1=enable, 2=messages, 4=states");
--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_devintf.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_devintf.c
@@ -834,7 +834,7 @@ static const struct file_operations ipmi
 
 #define DEVICE_NAME     "ipmidev"
 
-static int ipmi_major = 0;
+static int ipmi_major;
 module_param(ipmi_major, int, 0);
 MODULE_PARM_DESC(ipmi_major, "Sets the major number of the IPMI device.  By"
 		 " default, or if you set it to zero, it will choose the next"
--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_msghandler.c
@@ -53,10 +53,10 @@
 static struct ipmi_recv_msg *ipmi_alloc_recv_msg(void);
 static int ipmi_init_msghandler(void);
 
-static int initialized = 0;
+static int initialized;
 
 #ifdef CONFIG_PROC_FS
-static struct proc_dir_entry *proc_ipmi_root = NULL;
+static struct proc_dir_entry *proc_ipmi_root;
 #endif /* CONFIG_PROC_FS */
 
 /* Remain in auto-maintenance mode for this amount of time (in ms). */
@@ -4041,7 +4041,7 @@ static void send_panic_events(char *str)
 }
 #endif /* CONFIG_IPMI_PANIC_EVENT */
 
-static int has_panicked = 0;
+static int has_panicked;
 
 static int panic_event(struct notifier_block *this,
 		       unsigned long         event,
--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_poweroff.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_poweroff.c
@@ -58,10 +58,10 @@ static int poweroff_powercycle;
 static int ifnum_to_use = -1;
 
 /* Our local state. */
-static int ready = 0;
+static int ready;
 static ipmi_user_t ipmi_user;
 static int ipmi_ifnum;
-static void (*specific_poweroff_func)(ipmi_user_t user) = NULL;
+static void (*specific_poweroff_func)(ipmi_user_t user);
 
 /* Holds the old poweroff function so we can restore it on removal. */
 static void (*old_poweroff_func)(void);
@@ -182,7 +182,7 @@ static int ipmi_request_in_rc_mode(ipmi_
 #define IPMI_MOTOROLA_MANUFACTURER_ID		0x0000A1
 #define IPMI_MOTOROLA_PPS_IPMC_PRODUCT_ID	0x0051
 
-static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;
+static void (*atca_oem_poweroff_hook)(ipmi_user_t user);
 
 static void pps_poweroff_atca (ipmi_user_t user)
 {
--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_si_intf.c
@@ -845,7 +845,7 @@ static void request_events(void *send_in
 	atomic_set(&smi_info->req_events, 1);
 }
 
-static int initialized = 0;
+static int initialized;
 
 static void smi_timeout(unsigned long data)
 {
@@ -1018,13 +1018,13 @@ static int num_ports;
 static int           irqs[SI_MAX_PARMS];
 static int num_irqs;
 static int           regspacings[SI_MAX_PARMS];
-static int num_regspacings = 0;
+static int num_regspacings;
 static int           regsizes[SI_MAX_PARMS];
-static int num_regsizes = 0;
+static int num_regsizes;
 static int           regshifts[SI_MAX_PARMS];
-static int num_regshifts = 0;
+static int num_regshifts;
 static int slave_addrs[SI_MAX_PARMS];
-static int num_slave_addrs = 0;
+static int num_slave_addrs;
 
 #define IPMI_IO_ADDR_SPACE  0
 #define IPMI_MEM_ADDR_SPACE 1
@@ -1668,7 +1668,7 @@ static __devinit void hardcode_find_bmc(
 /* Once we get an ACPI failure, we don't try any more, because we go
    through the tables sequentially.  Once we don't find a table, there
    are no more. */
-static int acpi_failure = 0;
+static int acpi_failure;
 
 /* For GPE-type interrupts. */
 static u32 ipmi_acpi_gpe(void *context)
--- linux-2.6.19-git4.orig/drivers/char/ipmi/ipmi_watchdog.c
+++ linux-2.6.19-git4/drivers/char/ipmi/ipmi_watchdog.c
@@ -134,14 +134,14 @@
 
 static int nowayout = WATCHDOG_NOWAYOUT;
 
-static ipmi_user_t watchdog_user = NULL;
+static ipmi_user_t watchdog_user;
 static int watchdog_ifnum;
 
 /* Default the timeout to 10 seconds. */
 static int timeout = 10;
 
 /* The pre-timeout is disabled by default. */
-static int pretimeout = 0;
+static int pretimeout;
 
 /* Default action is to reset the board on a timeout. */
 static unsigned char action_val = WDOG_TIMEOUT_RESET;
@@ -156,10 +156,10 @@ static unsigned char preop_val = WDOG_PR
 
 static char preop[16] = "preop_none";
 static DEFINE_SPINLOCK(ipmi_read_lock);
-static char data_to_read = 0;
+static char data_to_read;
 static DECLARE_WAIT_QUEUE_HEAD(read_q);
-static struct fasync_struct *fasync_q = NULL;
-static char pretimeout_since_last_heartbeat = 0;
+static struct fasync_struct *fasync_q;
+static char pretimeout_since_last_heartbeat;
 static char expect_close;
 
 static int ifnum_to_use = -1;
@@ -177,7 +177,7 @@ static void ipmi_unregister_watchdog(int
 
 /* If true, the driver will start running as soon as it is configured
    and ready. */
-static int start_now = 0;
+static int start_now;
 
 static int set_param_int(const char *val, struct kernel_param *kp)
 {
@@ -300,16 +300,16 @@ MODULE_PARM_DESC(nowayout, "Watchdog can
 static unsigned char ipmi_watchdog_state = WDOG_TIMEOUT_NONE;
 
 /* If shutting down via IPMI, we ignore the heartbeat. */
-static int ipmi_ignore_heartbeat = 0;
+static int ipmi_ignore_heartbeat;
 
 /* Is someone using the watchdog?  Only one user is allowed. */
-static unsigned long ipmi_wdog_open = 0;
+static unsigned long ipmi_wdog_open;
 
 /* If set to 1, the heartbeat command will set the state to reset and
    start the timer.  The timer doesn't normally run when the driver is
    first opened until the heartbeat is set the first time, this
    variable is used to accomplish this. */
-static int ipmi_start_timer_on_heartbeat = 0;
+static int ipmi_start_timer_on_heartbeat;
 
 /* IPMI version of the BMC. */
 static unsigned char ipmi_version_major;


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  2:35   ` Corey Minyard
  2006-12-04  2:54     ` Randy Dunlap
@ 2006-12-04  3:00     ` Randy Dunlap
  2006-12-04  5:03     ` Andrew Morton
  2 siblings, 0 replies; 13+ messages in thread
From: Randy Dunlap @ 2006-12-04  3:00 UTC (permalink / raw)
  To: Corey Minyard
  Cc: Andrew Morton, Linux Kernel, OpenIPMI Developers, Joseph Barnett

On Sun, 03 Dec 2006 20:35:05 -0600 Corey Minyard wrote:

> Andrew Morton wrote:
> > On Fri, 1 Dec 2006 22:37:46 -0600
> > Corey Minyard <minyard@acm.org> wrote:
> >
> >   
> >> +static void (*atca_oem_poweroff_hook)(ipmi_user_t user) = NULL;
> >>     
> >
> > Sometime, please go through the IPMI code looking for all these
> > statically-allocated things which are initialised to 0 or NULL and remove
> > all those intialisations?  They're unneeded, they increase the vmlinux
> > image size and there are quite a number of them.  Thanks.
> >   
> I'll do that, thanks, and I'll work on the other changes you suggest.

BTW, how about killing this comment or at least the email address part of it:

ipmi_bt_sm.c: *  Author:        Rocky Craig <first.last@hp.com>

---
~Randy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  2:54     ` Randy Dunlap
@ 2006-12-04  3:53       ` Bela Lubkin
  2006-12-04  4:01         ` Randy Dunlap
  0 siblings, 1 reply; 13+ messages in thread
From: Bela Lubkin @ 2006-12-04  3:53 UTC (permalink / raw)
  To: Randy Dunlap, Andrew Morton
  Cc: Corey Minyard, OpenIPMI Developers, Linux Kernel, Joseph Barnett

Andrew Morton wrote:

> > Sometime, please go through the IPMI code looking for all these
> > statically-allocated things which are initialised to 0 or NULL and remove
> > all those intialisations?  They're unneeded, they increase the vmlinux
> > image size and there are quite a number of them.  Thanks.

Randy Dunlop replied:

> I was just about to send that patch.  Here it is,
> on top of the series-of-12.
...
> -static int bt_debug = BT_DEBUG_OFF;
> +static int bt_debug;

Is it wise to significantly degrade code readability to work around a minor
compiler / linker bug?

>Bela<

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  3:53       ` [Openipmi-developer] " Bela Lubkin
@ 2006-12-04  4:01         ` Randy Dunlap
  2006-12-04  4:12           ` Randy Dunlap
                             ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Randy Dunlap @ 2006-12-04  4:01 UTC (permalink / raw)
  To: Bela Lubkin
  Cc: Andrew Morton, Corey Minyard, OpenIPMI Developers, Linux Kernel,
	Joseph Barnett

Bela Lubkin wrote:
> Andrew Morton wrote:
> 
>>> Sometime, please go through the IPMI code looking for all these
>>> statically-allocated things which are initialised to 0 or NULL and remove
>>> all those intialisations?  They're unneeded, they increase the vmlinux
>>> image size and there are quite a number of them.  Thanks.
> 
> Randy Dunlop replied:
> 
>> I was just about to send that patch.  Here it is,
>> on top of the series-of-12.
> ...
>> -static int bt_debug = BT_DEBUG_OFF;
>> +static int bt_debug;
> 
> Is it wise to significantly degrade code readability to work around a minor
> compiler / linker bug?

Is that the only one that is a problem?

I don't think it's a problem.  We *know* that static data areas
are init to 0.  Everything depends on that.  If that didn't work
it would all break.

I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
but I think that it's more than coincidence.

-- 
~Randy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  4:01         ` Randy Dunlap
@ 2006-12-04  4:12           ` Randy Dunlap
  2006-12-05 14:01             ` Corey Minyard
  2006-12-04  4:21           ` Bela Lubkin
  2006-12-04 12:06           ` Horst H. von Brand
  2 siblings, 1 reply; 13+ messages in thread
From: Randy Dunlap @ 2006-12-04  4:12 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Bela Lubkin, Andrew Morton, Corey Minyard, OpenIPMI Developers,
	Linux Kernel, Joseph Barnett

Randy Dunlap wrote:
> Bela Lubkin wrote:
>> Andrew Morton wrote:
>>
>>>> Sometime, please go through the IPMI code looking for all these
>>>> statically-allocated things which are initialised to 0 or NULL and 
>>>> remove
>>>> all those intialisations?  They're unneeded, they increase the vmlinux
>>>> image size and there are quite a number of them.  Thanks.
>>
>> Randy Dunlop replied:
>>
>>> I was just about to send that patch.  Here it is,
>>> on top of the series-of-12.
>> ...
>>> -static int bt_debug = BT_DEBUG_OFF;
>>> +static int bt_debug;
>>
>> Is it wise to significantly degrade code readability to work around a 
>> minor
>> compiler / linker bug?
> 
> Is that the only one that is a problem?
> 
> I don't think it's a problem.  We *know* that static data areas
> are init to 0.  Everything depends on that.  If that didn't work
> it would all break.
> 
> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
> but I think that it's more than coincidence.

It's Corey's decision.  However, while code readability is also very
important to me, I disagree with "significantly" above.

-- 
~Randy

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  4:01         ` Randy Dunlap
  2006-12-04  4:12           ` Randy Dunlap
@ 2006-12-04  4:21           ` Bela Lubkin
  2006-12-04 12:06           ` Horst H. von Brand
  2 siblings, 0 replies; 13+ messages in thread
From: Bela Lubkin @ 2006-12-04  4:21 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Andrew Morton, Corey Minyard, OpenIPMI Developers, Linux Kernel,
	Joseph Barnett

Randy Dunlap wrote:
> Bela Lubkin wrote:
>> Andrew Morton wrote:
>> 
>>> Sometime, please go through the IPMI code looking for all these
>>> statically-allocated things which are initialised to 0 or NULL and remove
>>> all those intialisations?  They're unneeded, they increase the vmlinux
>>> image size and there are quite a number of them.  Thanks.

>> Randy Dunlop replied:
 
<>> I was just about to send that patch.  Here it is,
>>> on top of the series-of-12.
>> ...
>>> -static int bt_debug = BT_DEBUG_OFF;
>>> +static int bt_debug;
>>
>> Is it wise to significantly degrade code readability to work around a
minor
>> compiler / linker bug?
>
> Is that the only one that is a problem?

It was the most obvious one at a quick glance.

I would argue that _every single one_ is a small problem because it makes the
code murkier.  Yes we "know" that statics are initialized to zero.  Not every
reader is perfectly versed in that.  The fact that statics and dynamics are
different is an added subtlety.  Everywhere you've removed an initializer is
a potential bug if someone needs to change that variable to a different
storage class.  Yes they "should" know better.

> I don't think it's a problem.  We *know* that static data areas
> are init to 0.  Everything depends on that.  If that didn't work
> it would all break.

Agreed; that's why it's a compiler/linker bug if _explicit_ initialization to
zero results in any difference in the final binary.  So here you're
submitting dozens of source level changes to repair a toolchain bug.

> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
> but I think that it's more than coincidence.

I agree, it's more than coincidence.  But the new version tells you less.  It
ccertainly _could_ have been coded such that debug == 1 means no debug, 0
means debug.  Dumb, but possible.  It could also have been coded such that
debug == BT_DEBUG_OFF means debug, but that's egregiously stupid.

In other words, I believe the reader of:

  static int bt_debug = BT_DEBUG_OFF;

_knows_ debug is off.  The reader of:

  static int bt_debug = 0;

has a _strong suspicion_ that debug is off.  The reader of:

  static int bt_debug;

has at best a _strong hint_ that debug is off.

Any of those three statements could be shortly followed by:

  bt_debug = BT_DEBUG_ON;

but this would be a moderately surprising construction after the first two;
not at all surprising after the third.

>Bela<

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  2:35   ` Corey Minyard
  2006-12-04  2:54     ` Randy Dunlap
  2006-12-04  3:00     ` Randy Dunlap
@ 2006-12-04  5:03     ` Andrew Morton
  2 siblings, 0 replies; 13+ messages in thread
From: Andrew Morton @ 2006-12-04  5:03 UTC (permalink / raw)
  To: Corey Minyard; +Cc: Linux Kernel, OpenIPMI Developers, Joseph Barnett

On Sun, 03 Dec 2006 20:35:05 -0600
Corey Minyard <minyard@acm.org> wrote:

> Do you prefer patches to fold into the existing patches or new versions?

Incremental updates are preferred where it makes sense, please.  That way
we can see what changed.  I often will convert updated patches into
incremental patches just to see what happened.  I've caught bugs that way..

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  4:01         ` Randy Dunlap
  2006-12-04  4:12           ` Randy Dunlap
  2006-12-04  4:21           ` Bela Lubkin
@ 2006-12-04 12:06           ` Horst H. von Brand
  2 siblings, 0 replies; 13+ messages in thread
From: Horst H. von Brand @ 2006-12-04 12:06 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Bela Lubkin, Andrew Morton, Corey Minyard, OpenIPMI Developers,
	Linux Kernel, Joseph Barnett

Randy Dunlap <randy.dunlap@oracle.com> wrote:
> Bela Lubkin wrote:
> > Andrew Morton wrote:
> >
> >>> Sometime, please go through the IPMI code looking for all these
> >>> statically-allocated things which are initialised to 0 or NULL and remove
> >>> all those intialisations?  They're unneeded, they increase the vmlinux
> >>> image size and there are quite a number of them.  Thanks.
> > Randy Dunlop replied:
> >
> >> I was just about to send that patch.  Here it is,
> >> on top of the series-of-12.
> > ...
> >> -static int bt_debug = BT_DEBUG_OFF;
> >> +static int bt_debug;
> > Is it wise to significantly degrade code readability to work around
> > a minor
> > compiler / linker bug?
> 
> Is that the only one that is a problem?
> 
> I don't think it's a problem.  We *know* that static data areas
> are init to 0.  Everything depends on that.  If that didn't work
> it would all break.

Right. And we know NULL == 0.

> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
> but I think that it's more than coincidence.

I'd have had to look over the code to find out what it was initialized
to. In cases where it is not an explicit 0/NULL, I'd leave it as is. It
could also break if somebody later on changes the value of BT_DEBUG_OFF
(yes, very unlikely, but...).

Bug your friendly GCC guy to loose static initializations to zero
(shouldn't be /that/ hard to do...) instead of obfuscating kernel's code.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239
Casilla 110-V, Valparaiso, Chile               Fax:  +56 32 2797513

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-04  4:12           ` Randy Dunlap
@ 2006-12-05 14:01             ` Corey Minyard
  2006-12-05 16:19               ` Bela Lubkin
  0 siblings, 1 reply; 13+ messages in thread
From: Corey Minyard @ 2006-12-05 14:01 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Bela Lubkin, Andrew Morton, OpenIPMI Developers, Linux Kernel,
	Joseph Barnett

Randy Dunlap wrote:
> Randy Dunlap wrote:
>> Bela Lubkin wrote:
>>> Andrew Morton wrote:
>>>
>>>>> Sometime, please go through the IPMI code looking for all these
>>>>> statically-allocated things which are initialised to 0 or NULL and
>>>>> remove
>>>>> all those intialisations?  They're unneeded, they increase the
>>>>> vmlinux
>>>>> image size and there are quite a number of them.  Thanks.
>>>
>>> Randy Dunlop replied:
>>>
>>>> I was just about to send that patch.  Here it is,
>>>> on top of the series-of-12.
>>> ...
>>>> -static int bt_debug = BT_DEBUG_OFF;
>>>> +static int bt_debug;
>>>
>>> Is it wise to significantly degrade code readability to work around
>>> a minor
>>> compiler / linker bug?
>>
>> Is that the only one that is a problem?
>>
>> I don't think it's a problem.  We *know* that static data areas
>> are init to 0.  Everything depends on that.  If that didn't work
>> it would all break.
>>
>> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
>> but I think that it's more than coincidence.
>
> It's Corey's decision.  However, while code readability is also very
> important to me, I disagree with "significantly" above.
>
I think the optimizations are probably important enough that this should
be done.  Let's take Randy's patch and I will add a comment to
BT_DEBUG_OFF that says that the value must be zero to correspond to
the default uninitialized value.

-Corey


^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [Openipmi-developer] [PATCH 9/12] IPMI: add pigeonpoint poweroff
  2006-12-05 14:01             ` Corey Minyard
@ 2006-12-05 16:19               ` Bela Lubkin
  0 siblings, 0 replies; 13+ messages in thread
From: Bela Lubkin @ 2006-12-05 16:19 UTC (permalink / raw)
  To: Corey Minyard, Randy Dunlap
  Cc: Andrew Morton, OpenIPMI Developers, Linux Kernel, Joseph Barnett

Corey Minyard wrote: 
> Randy Dunlap wrote:
>> Randy Dunlap wrote:
>>> Bela Lubkin wrote:
>>>> Andrew Morton wrote:
>>>>
>>>>>> Sometime, please go through the IPMI code looking for all these
>>>>>> statically-allocated things which are initialised to 0 or NULL
>>>>>> and remove all those intialisations?  They're unneeded, they
>>>>>> increase the vmlinux image size and there are quite a number of
>>>>>> them.  Thanks.
>>>>
>>>> Randy Dunlop replied:
>>>>
>>>>> I was just about to send that patch.  Here it is,
>>>>> on top of the series-of-12.
>>>> ...
>>>>> -static int bt_debug = BT_DEBUG_OFF;
>>>>> +static int bt_debug;
>>>>
>>>> Is it wise to significantly degrade code readability to work around
>>>> a minor compiler / linker bug?
>>>
>>> Is that the only one that is a problem?
>>>
>>> I don't think it's a problem.  We *know* that static data areas
>>> are init to 0.  Everything depends on that.  If that didn't work
>>> it would all break.
>>>
>>> I could say that it's a nice coincidence that BT_DEBUG_OFF == 0,
>>> but I think that it's more than coincidence.
>>
>> It's Corey's decision.  However, while code readability is also very
>> important to me, I disagree with "significantly" above.
>
> I think the optimizations are probably important enough that this
> should be done.  Let's take Randy's patch and I will add a comment to
> BT_DEBUG_OFF that says that the value must be zero to correspond to
> the default uninitialized value.

Patch the declaration to:

  static int bt_debug;  /* 0 == BT_DEBUG_OFF */

Then any sort of grep / cscope / patch excerpts / etc. are self-
documenting.

>Bela<

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2006-12-05 16:19 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-12-02  4:37 [PATCH 9/12] IPMI: add pigeonpoint poweroff Corey Minyard
2006-12-03 21:26 ` Andrew Morton
2006-12-04  2:35   ` Corey Minyard
2006-12-04  2:54     ` Randy Dunlap
2006-12-04  3:53       ` [Openipmi-developer] " Bela Lubkin
2006-12-04  4:01         ` Randy Dunlap
2006-12-04  4:12           ` Randy Dunlap
2006-12-05 14:01             ` Corey Minyard
2006-12-05 16:19               ` Bela Lubkin
2006-12-04  4:21           ` Bela Lubkin
2006-12-04 12:06           ` Horst H. von Brand
2006-12-04  3:00     ` Randy Dunlap
2006-12-04  5:03     ` Andrew Morton

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.