[STACK] >3k call path in ide

[STACK] >3k call path in ide

[Jörn Engel]

Bartlomiej, can you put ide_config on a diet?

stackframes for call path too long (3052):
    size  function
       0  client_reg_t->event_handler
    1168  ide_config
      12  ide_register_hw
      44  ide_unregister
      12  ide_unregister_subdriver
       0  pnpide_init
       0  pnp_register_driver
       0  driver_register
      20  bus_add_driver
      16  driver_attach
      72  tty_register_device
       0  class_simple_device_add
       0  class_device_register
      16  class_device_add
       0  kobject_add
       0  kobject_hotplug
     132  call_usermodehelper
      80  wait_for_completion
      84  schedule
      16  __put_task_struct
      20  audit_free
      36  audit_log_start
      16  __kmalloc
       0  __get_free_pages
      28  __alloc_pages
     284  try_to_free_pages
       0  out_of_memory
       0  mmput
      16  exit_aio
       0  __put_ioctx
      16  do_munmap
       0  split_vma
      36  vma_adjust
       0  fput
       0  __fput
       0  locks_remove_flock
      12  panic
       0  sys_sync
       0  sync_inodes
     308  sync_inodes_sb
       0  do_writepages
     128  mpage_writepages
       4  write_boundary_block
       0  ll_rw_block
      28  submit_bh
       0  bio_alloc
      88  mempool_alloc
     256  wakeup_bdflush
      20  pdflush_operation
       0  printk
       0  preempt_schedule
      84  schedule

Jörn

-- 
When you close your hand, you own nothing. When you open it up, you
own the whole world.
-- Li Mu Bai in Tiger & Dragon

Re: [STACK] >3k call path in ide

[Michael Clark]

Hi Jorn,

Noticed that try_to_free_pages, sync_inodes_sb and wakeup_bdflush features
in almost all of these traces and although at 284, 308 and 256 respectively
their not huge but together their neither that small (considering they
occur all in the same stack trace).

This is consumed mostly by a struct page_state which is 148 bytes big
although looking at the code get_page_state(struct page_state *ret)
only populates the first 6 fields or 24 bytes. get_full_page_state
which is hardly used updates these other fields.

Is this a candidate for splitting into 2 structs? 1 containing just the
first 6 fields needed by the majority of users: try_to_free_pages,
shrink_all_memory, kswapd, get_dirty_limits, wakeup_bdflush, sync_inodes_sb

mclark@monty:/usr/src/linux$ find . -name '*.c' | xargs grep get_page_state
./fs/proc/proc_misc.c:  get_page_state(&ps);
./fs/fs-writeback.c:    get_page_state(&ps);
./mm/page_alloc.c:void __get_page_state(struct page_state *ret, int nr)
./mm/page_alloc.c:void get_page_state(struct page_state *ret)
./mm/page_alloc.c:      __get_page_state(ret, nr + 1);
./mm/page_alloc.c:      __get_page_state(ret, sizeof(*ret) / sizeof(unsigned long));
./mm/page_alloc.c:      get_page_state(&ps);
./mm/vmscan.c:          get_page_state(&ps);
./mm/vmscan.c:          get_page_state(&ps);
./mm/vmscan.c:          get_page_state(&ps);
./mm/page-writeback.c:  get_page_state(ps);
./mm/page-writeback.c: * On really big machines, get_page_state is expensive, so try to avoid calling
./mm/page-writeback.c:          get_page_state(&ps);
./mm/page-writeback.c:  get_page_state(&ps);
./mm/page-writeback.c: * If it is too low then SMP machines will call the (expensive) get_page_state

mclark@monty:/usr/src/linux$ find . -name '*.c' | xargs grep get_full_page_state
./drivers/parisc/led.c: get_full_page_state(&pgstat); /* get no of sectors in & out */
./arch/s390/appldata/appldata_base.c:void get_full_page_state(struct page_state *ps)
./arch/s390/appldata/appldata_base.c:EXPORT_SYMBOL_GPL(get_full_page_state);
./arch/s390/appldata/appldata_mem.c:    get_full_page_state(&ps);
./mm/page_alloc.c:void get_full_page_state(struct page_state *ret)
./mm/page_alloc.c:      get_full_page_state(ps);

~mc

On 06/09/04 20:29, Jörn Engel wrote:
> Bartlomiej, can you put ide_config on a diet?
> 
> stackframes for call path too long (3052):
>     size  function
>        0  client_reg_t->event_handler
>     1168  ide_config
>       12  ide_register_hw
>       44  ide_unregister
>       12  ide_unregister_subdriver
>        0  pnpide_init
>        0  pnp_register_driver
>        0  driver_register
>       20  bus_add_driver
>       16  driver_attach
>       72  tty_register_device
>        0  class_simple_device_add
>        0  class_device_register
>       16  class_device_add
>        0  kobject_add
>        0  kobject_hotplug
>      132  call_usermodehelper
>       80  wait_for_completion
>       84  schedule
>       16  __put_task_struct
>       20  audit_free
>       36  audit_log_start
>       16  __kmalloc
>        0  __get_free_pages
>       28  __alloc_pages
>      284  try_to_free_pages
>        0  out_of_memory
>        0  mmput
>       16  exit_aio
>        0  __put_ioctx
>       16  do_munmap
>        0  split_vma
>       36  vma_adjust
>        0  fput
>        0  __fput
>        0  locks_remove_flock
>       12  panic
>        0  sys_sync
>        0  sync_inodes
>      308  sync_inodes_sb
>        0  do_writepages
>      128  mpage_writepages
>        4  write_boundary_block
>        0  ll_rw_block
>       28  submit_bh
>        0  bio_alloc
>       88  mempool_alloc
>      256  wakeup_bdflush
>       20  pdflush_operation
>        0  printk
>        0  preempt_schedule
>       84  schedule
> 
> Jörn
> 

