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
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, . . . . . . . . . . . . [email protected]
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
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 <[email protected]>
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);
J?rn Engel <[email protected]> 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);
_
On Wed, 9 June 2004 12:27:21 -0700, Andrew Morton wrote:
> J?rn Engel <[email protected]> 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
J?rn Engel <[email protected]> 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 <[email protected]>
---
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)
{
_
On Thu, 10 June 2004 16:10:21 -0700, Andrew Morton wrote:
> J?rn Engel <[email protected]> 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
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 <[email protected]>
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 */
/*======================================================================
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
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
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
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
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 <[email protected]>
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 */
/*======================================================================
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
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 <[email protected]>
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 */
/*======================================================================
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
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 <[email protected]>
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 */
/*======================================================================
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
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 <[email protected]>
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 */
/*======================================================================
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 <[email protected]>
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 */
/*======================================================================
On Fri, 2004-06-11 at 09:10, Andrew Morton wrote:
> J?rn Engel <[email protected]> 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 <[email protected]>
...
> +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