-- 
Michael Clark,  . . . . . . . . . . . .  michael@metaparadigm.com
Managing Director,  . . . . . . . . . http://www.metaparadigm.com
Metaparadigm Pte. Ltd.  . . . . . . . . . .  phone: +65 6395 6277
25F Paterson Road,  . . . . . . . . . . . . mobile: +65 9645 9612
Singapore 238515  . . . . . . . . . . . . . .  fax: +65 6234 4043

Re: [STACK] >3k call path in ide

[Jörn Engel]

On Wed, 9 June 2004 23:23:20 +0800, Michael Clark wrote:
> 
> Noticed that try_to_free_pages, sync_inodes_sb and wakeup_bdflush features
> in almost all of these traces and although at 284, 308 and 256 respectively
> their not huge but together their neither that small (considering they
> occur all in the same stack trace).
> 
> This is consumed mostly by a struct page_state which is 148 bytes big
> although looking at the code get_page_state(struct page_state *ret)
> only populates the first 6 fields or 24 bytes. get_full_page_state
> which is hardly used updates these other fields.
> 
> Is this a candidate for splitting into 2 structs? 1 containing just the
> first 6 fields needed by the majority of users: try_to_free_pages,
> shrink_all_memory, kswapd, get_dirty_limits, wakeup_bdflush, sync_inodes_sb

Well noticed, although Hugh and Andrew have already exchanged some
patches, see
http://www.uwsg.indiana.edu/hypermail/linux/kernel/0406.1/0134.html

For the moment I consider try_to_free_pages() fixed.

Andrew, what do you thing about the patch below for sync_inodes_sb()?
It's stack consumption is reduced from 308 to 64, at the cost of one
more function call.

Jörn

-- 
Premature optimization is the root of all evil.
-- Donald Knuth

Move a struct page_state into it's own function.  This reduces the stack
consumption for sync_inodes_sb(), as the stack is already partially rolled
back before other functions get called.

Signed-off-by: Jörn Engel <joern@wohnheim.fh-wedel.de>

 fs-writeback.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

--- linux-2.6.6cow/fs/fs-writeback.c~sync_inodes_sb	2004-06-09 18:19:25.000000000 +0200
+++ linux-2.6.6cow/fs/fs-writeback.c	2004-06-09 18:23:44.000000000 +0200
@@ -396,6 +396,15 @@
 	spin_unlock(&inode_lock);
 }
 
+static long get_nr_to_write(void)
+{
+	struct page_state ps;
+
+	get_page_state(&ps);
+	return ps.nr_dirty + ps.nr_unstable + ps.nr_dirty + ps.nr_unstable +
+			(inodes_stat.nr_inodes - inodes_stat.nr_unused);
+}
+
 /*
  * writeback and wait upon the filesystem's dirty inodes.  The caller will
  * do this in two passes - one to write, and one to wait.  WB_SYNC_HOLD is
@@ -409,7 +418,6 @@
  */
 void sync_inodes_sb(struct super_block *sb, int wait)
 {
-	struct page_state ps;
 	struct writeback_control wbc = {
 		.bdi		= NULL,
 		.sync_mode	= wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
@@ -417,10 +425,7 @@
 		.nr_to_write	= 0,
 	};
 
-	get_page_state(&ps);
-	wbc.nr_to_write = ps.nr_dirty + ps.nr_unstable +
-			(inodes_stat.nr_inodes - inodes_stat.nr_unused) +
-			ps.nr_dirty + ps.nr_unstable;
+	wbc.nr_to_write = get_nr_to_write();
 	wbc.nr_to_write += wbc.nr_to_write / 2;		/* Bit more for luck */
 	spin_lock(&inode_lock);
 	sync_sb_inodes(sb, &wbc);

Re: [STACK] >3k call path in ide

[Andrew Morton]

Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
>
>  Andrew, what do you thing about the patch below for sync_inodes_sb()?
>  It's stack consumption is reduced from 308 to 64, at the cost of one
>  more function call.

Like this:

--- 25/fs/fs-writeback.c~sync_inodes_sb-stack-reduction	2004-06-09 12:25:57.111389456 -0700
+++ 25-akpm/fs/fs-writeback.c	2004-06-09 12:25:57.115388848 -0700
@@ -433,15 +433,15 @@ restart:
  */
 void sync_inodes_sb(struct super_block *sb, int wait)
 {
-	struct page_state ps;
 	struct writeback_control wbc = {
 		.sync_mode	= wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
 	};
+	unsigned long nr_dirty = read_page_state(nr_dirty);
+	unsigned long nr_unstable = read_page_state(nr_unstable);
 
-	get_page_state(&ps);
-	wbc.nr_to_write = ps.nr_dirty + ps.nr_unstable +
+	wbc.nr_to_write = nr_dirty + nr_unstable +
 			(inodes_stat.nr_inodes - inodes_stat.nr_unused) +
-			ps.nr_dirty + ps.nr_unstable;
+			nr_dirty + nr_unstable;
 	wbc.nr_to_write += wbc.nr_to_write / 2;		/* Bit more for luck */
 	spin_lock(&inode_lock);
 	sync_sb_inodes(sb, &wbc);
_

Re: [STACK] >3k call path in ide

[Jörn Engel]

On Wed, 9 June 2004 12:27:21 -0700, Andrew Morton wrote:
> Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
> >
> >  Andrew, what do you thing about the patch below for sync_inodes_sb()?
> >  It's stack consumption is reduced from 308 to 64, at the cost of one
> >  more function call.
> 
> Like this:
> 
> --- 25/fs/fs-writeback.c~sync_inodes_sb-stack-reduction	2004-06-09 12:25:57.111389456 -0700
> +++ 25-akpm/fs/fs-writeback.c	2004-06-09 12:25:57.115388848 -0700
> @@ -433,15 +433,15 @@ restart:
>   */
>  void sync_inodes_sb(struct super_block *sb, int wait)
>  {
> -	struct page_state ps;
>  	struct writeback_control wbc = {
>  		.sync_mode	= wait ? WB_SYNC_ALL : WB_SYNC_HOLD,
>  	};
> +	unsigned long nr_dirty = read_page_state(nr_dirty);
> +	unsigned long nr_unstable = read_page_state(nr_unstable);

read_page_state doesn't exist in 2.6.7-rc3 or 2.6.6-mm5.  How is it
defined?

If it is just a simple macro to access the right fields, then the
patch looks fine to me.

> -	get_page_state(&ps);
> -	wbc.nr_to_write = ps.nr_dirty + ps.nr_unstable +
> +	wbc.nr_to_write = nr_dirty + nr_unstable +
>  			(inodes_stat.nr_inodes - inodes_stat.nr_unused) +
> -			ps.nr_dirty + ps.nr_unstable;
> +			nr_dirty + nr_unstable;
>  	wbc.nr_to_write += wbc.nr_to_write / 2;		/* Bit more for luck */
>  	spin_lock(&inode_lock);
>  	sync_sb_inodes(sb, &wbc);
> _

Jörn

-- 
Fantasy is more important than knowledge. Knowledge is limited,
while fantasy embraces the whole world.
-- Albert Einstein

Re: [STACK] >3k call path in ide

[Andrew Morton]

Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
>
> read_page_state doesn't exist in 2.6.7-rc3 or 2.6.6-mm5.  How is it
> defined?

It was in 2.6.7-rc3-mm1.



struct page_state is large (148 bytes) and we put them on the stack in awkward
code paths (page reclaim...)

So implement a simple read_page_state() which can be used to pluck out a
single member from the all-cpus page_state accumulators.

Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 25-akpm/include/linux/page-flags.h |    4 ++++
 25-akpm/mm/page_alloc.c            |   17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff -puN include/linux/page-flags.h~read_page_state include/linux/page-flags.h
--- 25/include/linux/page-flags.h~read_page_state	Wed Jun  9 17:06:33 2004
+++ 25-akpm/include/linux/page-flags.h	Wed Jun  9 17:06:33 2004
@@ -139,6 +139,10 @@ DECLARE_PER_CPU(struct page_state, page_
 
 extern void get_page_state(struct page_state *ret);
 extern void get_full_page_state(struct page_state *ret);
+extern unsigned long __read_page_state(unsigned offset);
+
+#define read_page_state(member) \
+	__read_page_state(offsetof(struct page_state, member))
 
 #define mod_page_state(member, delta)					\
 	do {								\
diff -puN mm/page_alloc.c~read_page_state mm/page_alloc.c
--- 25/mm/page_alloc.c~read_page_state	Wed Jun  9 17:06:33 2004
+++ 25-akpm/mm/page_alloc.c	Wed Jun  9 17:11:57 2004
@@ -947,6 +947,23 @@ void get_full_page_state(struct page_sta
 	__get_page_state(ret, sizeof(*ret) / sizeof(unsigned long));
 }
 
+unsigned long __read_page_state(unsigned offset)
+{
+	unsigned long ret = 0;
+	int cpu;
+
+	for (cpu = 0; cpu < NR_CPUS; cpu++) {
+		unsigned long in;
+
+		if (!cpu_possible(cpu))
+			continue;
+
+		in = (unsigned long)&per_cpu(page_states, cpu) + offset;
+		ret += *((unsigned long *)in);
+	}
+	return ret;
+}
+
 void get_zone_counts(unsigned long *active,
 		unsigned long *inactive, unsigned long *free)
 {
_

Re: [STACK] >3k call path in ide

[Jörn Engel]

On Thu, 10 June 2004 16:10:21 -0700, Andrew Morton wrote:
> Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
> >
> > read_page_state doesn't exist in 2.6.7-rc3 or 2.6.6-mm5.  How is it
> > defined?
> 
> It was in 2.6.7-rc3-mm1.

A beauty!  Your trick about submitting an ugly patch and waiting for
others to do something better really works. :)

Jörn

-- 
My second remark is that our intellectual powers are rather geared to
master static relations and that our powers to visualize processes
evolving in time are relatively poorly developed.
-- Edsger W. Dijkstra

Re: [STACK] >3k call path in ide

[Rusty Russell]

On Fri, 2004-06-11 at 09:10, Andrew Morton wrote:
> Jörn Engel <joern@wohnheim.fh-wedel.de> wrote:
> >
> > read_page_state doesn't exist in 2.6.7-rc3 or 2.6.6-mm5.  How is it
> > defined?
> 
> It was in 2.6.7-rc3-mm1.
> 
> 
> 
> struct page_state is large (148 bytes) and we put them on the stack in awkward
> code paths (page reclaim...)
> 
> So implement a simple read_page_state() which can be used to pluck out a
> single member from the all-cpus page_state accumulators.
> 
> Signed-off-by: Andrew Morton <akpm@osdl.org>
...
> +unsigned long __read_page_state(unsigned offset)
> +{
> +	unsigned long ret = 0;
> +	int cpu;
> +
> +	for (cpu = 0; cpu < NR_CPUS; cpu++) {
> +		unsigned long in;
> +
> +		if (!cpu_possible(cpu))
> +			continue;

for_each_cpu(cpu) here perhaps?

Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell

[PATCH] [STACK] >3k call path in ide

[Randy.Dunlap]

On Wed, 9 Jun 2004 14:29:21 +0200 Jörn Engel wrote:

| Bartlomiej, can you put ide_config on a diet?
| 
| stackframes for call path too long (3052):
|     size  function
|        0  client_reg_t->event_handler
|     1168  ide_config


Here's a patch for ide_config(), the worst offender in this
call chain.


Reduce large stack usage in ide_config() by using kmalloc(),
  down from 0x4a4 bytes to 0x74 bytes (x86-32).
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  137 ++++++++++++++++++++++++++------------------
 1 files changed, 81 insertions(+), 56 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-15 15:32:42.000000000 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,86 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
-
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    u_short *tbuf;
+    cisparse_t *cisparse;
+    config_info_t *cfginfo = 0;
+    cistpl_cftable_entry_t *cfg;
+    cistpl_cftable_entry_t *def_cte = 0;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
+    if (!tbuf) goto err_tbuf;
+    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
+    if (!def_cte) goto err_def_cte;
+    memset(def_cte, 0, sizeof(*def_cte));
+    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
+    if (!cfginfo) goto err_cfginfo;
+    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
+    if (!cisparse) goto err_cisparse;
+    cfg = &cisparse->cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)tbuf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, cisparse));
+    link->conf.ConfigBase = cisparse->config.base;
+    link->conf.Present = cisparse->config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, cisparse))
+	is_kme = ((cisparse->manfid.manf == MANFID_KME) &&
+		  ((cisparse->manfid.card == PRODID_KME_KXLC005_A) ||
+		   (cisparse->manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, cfginfo));
+    link->conf.Vcc = cfginfo->Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, cisparse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (def_cte->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != def_cte->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (def_cte->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		def_cte->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (def_cte->io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &def_cte->io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +319,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(def_cte, cfg, sizeof(*def_cte));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(def_cte, 0, sizeof(*def_cte));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +349,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +378,34 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
     return;
-    
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
 
+    /* memory allocation errors */
+err_cisparse:
+    kfree(cfginfo);
+err_cfginfo:
+    kfree(def_cte);
+err_def_cte:
+    kfree(tbuf);
+err_tbuf:
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
 } /* ide_config */
 
 /*======================================================================

Re: [PATCH] [STACK] >3k call path in ide

[Florian Schirmer]

Hi,

>  failed:
>      ide_release(link);
>      link->state &= ~DEV_CONFIG_PENDING;
>
> +    /* memory allocation errors */
> +err_cisparse:
> +    kfree(cfginfo);
> +err_cfginfo:
> +    kfree(def_cte);
> +err_def_cte:
> +    kfree(tbuf);
> +err_tbuf:
> +    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
> +    goto failed;
>  } /* ide_config */

Huh? This will either leak memory (non alloc error case) or deadlock (mem 
alloc error case). I'm missing something?

Best,
   Florian

Re: [PATCH] [STACK] >3k call path in ide

[Jörn Engel]

On Wed, 16 June 2004 09:11:11 +0200, Florian Schirmer wrote:
> 
> >  failed:
> >      ide_release(link);
> >      link->state &= ~DEV_CONFIG_PENDING;
> >
> > +    /* memory allocation errors */
> > +err_cisparse:
> > +    kfree(cfginfo);
> > +err_cfginfo:
> > +    kfree(def_cte);
> > +err_def_cte:
> > +    kfree(tbuf);
> > +err_tbuf:
> > +    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
> > +    goto failed;
> >  } /* ide_config */
> 
> Huh? This will either leak memory (non alloc error case) or deadlock (mem 
> alloc error case). I'm missing something?

Leak memory.  I also tend to depend on the fact that kfree(NULL) works
just fine:

err_kfree:
	kfree(cfginfo);
	kfree(def_cte);
	kfree(tbuf);
	printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
	goto failed;

Makes the error path a little simpler.

Jörn

-- 
The strong give up and move away, while the weak give up and stay.
-- unknown

Re: [PATCH] [STACK] >3k call path in ide

[Florian Schirmer]

Hi,

>Leak memory.  I also tend to depend on the fact that kfree(NULL) works
>just fine:
>
>err_kfree:
>	kfree(cfginfo);
>	kfree(def_cte);
>	kfree(tbuf);
>	printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
>	goto failed;
>
>Makes the error path a little simpler.
>  
>

Nope. It will deadlock just like the original patch because failed falls 
through to err_kfree which then will jump to failed...

Regards,
    Florian

Re: [PATCH] [STACK] >3k call path in ide

[Jörn Engel]

On Wed, 16 June 2004 11:55:52 +0200, Florian Schirmer wrote:
> 
> >Leak memory.
> 
> Nope. It will deadlock just like the original patch because failed falls 
> through to err_kfree which then will jump to failed...

Both, leak memory on good path, deadlock on error path.  Fun patch. :)

Jörn

-- 
It is better to die of hunger having lived without grief and fear,
than to live with a troubled spirit amid abundance.
-- Epictetus

[PATCH] [STACK] reduce >3k call path in ide

[Randy.Dunlap]

Thanks for the helpful comments.  Here's a corrected patch.



Reduce large stack usage in ide_config() by using kmalloc().
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  135 +++++++++++++++++++++++++-------------------
 1 files changed, 79 insertions(+), 56 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-16 09:11:14.431098048 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,86 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
-
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    u_short *tbuf = 0;
+    cisparse_t *cisparse = 0;
+    config_info_t *cfginfo = 0;
+    cistpl_cftable_entry_t *cfg;
+    cistpl_cftable_entry_t *def_cte = 0;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
+    if (!tbuf) goto err_kfree;
+    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
+    if (!def_cte) goto err_kfree;
+    memset(def_cte, 0, sizeof(*def_cte));
+    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
+    if (!cfginfo) goto err_kfree;
+    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
+    if (!cisparse) goto err_kfree;
+    cfg = &cisparse->cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)tbuf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, cisparse));
+    link->conf.ConfigBase = cisparse->config.base;
+    link->conf.Present = cisparse->config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, cisparse))
+	is_kme = ((cisparse->manfid.manf == MANFID_KME) &&
+		  ((cisparse->manfid.card == PRODID_KME_KXLC005_A) ||
+		   (cisparse->manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, cfginfo));
+    link->conf.Vcc = cfginfo->Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, cisparse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (def_cte->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != def_cte->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (def_cte->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		def_cte->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (def_cte->io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &def_cte->io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +319,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(def_cte, cfg, sizeof(*def_cte));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(def_cte, 0, sizeof(*def_cte));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +349,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +378,32 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
     return;
-    
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
+    return;
 
+    /* memory allocation errors */
+err_kfree:
+    kfree(cfginfo);
+    kfree(def_cte);
+    kfree(tbuf);
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
 } /* ide_config */
 
 /*======================================================================

Re: [PATCH] [STACK] reduce >3k call path in ide

[Jörn Engel]

On Wed, 16 June 2004 10:37:41 -0700, Randy.Dunlap wrote:
> 
> Thanks for the helpful comments.  Here's a corrected patch.

Looks, as if it still leaks memory:

>  
>      link->state &= ~DEV_CONFIG_PENDING;

about here.

>      return;
> -    
> +
>  cs_failed:
>      cs_error(link->handle, last_fn, last_ret);
>  failed:
>      ide_release(link);
>      link->state &= ~DEV_CONFIG_PENDING;
> +    return;
>  
> +    /* memory allocation errors */
> +err_kfree:
> +    kfree(cfginfo);
> +    kfree(def_cte);
> +    kfree(tbuf);
> +    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
> +    goto failed;
>  } /* ide_config */
>  
>  /*======================================================================

Jörn

-- 
Schrödinger's cat is <BLINK>not</BLINK> dead.
-- Illiad

Re: [PATCH] [STACK] reduce >3k call path in ide

[Randy.Dunlap]

On Wed, 16 Jun 2004 19:57:30 +0200 Jörn Engel wrote:

| On Wed, 16 June 2004 10:37:41 -0700, Randy.Dunlap wrote:
| > 
| > Thanks for the helpful comments.  Here's a corrected patch.
| 
| Looks, as if it still leaks memory:

duh.  fudge.  Thanks.  How's this one?




Reduce large stack usage in ide_config() by using kmalloc(),
  down from 0x4a4 bytes to 0x74 bytes (x86-32).
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  137 +++++++++++++++++++++++++-------------------
 1 files changed, 80 insertions(+), 57 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-16 10:42:00.554161496 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,86 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
-
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    u_short *tbuf = 0;
+    cisparse_t *cisparse = 0;
+    config_info_t *cfginfo = 0;
+    cistpl_cftable_entry_t *cfg;
+    cistpl_cftable_entry_t *def_cte = 0;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
+    if (!tbuf) goto err_mem;
+    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
+    if (!def_cte) goto err_mem;
+    memset(def_cte, 0, sizeof(*def_cte));
+    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
+    if (!cfginfo) goto err_mem;
+    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
+    if (!cisparse) goto err_mem;
+    cfg = &cisparse->cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)tbuf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, cisparse));
+    link->conf.ConfigBase = cisparse->config.base;
+    link->conf.Present = cisparse->config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, cisparse))
+	is_kme = ((cisparse->manfid.manf == MANFID_KME) &&
+		  ((cisparse->manfid.card == PRODID_KME_KXLC005_A) ||
+		   (cisparse->manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, cfginfo));
+    link->conf.Vcc = cfginfo->Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, cisparse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (def_cte->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != def_cte->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (def_cte->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		def_cte->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (def_cte->io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &def_cte->io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +319,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(def_cte, cfg, sizeof(*def_cte));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(def_cte, 0, sizeof(*def_cte));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +349,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +378,32 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
     return;
-    
+
+err_mem:
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
+    kfree(cisparse);
+    kfree(cfginfo);
+    kfree(def_cte);
+    kfree(tbuf);
+
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
-
 } /* ide_config */
 
 /*======================================================================

Re: [PATCH] [STACK] reduce >3k call path in ide

[Jörn Engel]

On Wed, 16 June 2004 11:16:21 -0700, Randy.Dunlap wrote:
> On Wed, 16 Jun 2004 19:57:30 +0200 Jörn Engel wrote:
> 
> | On Wed, 16 June 2004 10:37:41 -0700, Randy.Dunlap wrote:
> | > 
> | > Thanks for the helpful comments.  Here's a corrected patch.
> | 
> | Looks, as if it still leaks memory:
> 
> duh.  fudge.  Thanks.  How's this one?

Just four more lines?

>      link->dev = &info->node;
>      printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
> -	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
> -	   link->conf.Vpp1/10, link->conf.Vpp1%10);
> +	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
> +	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
>  
>      link->state &= ~DEV_CONFIG_PENDING;
+    kfree(cisparse);
+    kfree(cfginfo);
+    kfree(def_cte);
+    kfree(tbuf);
>      return;
> -    
> +
> +err_mem:
> +    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
> +    goto failed;
> +
>  cs_failed:
>      cs_error(link->handle, last_fn, last_ret);
>  failed:
> +    kfree(cisparse);
> +    kfree(cfginfo);
> +    kfree(def_cte);
> +    kfree(tbuf);
> +
>      ide_release(link);
>      link->state &= ~DEV_CONFIG_PENDING;
> -
>  } /* ide_config */
>  
>  /*======================================================================

Jörn

-- 
Fancy algorithms are slow when n is small, and n is usually small.
Fancy algorithms have big constants. Until you know that n is
frequently going to be big, don't get fancy.
-- Rob Pike

Re: [PATCH] [STACK] reduce >3k call path in ide

[Randy.Dunlap]

On Wed, 16 Jun 2004 20:29:10 +0200 Jörn Engel wrote:

| On Wed, 16 June 2004 11:16:21 -0700, Randy.Dunlap wrote:
| > On Wed, 16 Jun 2004 19:57:30 +0200 Jörn Engel wrote:
| > 
| > | On Wed, 16 June 2004 10:37:41 -0700, Randy.Dunlap wrote:
| > | > 
| > | > Thanks for the helpful comments.  Here's a corrected patch.
| > | 
| > | Looks, as if it still leaks memory:
| > 
| > duh.  fudge.  Thanks.  How's this one?
| 
| Just four more lines?

OK, Randy, slow down and go to lunch.

Thanks, Jörn.



Reduce large stack usage in ide_config() by using kmalloc(),
  down from 0x4a4 bytes to 0x74 bytes (x86-32).
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  141 ++++++++++++++++++++++++++------------------
 1 files changed, 84 insertions(+), 57 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-16 11:01:43.000000000 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,86 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
-
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    u_short *tbuf = 0;
+    cisparse_t *cisparse = 0;
+    config_info_t *cfginfo = 0;
+    cistpl_cftable_entry_t *cfg;
+    cistpl_cftable_entry_t *def_cte = 0;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
+    if (!tbuf) goto err_mem;
+    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
+    if (!def_cte) goto err_mem;
+    memset(def_cte, 0, sizeof(*def_cte));
+    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
+    if (!cfginfo) goto err_mem;
+    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
+    if (!cisparse) goto err_mem;
+    cfg = &cisparse->cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)tbuf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, cisparse));
+    link->conf.ConfigBase = cisparse->config.base;
+    link->conf.Present = cisparse->config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, cisparse))
+	is_kme = ((cisparse->manfid.manf == MANFID_KME) &&
+		  ((cisparse->manfid.card == PRODID_KME_KXLC005_A) ||
+		   (cisparse->manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, cfginfo));
+    link->conf.Vcc = cfginfo->Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, cisparse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (def_cte->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (cfginfo->Vcc != def_cte->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (def_cte->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		def_cte->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (def_cte->io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &def_cte->io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +319,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(def_cte, cfg, sizeof(*def_cte));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(def_cte, 0, sizeof(*def_cte));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +349,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +378,36 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
+    kfree(cisparse);
+    kfree(cfginfo);
+    kfree(def_cte);
+    kfree(tbuf);
     return;
-    
+
+err_mem:
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
+    kfree(cisparse);
+    kfree(cfginfo);
+    kfree(def_cte);
+    kfree(tbuf);
+
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
-
 } /* ide_config */
 
 /*======================================================================

Re: [PATCH] [STACK] reduce >3k call path in ide

[Brian Gerst]

Randy.Dunlap wrote:
> +    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
> +    if (!tbuf) goto err_kfree;
> +    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
> +    if (!def_cte) goto err_kfree;
> +    memset(def_cte, 0, sizeof(*def_cte));
> +    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
> +    if (!cfginfo) goto err_kfree;
> +    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
> +    if (!cisparse) goto err_kfree;

This can be condensed into a single kmalloc.  Define a struct that 
contains these variables and kmalloc the whole struct in one call.

--
				Brian Gerst

Re: [PATCH] [STACK] reduce >3k call path in ide

[Randy.Dunlap]

On Wed, 16 Jun 2004 14:58:19 -0400 Brian Gerst wrote:

| Randy.Dunlap wrote:
| > +    tbuf = kmalloc(128 * sizeof(u_short), GFP_KERNEL);
| > +    if (!tbuf) goto err_kfree;
| > +    def_cte = kmalloc(sizeof(*def_cte), GFP_KERNEL);
| > +    if (!def_cte) goto err_kfree;
| > +    memset(def_cte, 0, sizeof(*def_cte));
| > +    cfginfo = kmalloc(sizeof(*cfginfo), GFP_KERNEL);
| > +    if (!cfginfo) goto err_kfree;
| > +    cisparse = kmalloc(sizeof(*cisparse), GFP_KERNEL);
| > +    if (!cisparse) goto err_kfree;
| 
| This can be condensed into a single kmalloc.  Define a struct that 
| contains these variables and kmalloc the whole struct in one call.


OK, how's this one?
and thanks.


Reduce large stack usage in ide_config() by using kmalloc(),
  down from 0x4a4 bytes to 0x64 bytes (x86-32).
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  130 +++++++++++++++++++++++++-------------------
 1 files changed, 74 insertions(+), 56 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-16 11:59:58.728659752 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,84 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
+struct stk {
+	u_short		buf[128];
+	cisparse_t	parse;
+	config_info_t	conf;
+	cistpl_cftable_entry_t dflt;
+};
 
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    struct stk *stk = 0;
+    cistpl_cftable_entry_t *cfg;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    stk = kmalloc(sizeof(*stk), GFP_KERNEL);
+    if (!stk) goto err_mem;
+    memset(stk, 0, sizeof(*stk));
+    cfg = &stk->parse.cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)&stk->buf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &stk->parse));
+    link->conf.ConfigBase = stk->parse.config.base;
+    link->conf.Present = stk->parse.config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, &stk->parse))
+	is_kme = ((stk->parse.manfid.manf == MANFID_KME) &&
+		  ((stk->parse.manfid.card == PRODID_KME_KXLC005_A) ||
+		   (stk->parse.manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &stk->conf));
+    link->conf.Vcc = stk->conf.Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, &stk->parse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (stk->conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (stk->dflt.vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (stk->conf.Vcc != stk->dflt.vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (stk->dflt.vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		stk->dflt.vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (stk->dflt.io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &stk->dflt.io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +317,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(&stk->dflt, cfg, sizeof(stk->dflt));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(&stk->dflt, 0, sizeof(stk->dflt));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +347,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +376,29 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
+    kfree(stk);
     return;
-    
+
+err_mem:
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
+    kfree(stk);
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
-
 } /* ide_config */
 
 /*======================================================================

Re: [PATCH] [STACK] reduce >3k call path in ide

[Randy.Dunlap]

This version moves the stack structure "stk" inside the ide_config()
function as an anonymous struct.
I hope this is the last one.... :)



Reduce large stack usage in ide_config() by using kmalloc(),
  down from 0x4a4 bytes to 0x64 bytes (x86-32).
Little whitespace cleanup.
Move function comment block to immediately above the function.
Module loaded and unloaded, otherwise not tested (no hardware).

Signed-off-by: Randy Dunlap <rddunlap@osdl.org>


diffstat:=
 drivers/ide/legacy/ide-cs.c |  130 ++++++++++++++++++++++++--------------------
 1 files changed, 73 insertions(+), 57 deletions(-)

diff -Naurp ./drivers/ide/legacy/ide-cs.c~idecs_stack ./drivers/ide/legacy/ide-cs.c
--- ./drivers/ide/legacy/ide-cs.c~idecs_stack	2004-05-09 19:32:53.000000000 -0700
+++ ./drivers/ide/legacy/ide-cs.c	2004-06-16 13:31:08.285160776 -0700
@@ -199,6 +199,16 @@ static void ide_detach(dev_link_t *link)
     
 } /* ide_detach */
 
+static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
+{
+    hw_regs_t hw;
+    memset(&hw, 0, sizeof(hw));
+    ide_init_hwif_ports(&hw, io, ctl, NULL);
+    hw.irq = irq;
+    hw.chipset = ide_pci;
+    return ide_register_hw(&hw, NULL);
+}
+
 /*======================================================================
 
     ide_config() is scheduled to run after a CARD_INSERTION event
@@ -210,84 +220,82 @@ static void ide_detach(dev_link_t *link)
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
 
-static int idecs_register(unsigned long io, unsigned long ctl, unsigned long irq)
-{
-    hw_regs_t hw;
-    memset(&hw, 0, sizeof(hw));
-    ide_init_hwif_ports(&hw, io, ctl, NULL);
-    hw.irq = irq;
-    hw.chipset = ide_pci;
-    return ide_register_hw(&hw, NULL);
-}
-
 void ide_config(dev_link_t *link)
 {
     client_handle_t handle = link->handle;
     ide_info_t *info = link->priv;
     tuple_t tuple;
-    u_short buf[128];
-    cisparse_t parse;
-    config_info_t conf;
-    cistpl_cftable_entry_t *cfg = &parse.cftable_entry;
-    cistpl_cftable_entry_t dflt = { 0 };
-    int i, pass, last_ret, last_fn, hd, is_kme = 0;
+    struct {
+	u_short		buf[128];
+	cisparse_t	parse;
+	config_info_t	conf;
+	cistpl_cftable_entry_t dflt;
+    } *stk = 0;
+    cistpl_cftable_entry_t *cfg;
+    int i, pass, last_ret = 0, last_fn = 0, hd, is_kme = 0;
     unsigned long io_base, ctl_base;
 
     DEBUG(0, "ide_config(0x%p)\n", link);
-    
-    tuple.TupleData = (cisdata_t *)buf;
-    tuple.TupleOffset = 0; tuple.TupleDataMax = 255;
+
+    stk = kmalloc(sizeof(*stk), GFP_KERNEL);
+    if (!stk) goto err_mem;
+    memset(stk, 0, sizeof(*stk));
+    cfg = &stk->parse.cftable_entry;
+
+    tuple.TupleData = (cisdata_t *)&stk->buf;
+    tuple.TupleOffset = 0;
+    tuple.TupleDataMax = 255;
     tuple.Attributes = 0;
     tuple.DesiredTuple = CISTPL_CONFIG;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     CS_CHECK(GetTupleData, pcmcia_get_tuple_data(handle, &tuple));
-    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &parse));
-    link->conf.ConfigBase = parse.config.base;
-    link->conf.Present = parse.config.rmask[0];
+    CS_CHECK(ParseTuple, pcmcia_parse_tuple(handle, &tuple, &stk->parse));
+    link->conf.ConfigBase = stk->parse.config.base;
+    link->conf.Present = stk->parse.config.rmask[0];
 
     tuple.DesiredTuple = CISTPL_MANFID;
     if (!pcmcia_get_first_tuple(handle, &tuple) &&
 	!pcmcia_get_tuple_data(handle, &tuple) &&
-	!pcmcia_parse_tuple(handle, &tuple, &parse))
-	is_kme = ((parse.manfid.manf == MANFID_KME) &&
-		  ((parse.manfid.card == PRODID_KME_KXLC005_A) ||
-		   (parse.manfid.card == PRODID_KME_KXLC005_B)));
+	!pcmcia_parse_tuple(handle, &tuple, &stk->parse))
+	is_kme = ((stk->parse.manfid.manf == MANFID_KME) &&
+		  ((stk->parse.manfid.card == PRODID_KME_KXLC005_A) ||
+		   (stk->parse.manfid.card == PRODID_KME_KXLC005_B)));
 
     /* Configure card */
     link->state |= DEV_CONFIG;
 
     /* Not sure if this is right... look up the current Vcc */
-    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &conf));
-    link->conf.Vcc = conf.Vcc;
-    
+    CS_CHECK(GetConfigurationInfo, pcmcia_get_configuration_info(handle, &stk->conf));
+    link->conf.Vcc = stk->conf.Vcc;
+
     pass = io_base = ctl_base = 0;
     tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
     tuple.Attributes = 0;
     CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
     while (1) {
     	if (pcmcia_get_tuple_data(handle, &tuple) != 0) goto next_entry;
-	if (pcmcia_parse_tuple(handle, &tuple, &parse) != 0) goto next_entry;
+	if (pcmcia_parse_tuple(handle, &tuple, &stk->parse) != 0) goto next_entry;
 
 	/* Check for matching Vcc, unless we're desperate */
 	if (!pass) {
-	    if (cfg->vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM]/10000)
+	    if (cfg->vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (stk->conf.Vcc != cfg->vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
-	    } else if (dflt.vcc.present & (1<<CISTPL_POWER_VNOM)) {
-		if (conf.Vcc != dflt.vcc.param[CISTPL_POWER_VNOM]/10000)
+	    } else if (stk->dflt.vcc.present & (1 << CISTPL_POWER_VNOM)) {
+		if (stk->conf.Vcc != stk->dflt.vcc.param[CISTPL_POWER_VNOM] / 10000)
 		    goto next_entry;
 	    }
 	}
-	
-	if (cfg->vpp1.present & (1<<CISTPL_POWER_VNOM))
+
+	if (cfg->vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		cfg->vpp1.param[CISTPL_POWER_VNOM]/10000;
-	else if (dflt.vpp1.present & (1<<CISTPL_POWER_VNOM))
+		cfg->vpp1.param[CISTPL_POWER_VNOM] / 10000;
+	else if (stk->dflt.vpp1.present & (1 << CISTPL_POWER_VNOM))
 	    link->conf.Vpp1 = link->conf.Vpp2 =
-		dflt.vpp1.param[CISTPL_POWER_VNOM]/10000;
-	
-	if ((cfg->io.nwin > 0) || (dflt.io.nwin > 0)) {
-	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &dflt.io;
+		stk->dflt.vpp1.param[CISTPL_POWER_VNOM] / 10000;
+
+	if ((cfg->io.nwin > 0) || (stk->dflt.io.nwin > 0)) {
+	    cistpl_io_t *io = (cfg->io.nwin) ? &cfg->io : &stk->dflt.io;
 	    link->conf.ConfigIndex = cfg->index;
 	    link->io.BasePort1 = io->win[0].base;
 	    link->io.IOAddrLines = io->flags & CISTPL_IO_LINES_MASK;
@@ -307,23 +315,24 @@ void ide_config(dev_link_t *link)
 		if (pcmcia_request_io(link->handle, &link->io) != 0)
 			goto next_entry;
 		io_base = link->io.BasePort1;
-		ctl_base = link->io.BasePort1+0x0e;
+		ctl_base = link->io.BasePort1 + 0x0e;
 	    } else goto next_entry;
 	    /* If we've got this far, we're done */
 	    break;
 	}
-	
+
     next_entry:
-	if (cfg->flags & CISTPL_CFTABLE_DEFAULT) dflt = *cfg;
+	if (cfg->flags & CISTPL_CFTABLE_DEFAULT)
+	    memcpy(&stk->dflt, cfg, sizeof(stk->dflt));
 	if (pass) {
 	    CS_CHECK(GetNextTuple, pcmcia_get_next_tuple(handle, &tuple));
 	} else if (pcmcia_get_next_tuple(handle, &tuple) != 0) {
 	    CS_CHECK(GetFirstTuple, pcmcia_get_first_tuple(handle, &tuple));
-	    memset(&dflt, 0, sizeof(dflt));
+	    memset(&stk->dflt, 0, sizeof(stk->dflt));
 	    pass++;
 	}
     }
-    
+
     CS_CHECK(RequestIRQ, pcmcia_request_irq(handle, &link->irq));
     CS_CHECK(RequestConfiguration, pcmcia_request_configuration(handle, &link->conf));
 
@@ -336,25 +345,27 @@ void ide_config(dev_link_t *link)
     outb(0x02, ctl_base);
 
     /* special setup for KXLC005 card */
-    if (is_kme) outb(0x81, ctl_base+1);
+    if (is_kme)
+	outb(0x81, ctl_base+1);
 
     /* retry registration in case device is still spinning up */
     for (hd = -1, i = 0; i < 10; i++) {
 	hd = idecs_register(io_base, ctl_base, link->irq.AssignedIRQ);
 	if (hd >= 0) break;
 	if (link->io.NumPorts1 == 0x20) {
-	    outb(0x02, ctl_base+0x10);
-	    hd = idecs_register(io_base+0x10, ctl_base+0x10,
+	    outb(0x02, ctl_base + 0x10);
+	    hd = idecs_register(io_base + 0x10, ctl_base + 0x10,
 				link->irq.AssignedIRQ);
 	    if (hd >= 0) {
-		io_base += 0x10; ctl_base += 0x10;
+		io_base += 0x10;
+		ctl_base += 0x10;
 		break;
 	    }
 	}
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	schedule_timeout(HZ/10);
     }
-    
+
     if (hd < 0) {
 	printk(KERN_NOTICE "ide-cs: ide_register() at 0x%3lx & 0x%3lx"
 	       ", irq %u failed\n", io_base, ctl_base,
@@ -363,24 +374,29 @@ void ide_config(dev_link_t *link)
     }
 
     info->ndev = 1;
-    sprintf(info->node.dev_name, "hd%c", 'a'+(hd*2));
+    sprintf(info->node.dev_name, "hd%c", 'a' + (hd * 2));
     info->node.major = ide_major[hd];
     info->node.minor = 0;
     info->hd = hd;
     link->dev = &info->node;
     printk(KERN_INFO "ide-cs: %s: Vcc = %d.%d, Vpp = %d.%d\n",
-	   info->node.dev_name, link->conf.Vcc/10, link->conf.Vcc%10,
-	   link->conf.Vpp1/10, link->conf.Vpp1%10);
+	   info->node.dev_name, link->conf.Vcc / 10, link->conf.Vcc % 10,
+	   link->conf.Vpp1 / 10, link->conf.Vpp1 % 10);
 
     link->state &= ~DEV_CONFIG_PENDING;
+    kfree(stk);
     return;
-    
+
+err_mem:
+    printk(KERN_NOTICE "ide-cs: ide_config failed memory allocation\n");
+    goto failed;
+
 cs_failed:
     cs_error(link->handle, last_fn, last_ret);
 failed:
+    kfree(stk);
     ide_release(link);
     link->state &= ~DEV_CONFIG_PENDING;
-
 } /* ide_config */
 
 /*======================================================================