This patch reduces the stack usage of the function smc91c92_event() in
smc91c92_cs driver from 3540 to 132. Currently this is the highest
stack user in linux-2.6.12-rc2-mm3. I used a patched version of gcc
3.4.3 on i386 with -fno-unit-at-a-time disabled.
The patch has only been compile tested. It would be nice to get
feedback if someone that owns the hardware can actually test this
patch.
Acked-by: J?rn Engel <[email protected]>
Acked-by: Randy Dunlap <[email protected]>
Signed-off-by: Yum Rayan <[email protected]>
smc91c92_cs.c | 287 ++++++++++++++++++++++++++++++++++++++--------------------
1 files changed, 189 insertions(+), 98 deletions(-)
diff -Nupr -X dontdiff
linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c
linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c
--- linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c 2005-04-14
22:15:43.000000000 -0700
+++ linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c 2005-04-20
18:12:00.000000000 -0700
@@ -127,6 +127,12 @@ struct smc_private {
int rx_ovrn;
};
+struct smc_cfg_mem {
+ tuple_t tuple;
+ cisparse_t parse;
+ u_char buf[255];
+};
+
/* Special definitions for Megahertz multifunction cards */
#define MEGAHERTZ_ISR 0x0380
@@ -498,14 +504,24 @@ static int mhz_mfc_config(dev_link_t *li
{
struct net_device *dev = link->priv;
struct smc_private *smc = netdev_priv(dev);
- tuple_t tuple;
- cisparse_t parse;
- u_char buf[255];
- cistpl_cftable_entry_t *cf = &parse.cftable_entry;
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ cisparse_t *parse;
+ cistpl_cftable_entry_t *cf;
+ u_char *buf;
win_req_t req;
memreq_t mem;
int i, k;
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return CS_OUT_OF_RESOURCE;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ cf = &parse->cftable_entry;
+ buf = cfg_mem->buf;
+
link->conf.Attributes |= CONF_ENABLE_SPKR;
link->conf.Status = CCSR_AUDIO_ENA;
link->irq.Attributes =
@@ -514,12 +530,12 @@ static int mhz_mfc_config(dev_link_t *li
link->io.Attributes2 = IO_DATA_PATH_WIDTH_8;
link->io.NumPorts2 = 8;
- tuple.Attributes = tuple.TupleOffset = 0;
- tuple.TupleData = (cisdata_t *)buf;
- tuple.TupleDataMax = sizeof(buf);
- tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
+ tuple->Attributes = tuple->TupleOffset = 0;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 255;
+ tuple->DesiredTuple = CISTPL_CFTABLE_ENTRY;
- i = first_tuple(link->handle, &tuple, &parse);
+ i = first_tuple(link->handle, tuple, parse);
/* The Megahertz combo cards have modem-like CIS entries, so
we have to explicitly try a bunch of port combinations. */
while (i == CS_SUCCESS) {
@@ -532,10 +548,10 @@ static int mhz_mfc_config(dev_link_t *li
if (i == CS_SUCCESS) break;
}
if (i == CS_SUCCESS) break;
- i = next_tuple(link->handle, &tuple, &parse);
+ i = next_tuple(link->handle, tuple, parse);
}
if (i != CS_SUCCESS)
- return i;
+ goto free_cfg_mem;
dev->base_addr = link->io.BasePort1;
/* Allocate a memory window, for accessing the ISR */
@@ -544,7 +560,7 @@ static int mhz_mfc_config(dev_link_t *li
req.AccessSpeed = 0;
i = pcmcia_request_window(&link->handle, &req, &link->win);
if (i != CS_SUCCESS)
- return i;
+ goto free_cfg_mem;
smc->base = ioremap(req.Base, req.Size);
mem.CardOffset = mem.Page = 0;
if (smc->manfid == MANFID_MOTOROLA)
@@ -556,6 +572,8 @@ static int mhz_mfc_config(dev_link_t *li
&& (smc->cardid == PRODID_MEGAHERTZ_EM3288))
mhz_3288_power(link);
+free_cfg_mem:
+ kfree(cfg_mem);
return i;
}
@@ -563,39 +581,61 @@ static int mhz_setup(dev_link_t *link)
{
client_handle_t handle = link->handle;
struct net_device *dev = link->priv;
- tuple_t tuple;
- cisparse_t parse;
- u_char buf[255], *station_addr;
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ cisparse_t *parse;
+ u_char *buf, *station_addr;
+ int rc;
+
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return -1;
- tuple.Attributes = tuple.TupleOffset = 0;
- tuple.TupleData = buf;
- tuple.TupleDataMax = sizeof(buf);
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ buf = cfg_mem->buf;
+
+ tuple->Attributes = tuple->TupleOffset = 0;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 255;
/* Read the station address from the CIS. It is stored as the last
(fourth) string in the Version 1 Version/ID tuple. */
- tuple.DesiredTuple = CISTPL_VERS_1;
- if (first_tuple(handle, &tuple, &parse) != CS_SUCCESS)
- return -1;
+ tuple->DesiredTuple = CISTPL_VERS_1;
+ if (first_tuple(handle, tuple, parse) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
/* Ugh -- the EM1144 card has two VERS_1 tuples!?! */
- if (next_tuple(handle, &tuple, &parse) != CS_SUCCESS)
- first_tuple(handle, &tuple, &parse);
- if (parse.version_1.ns > 3) {
- station_addr = parse.version_1.str + parse.version_1.ofs[3];
- if (cvt_ascii_address(dev, station_addr) == 0)
- return 0;
+ if (next_tuple(handle, tuple, parse) != CS_SUCCESS)
+ first_tuple(handle, tuple, parse);
+ if (parse->version_1.ns > 3) {
+ station_addr = parse->version_1.str + parse->version_1.ofs[3];
+ if (cvt_ascii_address(dev, station_addr) == 0) {
+ rc = 0;
+ goto free_cfg_mem;
+ }
}
/* Another possibility: for the EM3288, in a special tuple */
- tuple.DesiredTuple = 0x81;
- if (pcmcia_get_first_tuple(handle, &tuple) != CS_SUCCESS)
- return -1;
- if (pcmcia_get_tuple_data(handle, &tuple) != CS_SUCCESS)
- return -1;
+ tuple->DesiredTuple = 0x81;
+ if (pcmcia_get_first_tuple(handle, tuple) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
+ if (pcmcia_get_tuple_data(handle, tuple) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
buf[12] = '\0';
- if (cvt_ascii_address(dev, buf) == 0)
- return 0;
-
- return -1;
+ if (cvt_ascii_address(dev, buf) == 0) {
+ rc = 0;
+ goto free_cfg_mem;
+ }
+ rc = -1;
+free_cfg_mem:
+ kfree(cfg_mem);
+ return rc;
}
/*======================================================================
@@ -665,19 +705,29 @@ static int mot_setup(dev_link_t *link)
static int smc_config(dev_link_t *link)
{
struct net_device *dev = link->priv;
- tuple_t tuple;
- cisparse_t parse;
- u_char buf[255];
- cistpl_cftable_entry_t *cf = &parse.cftable_entry;
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ cisparse_t *parse;
+ cistpl_cftable_entry_t *cf;
+ u_char *buf;
int i;
- tuple.Attributes = tuple.TupleOffset = 0;
- tuple.TupleData = (cisdata_t *)buf;
- tuple.TupleDataMax = sizeof(buf);
- tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return CS_OUT_OF_RESOURCE;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ cf = &parse->cftable_entry;
+ buf = cfg_mem->buf;
+
+ tuple->Attributes = tuple->TupleOffset = 0;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 255;
+ tuple->DesiredTuple = CISTPL_CFTABLE_ENTRY;
link->io.NumPorts1 = 16;
- i = first_tuple(link->handle, &tuple, &parse);
+ i = first_tuple(link->handle, tuple, parse);
while (i != CS_NO_MORE_ITEMS) {
if (i == CS_SUCCESS) {
link->conf.ConfigIndex = cf->index;
@@ -686,10 +736,12 @@ static int smc_config(dev_link_t *link)
i = pcmcia_request_io(link->handle, &link->io);
if (i == CS_SUCCESS) break;
}
- i = next_tuple(link->handle, &tuple, &parse);
+ i = next_tuple(link->handle, tuple, parse);
}
if (i == CS_SUCCESS)
dev->base_addr = link->io.BasePort1;
+
+ kfree(cfg_mem);
return i;
}
@@ -697,41 +749,58 @@ static int smc_setup(dev_link_t *link)
{
client_handle_t handle = link->handle;
struct net_device *dev = link->priv;
- tuple_t tuple;
- cisparse_t parse;
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ cisparse_t *parse;
cistpl_lan_node_id_t *node_id;
- u_char buf[255], *station_addr;
- int i;
+ u_char *buf, *station_addr;
+ int i, rc;
- tuple.Attributes = tuple.TupleOffset = 0;
- tuple.TupleData = buf;
- tuple.TupleDataMax = sizeof(buf);
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return CS_OUT_OF_RESOURCE;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ buf = cfg_mem->buf;
+
+ tuple->Attributes = tuple->TupleOffset = 0;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 255;
/* Check for a LAN function extension tuple */
- tuple.DesiredTuple = CISTPL_FUNCE;
- i = first_tuple(handle, &tuple, &parse);
+ tuple->DesiredTuple = CISTPL_FUNCE;
+ i = first_tuple(handle, tuple, parse);
while (i == CS_SUCCESS) {
- if (parse.funce.type == CISTPL_FUNCE_LAN_NODE_ID)
+ if (parse->funce.type == CISTPL_FUNCE_LAN_NODE_ID)
break;
- i = next_tuple(handle, &tuple, &parse);
+ i = next_tuple(handle, tuple, parse);
}
if (i == CS_SUCCESS) {
- node_id = (cistpl_lan_node_id_t *)parse.funce.data;
+ node_id = (cistpl_lan_node_id_t *)parse->funce.data;
if (node_id->nb == 6) {
for (i = 0; i < 6; i++)
dev->dev_addr[i] = node_id->id[i];
- return 0;
+ rc = 0;
+ goto free_cfg_mem;
}
}
/* Try the third string in the Version 1 Version/ID tuple. */
- tuple.DesiredTuple = CISTPL_VERS_1;
- if (first_tuple(handle, &tuple, &parse) != CS_SUCCESS)
- return -1;
- station_addr = parse.version_1.str + parse.version_1.ofs[2];
- if (cvt_ascii_address(dev, station_addr) == 0)
- return 0;
+ tuple->DesiredTuple = CISTPL_VERS_1;
+ if (first_tuple(handle, tuple, parse) != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
+ }
+ station_addr = parse->version_1.str + parse->version_1.ofs[2];
+ if (cvt_ascii_address(dev, station_addr) == 0) {
+ rc = 0;
+ goto free_cfg_mem;
+ }
- return -1;
+ rc = -1;
+free_cfg_mem:
+ kfree(cfg_mem);
+ return rc;
}
/*====================================================================*/
@@ -773,26 +842,36 @@ static int osi_setup(dev_link_t *link, u
{
client_handle_t handle = link->handle;
struct net_device *dev = link->priv;
- tuple_t tuple;
- u_char buf[255];
- int i;
-
- tuple.Attributes = TUPLE_RETURN_COMMON;
- tuple.TupleData = buf;
- tuple.TupleDataMax = sizeof(buf);
- tuple.TupleOffset = 0;
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ u_char *buf;
+ int i, rc;
+
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return -1;
+
+ tuple = &cfg_mem->tuple;
+ buf = cfg_mem->buf;
+
+ tuple->Attributes = TUPLE_RETURN_COMMON;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 255;
+ tuple->TupleOffset = 0;
/* Read the station address from tuple 0x90, subtuple 0x04 */
- tuple.DesiredTuple = 0x90;
- i = pcmcia_get_first_tuple(handle, &tuple);
+ tuple->DesiredTuple = 0x90;
+ i = pcmcia_get_first_tuple(handle, tuple);
while (i == CS_SUCCESS) {
- i = pcmcia_get_tuple_data(handle, &tuple);
+ i = pcmcia_get_tuple_data(handle, tuple);
if ((i != CS_SUCCESS) || (buf[0] == 0x04))
break;
- i = pcmcia_get_next_tuple(handle, &tuple);
+ i = pcmcia_get_next_tuple(handle, tuple);
+ }
+ if (i != CS_SUCCESS) {
+ rc = -1;
+ goto free_cfg_mem;
}
- if (i != CS_SUCCESS)
- return -1;
for (i = 0; i < 6; i++)
dev->dev_addr[i] = buf[i+2];
@@ -814,8 +893,10 @@ static int osi_setup(dev_link_t *link, u
inw(link->io.BasePort1 + OSITECH_AUI_PWR),
inw(link->io.BasePort1 + OSITECH_RESET_ISR));
}
-
- return 0;
+ rc = 0;
+free_cfg_mem:
+ kfree(cfg_mem);
+ return rc;
}
/*======================================================================
@@ -887,9 +968,10 @@ static void smc91c92_config(dev_link_t *
client_handle_t handle = link->handle;
struct net_device *dev = link->priv;
struct smc_private *smc = netdev_priv(dev);
- tuple_t tuple;
- cisparse_t parse;
- u_short buf[32];
+ struct smc_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ cisparse_t *parse;
+ u_char *buf;
char *name;
int i, j, rev;
kio_addr_t ioaddr;
@@ -897,21 +979,29 @@ static void smc91c92_config(dev_link_t *
DEBUG(0, "smc91c92_config(0x%p)\n", link);
- tuple.Attributes = tuple.TupleOffset = 0;
- tuple.TupleData = (cisdata_t *)buf;
- tuple.TupleDataMax = sizeof(buf);
+ cfg_mem = kmalloc(sizeof(struct smc_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ goto config_failed;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ buf = cfg_mem->buf;
+
+ tuple->Attributes = tuple->TupleOffset = 0;
+ tuple->TupleData = (cisdata_t *)buf;
+ tuple->TupleDataMax = 64;
- tuple.DesiredTuple = CISTPL_CONFIG;
- i = first_tuple(handle, &tuple, &parse);
+ tuple->DesiredTuple = CISTPL_CONFIG;
+ i = first_tuple(handle, tuple, parse);
CS_EXIT_TEST(i, ParseTuple, config_failed);
- link->conf.ConfigBase = parse.config.base;
- link->conf.Present = parse.config.rmask[0];
+ link->conf.ConfigBase = parse->config.base;
+ link->conf.Present = parse->config.rmask[0];
- tuple.DesiredTuple = CISTPL_MANFID;
- tuple.Attributes = TUPLE_RETURN_COMMON;
- if (first_tuple(handle, &tuple, &parse) == CS_SUCCESS) {
- smc->manfid = parse.manfid.manf;
- smc->cardid = parse.manfid.card;
+ tuple->DesiredTuple = CISTPL_MANFID;
+ tuple->Attributes = TUPLE_RETURN_COMMON;
+ if (first_tuple(handle, tuple, parse) == CS_SUCCESS) {
+ smc->manfid = parse->manfid.manf;
+ smc->cardid = parse->manfid.card;
}
/* Configure card */
@@ -1046,7 +1136,7 @@ static void smc91c92_config(dev_link_t *
printk(KERN_NOTICE " No MII transceivers found!\n");
}
}
-
+ kfree(cfg_mem);
return;
config_undo:
@@ -1054,6 +1144,7 @@ config_undo:
config_failed: /* CS_EXIT_TEST() calls jump to here... */
smc91c92_release(link);
link->state &= ~DEV_CONFIG_PENDING;
+ kfree(cfg_mem);
} /* smc91c92_config */
This patch reduces the stack usage of the function serial_event() in
serial_cs from 2212 to 228. I used a patched version of gcc 3.4.3 on
i386 with -fno-unit-at-a-time disabled.
This patch is only compile tested. It would be nice to get feedback
from someone that owns the hardware and would like to test it.
Acked-by: Randy Dunlap <[email protected]>
Signed-off-by: Yum Rayan <[email protected]>
serial_cs.c | 170 ++++++++++++++++++++++++++++++++++++++----------------------
1 files changed, 110 insertions(+), 60 deletions(-)
diff -Nupr -X dontdiff
linux-2.6.12-rc2-mm3.a/drivers/serial/serial_cs.c
linux-2.6.12-rc2-mm3.b/drivers/serial/serial_cs.c
--- linux-2.6.12-rc2-mm3.a/drivers/serial/serial_cs.c 2005-04-19
23:20:14.000000000 -0700
+++ linux-2.6.12-rc2-mm3.b/drivers/serial/serial_cs.c 2005-04-20
23:22:19.000000000 -0700
@@ -110,6 +110,13 @@ struct serial_info {
int line[4];
};
+struct serial_cfg_mem {
+ tuple_t tuple;
+ cisparse_t parse;
+ u_char buf[256];
+};
+
+
static void serial_config(dev_link_t * link);
static int serial_event(event_t event, int priority,
event_callback_args_t * args);
@@ -388,14 +395,24 @@ static int simple_config(dev_link_t *lin
static int size_table[2] = { 8, 16 };
client_handle_t handle = link->handle;
struct serial_info *info = link->priv;
- tuple_t tuple;
- u_char buf[256];
- cisparse_t parse;
- cistpl_cftable_entry_t *cf = &parse.cftable_entry;
+ struct serial_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ u_char *buf;
+ cisparse_t *parse;
+ cistpl_cftable_entry_t *cf;
config_info_t config;
int i, j, try;
int s;
+ cfg_mem = kmalloc(sizeof(struct serial_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return -1;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ cf = &parse->cftable_entry;
+ buf = cfg_mem->buf;
+
/* If the card is already configured, look up the port and irq */
i = pcmcia_get_configuration_info(handle, &config);
if ((i == CS_SUCCESS) && (config.Attributes & CONF_VALID_CLIENT)) {
@@ -408,21 +425,23 @@ static int simple_config(dev_link_t *lin
port = config.BasePort1 + 0x28;
info->slave = 1;
}
- if (info->slave)
+ if (info->slave) {
+ kfree(cfg_mem);
return setup_serial(handle, info, port, config.AssignedIRQ);
+ }
}
link->conf.Vcc = config.Vcc;
/* First pass: look for a config entry that looks normal. */
- tuple.TupleData = (cisdata_t *) buf;
- tuple.TupleOffset = 0;
- tuple.TupleDataMax = 255;
- tuple.Attributes = 0;
- tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
+ tuple->TupleData = (cisdata_t *) buf;
+ tuple->TupleOffset = 0;
+ tuple->TupleDataMax = 255;
+ tuple->Attributes = 0;
+ tuple->DesiredTuple = CISTPL_CFTABLE_ENTRY;
/* Two tries: without IO aliases, then with aliases */
for (s = 0; s < 2; s++) {
for (try = 0; try < 2; try++) {
- i = first_tuple(handle, &tuple, &parse);
+ i = first_tuple(handle, tuple, parse);
while (i != CS_NO_MORE_ITEMS) {
if (i != CS_SUCCESS)
goto next_entry;
@@ -440,14 +459,14 @@ static int simple_config(dev_link_t *lin
goto found_port;
}
next_entry:
- i = next_tuple(handle, &tuple, &parse);
+ i = next_tuple(handle, tuple, parse);
}
}
}
/* Second pass: try to find an entry that isn't picky about
its base address, then try to grab any standard serial port
address, and finally try to get any free port. */
- i = first_tuple(handle, &tuple, &parse);
+ i = first_tuple(handle, tuple, parse);
while (i != CS_NO_MORE_ITEMS) {
if ((i == CS_SUCCESS) && (cf->io.nwin > 0) &&
((cf->io.flags & CISTPL_IO_LINES_MASK) <= 3)) {
@@ -460,7 +479,7 @@ next_entry:
goto found_port;
}
}
- i = next_tuple(handle, &tuple, &parse);
+ i = next_tuple(handle, tuple, parse);
}
found_port:
@@ -468,6 +487,7 @@ next_entry:
printk(KERN_NOTICE
"serial_cs: no usable port range found, giving up\n");
cs_error(link->handle, RequestIO, i);
+ kfree(cfg_mem);
return -1;
}
@@ -481,9 +501,10 @@ next_entry:
i = pcmcia_request_configuration(link->handle, &link->conf);
if (i != CS_SUCCESS) {
cs_error(link->handle, RequestConfiguration, i);
+ kfree(cfg_mem);
return -1;
}
-
+ kfree(cfg_mem);
return setup_serial(handle, info, link->io.BasePort1, link->irq.AssignedIRQ);
}
@@ -491,29 +512,39 @@ static int multi_config(dev_link_t * lin
{
client_handle_t handle = link->handle;
struct serial_info *info = link->priv;
- tuple_t tuple;
- u_char buf[256];
- cisparse_t parse;
- cistpl_cftable_entry_t *cf = &parse.cftable_entry;
+ struct serial_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ u_char *buf;
+ cisparse_t *parse;
+ cistpl_cftable_entry_t *cf;
config_info_t config;
- int i, base2 = 0;
+ int i, rc, base2 = 0;
+
+ cfg_mem = kmalloc(sizeof(struct serial_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ return -1;
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ cf = &parse->cftable_entry;
+ buf = cfg_mem->buf;
i = pcmcia_get_configuration_info(handle, &config);
if (i != CS_SUCCESS) {
cs_error(handle, GetConfigurationInfo, i);
- return -1;
+ rc = -1;
+ goto free_cfg_mem;
}
link->conf.Vcc = config.Vcc;
- tuple.TupleData = (cisdata_t *) buf;
- tuple.TupleOffset = 0;
- tuple.TupleDataMax = 255;
- tuple.Attributes = 0;
- tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
+ tuple->TupleData = (cisdata_t *) buf;
+ tuple->TupleOffset = 0;
+ tuple->TupleDataMax = 255;
+ tuple->Attributes = 0;
+ tuple->DesiredTuple = CISTPL_CFTABLE_ENTRY;
/* First, look for a generic full-sized window */
link->io.NumPorts1 = info->multi * 8;
- i = first_tuple(handle, &tuple, &parse);
+ i = first_tuple(handle, tuple, parse);
while (i != CS_NO_MORE_ITEMS) {
/* The quad port cards have bad CIS's, so just look for a
window larger than 8 ports and assume it will be right */
@@ -528,14 +559,14 @@ static int multi_config(dev_link_t * lin
if (i == CS_SUCCESS)
break;
}
- i = next_tuple(handle, &tuple, &parse);
+ i = next_tuple(handle, tuple, parse);
}
/* If that didn't work, look for two windows */
if (i != CS_SUCCESS) {
link->io.NumPorts1 = link->io.NumPorts2 = 8;
info->multi = 2;
- i = first_tuple(handle, &tuple, &parse);
+ i = first_tuple(handle, tuple, parse);
while (i != CS_NO_MORE_ITEMS) {
if ((i == CS_SUCCESS) && (cf->io.nwin == 2)) {
link->conf.ConfigIndex = cf->index;
@@ -548,13 +579,14 @@ static int multi_config(dev_link_t * lin
if (i == CS_SUCCESS)
break;
}
- i = next_tuple(handle, &tuple, &parse);
+ i = next_tuple(handle, tuple, parse);
}
}
if (i != CS_SUCCESS) {
cs_error(link->handle, RequestIO, i);
- return -1;
+ rc = -1;
+ goto free_cfg_mem;
}
i = pcmcia_request_irq(link->handle, &link->irq);
@@ -572,7 +604,8 @@ static int multi_config(dev_link_t * lin
i = pcmcia_request_configuration(link->handle, &link->conf);
if (i != CS_SUCCESS) {
cs_error(link->handle, RequestConfiguration, i);
- return -1;
+ rc = -1;
+ goto free_cfg_mem;
}
/* The Oxford Semiconductor OXCF950 cards are in fact single-port:
@@ -593,17 +626,22 @@ static int multi_config(dev_link_t * lin
}
info->c950ctrl = base2;
wakeup_card(info);
- return 0;
+ rc = 0;
+ goto free_cfg_mem;
}
setup_serial(handle, info, link->io.BasePort1, link->irq.AssignedIRQ);
/* The Nokia cards are not really multiport cards */
- if (info->manfid == MANFID_NOKIA)
- return 0;
+ if (info->manfid == MANFID_NOKIA) {
+ rc = 0;
+ goto free_cfg_mem;
+ }
for (i = 0; i < info->multi - 1; i++)
setup_serial(handle, info, base2 + (8 * i), link->irq.AssignedIRQ);
-
- return 0;
+ rc = 0;
+free_cfg_mem:
+ kfree(cfg_mem);
+ return rc;
}
/*======================================================================
@@ -618,39 +656,49 @@ void serial_config(dev_link_t * link)
{
client_handle_t handle = link->handle;
struct serial_info *info = link->priv;
- tuple_t tuple;
- u_short buf[128];
- cisparse_t parse;
- cistpl_cftable_entry_t *cf = &parse.cftable_entry;
+ struct serial_cfg_mem *cfg_mem;
+ tuple_t *tuple;
+ u_char *buf;
+ cisparse_t *parse;
+ cistpl_cftable_entry_t *cf;
int i, last_ret, last_fn;
DEBUG(0, "serial_config(0x%p)\n", link);
- tuple.TupleData = (cisdata_t *) buf;
- tuple.TupleOffset = 0;
- tuple.TupleDataMax = 255;
- tuple.Attributes = 0;
+ cfg_mem = kmalloc(sizeof(struct serial_cfg_mem), GFP_KERNEL);
+ if (!cfg_mem)
+ goto failed;
+
+ tuple = &cfg_mem->tuple;
+ parse = &cfg_mem->parse;
+ cf = &parse->cftable_entry;
+ buf = cfg_mem->buf;
+
+ tuple->TupleData = (cisdata_t *) buf;
+ tuple->TupleOffset = 0;
+ tuple->TupleDataMax = 255;
+ tuple->Attributes = 0;
/* Get configuration register information */
- tuple.DesiredTuple = CISTPL_CONFIG;
- last_ret = first_tuple(handle, &tuple, &parse);
+ tuple->DesiredTuple = CISTPL_CONFIG;
+ last_ret = first_tuple(handle, tuple, parse);
if (last_ret != CS_SUCCESS) {
last_fn = ParseTuple;
goto cs_failed;
}
- link->conf.ConfigBase = parse.config.base;
- link->conf.Present = parse.config.rmask[0];
+ link->conf.ConfigBase = parse->config.base;
+ link->conf.Present = parse->config.rmask[0];
/* Configure card */
link->state |= DEV_CONFIG;
/* Is this a compliant multifunction card? */
- tuple.DesiredTuple = CISTPL_LONGLINK_MFC;
- tuple.Attributes = TUPLE_RETURN_COMMON | TUPLE_RETURN_LINK;
- info->multi = (first_tuple(handle, &tuple, &parse) == CS_SUCCESS);
+ tuple->DesiredTuple = CISTPL_LONGLINK_MFC;
+ tuple->Attributes = TUPLE_RETURN_COMMON | TUPLE_RETURN_LINK;
+ info->multi = (first_tuple(handle, tuple, parse) == CS_SUCCESS);
/* Is this a multiport card? */
- tuple.DesiredTuple = CISTPL_MANFID;
- if (first_tuple(handle, &tuple, &parse) == CS_SUCCESS) {
+ tuple->DesiredTuple = CISTPL_MANFID;
+ if (first_tuple(handle, tuple, parse) == CS_SUCCESS) {
info->manfid = le16_to_cpu(buf[0]);
info->prodid = le16_to_cpu(buf[1]);
for (i = 0; i < MULTI_COUNT; i++)
@@ -663,13 +711,13 @@ void serial_config(dev_link_t * link)
/* Another check for dual-serial cards: look for either serial or
multifunction cards that ask for appropriate IO port ranges */
- tuple.DesiredTuple = CISTPL_FUNCID;
+ tuple->DesiredTuple = CISTPL_FUNCID;
if ((info->multi == 0) &&
- ((first_tuple(handle, &tuple, &parse) != CS_SUCCESS) ||
- (parse.funcid.func == CISTPL_FUNCID_MULTI) ||
- (parse.funcid.func == CISTPL_FUNCID_SERIAL))) {
- tuple.DesiredTuple = CISTPL_CFTABLE_ENTRY;
- if (first_tuple(handle, &tuple, &parse) == CS_SUCCESS) {
+ ((first_tuple(handle, tuple, parse) != CS_SUCCESS) ||
+ (parse->funcid.func == CISTPL_FUNCID_MULTI) ||
+ (parse->funcid.func == CISTPL_FUNCID_SERIAL))) {
+ tuple->DesiredTuple = CISTPL_CFTABLE_ENTRY;
+ if (first_tuple(handle, tuple, parse) == CS_SUCCESS) {
if ((cf->io.nwin == 1) && (cf->io.win[0].len % 8 == 0))
info->multi = cf->io.win[0].len >> 3;
if ((cf->io.nwin == 2) && (cf->io.win[0].len == 8) &&
@@ -704,6 +752,7 @@ void serial_config(dev_link_t * link)
link->dev = &info->node[0];
link->state &= ~DEV_CONFIG_PENDING;
+ kfree(cfg_mem);
return;
cs_failed:
@@ -711,6 +760,7 @@ void serial_config(dev_link_t * link)
failed:
serial_remove(link);
link->state &= ~DEV_CONFIG_PENDING;
+ kfree(cfg_mem);
}
/*======================================================================
On Friday 22 April 2005 01:02, Yum Rayan wrote:
> This patch reduces the stack usage of the function smc91c92_event() in
> smc91c92_cs driver from 3540 to 132. Currently this is the highest
> stack user in linux-2.6.12-rc2-mm3. I used a patched version of gcc
> 3.4.3 on i386 with -fno-unit-at-a-time disabled.
>
> The patch has only been compile tested. It would be nice to get
> feedback if someone that owns the hardware can actually test this
> patch.
>
> Acked-by: J?rn Engel <[email protected]>
> Acked-by: Randy Dunlap <[email protected]>
> Signed-off-by: Yum Rayan <[email protected]>
>
> smc91c92_cs.c | 287 ++++++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 189 insertions(+), 98 deletions(-)
>
> diff -Nupr -X dontdiff
> linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c
> linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c
> --- linux-2.6.12-rc2-mm3.a/drivers/net/pcmcia/smc91c92_cs.c 2005-04-14
> 22:15:43.000000000 -0700
> +++ linux-2.6.12-rc2-mm3.b/drivers/net/pcmcia/smc91c92_cs.c 2005-04-20
> 18:12:00.000000000 -0700
> @@ -127,6 +127,12 @@ struct smc_private {
> int rx_ovrn;
> };
>
> +struct smc_cfg_mem {
> + tuple_t tuple;
> + cisparse_t parse;
> + u_char buf[255];
> +};
> +
> /* Special definitions for Megahertz multifunction cards */
> #define MEGAHERTZ_ISR 0x0380
>
> @@ -498,14 +504,24 @@ static int mhz_mfc_config(dev_link_t *li
> {
> struct net_device *dev = link->priv;
> struct smc_private *smc = netdev_priv(dev);
> - tuple_t tuple;
> - cisparse_t parse;
> - u_char buf[255];
> - cistpl_cftable_entry_t *cf = &parse.cftable_entry;
> + struct smc_cfg_mem *cfg_mem;
> + tuple_t *tuple;
> + cisparse_t *parse;
> + cistpl_cftable_entry_t *cf;
> + u_char *buf;
I do it this way:
int f()
{
- tuple_t tuple;
- cisparse_t parse;
- u_char buf[255];
+ struct {
+ tuple_t tuple;
+ cisparse_t parse;
+ u_char buf[255];
+ } local;
+ local = kmalloc(sizeof(*local),...); if(!local)...
...
- ? ? tuple.Attributes = tuple.TupleOffset = 0;
- ? ? tuple.TupleData = (cisdata_t *)buf;
- ? ? tuple.TupleDataMax = sizeof(buf);
+ ? ? local->tuple.Attributes = local->tuple.TupleOffset = 0;
+ ? ? local->tuple.TupleData = (cisdata_t *)local->buf;
+ ? ? local->tuple.TupleDataMax = sizeof(local->buf);
I see the following advantages:
1) struct is unnamed and local to function
2) Variables do not change their type, the just sit in local-> now.
I can just add 'local->' to each affected variable,
without "it was an object, now it is a pointer" changes.
No need to replace . with ->, remove &, etc.
3) I do not need to do this part of your patch which adds more locals:
+ ? ?tuple_t *tuple;
+ ? ?cisparse_t *parse;
+ ? ?cistpl_cftable_entry_t *cf;
+ ? ?u_char *buf;
...
+ ? ?tuple = &cfg_mem->tuple;
+ ? ?parse = &cfg_mem->parse;
+ ? ?buf = cfg_mem->buf;
4) in resulting asm one base pointer instead of many will require
less registers
Look into nfs4_proc_unlink_setup() for example.
I see that Trond do not use locally declared struct there,
but otherwise it is done as described above.
--
vda
On Fri, 22 April 2005 11:22:51 +0300, Denis Vlasenko wrote:
>
> I do it this way:
>
> int f()
> {
> - tuple_t tuple;
> - cisparse_t parse;
> - u_char buf[255];
> + struct {
> + tuple_t tuple;
> + cisparse_t parse;
> + u_char buf[255];
> + } local;
> + local = kmalloc(sizeof(*local),...); if(!local)...
> ...
> - ? ? tuple.Attributes = tuple.TupleOffset = 0;
> - ? ? tuple.TupleData = (cisdata_t *)buf;
> - ? ? tuple.TupleDataMax = sizeof(buf);
> + ? ? local->tuple.Attributes = local->tuple.TupleOffset = 0;
> + ? ? local->tuple.TupleData = (cisdata_t *)local->buf;
> + ? ? local->tuple.TupleDataMax = sizeof(local->buf);
>
> I see the following advantages:
>
> 1) struct is unnamed and local to function
> 2) Variables do not change their type, the just sit in local-> now.
> I can just add 'local->' to each affected variable,
> without "it was an object, now it is a pointer" changes.
> No need to replace . with ->, remove &, etc.
I'd have proposed the same, before reading further down in the patch.
Basically, the driver is full of duplication, so the exact same struct
can be used several times. Therefore, the downsides of your approach
seem to prevail.
> 3) I do not need to do this part of your patch which adds more locals:
> + ? ?tuple_t *tuple;
> + ? ?cisparse_t *parse;
> + ? ?cistpl_cftable_entry_t *cf;
> + ? ?u_char *buf;
> ...
> + ? ?tuple = &cfg_mem->tuple;
> + ? ?parse = &cfg_mem->parse;
> + ? ?buf = cfg_mem->buf;
> 4) in resulting asm one base pointer instead of many will require
> less registers
Yup. There are thousands of detail to improve in that driver. It's
current maintainership (there is none) may explain that state.
J?rn
--
Fantasy is more important than knowledge. Knowledge is limited,
while fantasy embraces the whole world.
-- Albert Einstein
On Saturday 23 April 2005 03:12, J?rn Engel wrote:
> > 1) struct is unnamed and local to function
> > 2) Variables do not change their type, the just sit in local-> now.
> > I can just add 'local->' to each affected variable,
> > without "it was an object, now it is a pointer" changes.
> > No need to replace . with ->, remove &, etc.
>
> I'd have proposed the same, before reading further down in the patch.
> Basically, the driver is full of duplication, so the exact same struct
> can be used several times. Therefore, the downsides of your approach
> seem to prevail.
What downsides? I must admit I do not understand your answer here.
Let me stay on the subject of converting large stack onjects
to kmalloc()ed ones, without reference to current state
of code in a particular module.
Basically, these objects are local to function. We do not
introduce struct as an 'object'. It merely aggregates
all locals we decided to move to kmalloc space,
only because it's easier that way to allocate and reference
all of them in C.
Thus, even if there are many functions with similar
set of locals, it makes little sense to declare common struct.
IOW, I wouldn't do this:
struct big {
large_t large;
huge_t huge;
};
int f() {
struct big *local;
local = kmalloc(sizeof(big),...);
...
}
int g() {
struct big *local;
local = kmalloc(sizeof(big),...);
...
}
For one, what will happen when you will need to add
another local in one function only?
Instead, I'd do it like I described in previous mail:
int f() {
struct {
large_t large;
huge_t huge;
} *local;
local = kmalloc(sizeof(*local),...);
...
}
int g() {
struct {
large_t large;
huge_t huge;
} *local;
local = kmalloc(sizeof(*local),...);
...
}
--
vda
On 4/23/05, Denis Vlasenko <[email protected]> wrote:
> On Saturday 23 April 2005 03:12, J?rn Engel wrote:
>
> > > 1) struct is unnamed and local to function
> > > 2) Variables do not change their type, the just sit in local-> now.
> > > I can just add 'local->' to each affected variable,
> > > without "it was an object, now it is a pointer" changes.
> > > No need to replace . with ->, remove &, etc.
> >
> > I'd have proposed the same, before reading further down in the patch.
> > Basically, the driver is full of duplication, so the exact same struct
> > can be used several times. Therefore, the downsides of your approach
> > seem to prevail.
>
> What downsides? I must admit I do not understand your answer here.
>
> Let me stay on the subject of converting large stack onjects
> to kmalloc()ed ones, without reference to current state
> of code in a particular module.
>
> Basically, these objects are local to function. We do not
And seem to be always used in conjuction with each other, not alone
across various functions in this driver alone but also across various
card services drivers.
> introduce struct as an 'object'. It merely aggregates
> all locals we decided to move to kmalloc space,
> only because it's easier that way to allocate and reference
> all of them in C.
>
> Thus, even if there are many functions with similar
> set of locals, it makes little sense to declare common struct.
> IOW, I wouldn't do this:
>
> struct big {
> large_t large;
> huge_t huge;
> };
>
> int f() {
> struct big *local;
> local = kmalloc(sizeof(big),...);
> ...
> }
>
> int g() {
> struct big *local;
> local = kmalloc(sizeof(big),...);
> ...
> }
>
> For one, what will happen when you will need to add
> another local in one function only?
struct another_local_struct *another_local;
another_local = kmalloc(sizeof(sruct another_local_struct),...);
or if single kmalloc is preferred, then:
struct another_big_struct {
struct old_big_struct old_big;
struct new_big_struct new_big;
} ;
I'd agree with your suggestion had it been "struct big", or "struct
huge" or "struct stack" or something like that, but does not seem
appropriate in this particular case.
Regards,
Rayan
On Sat, 23 April 2005 18:21:30 +0300, Denis Vlasenko wrote:
> On Saturday 23 April 2005 03:12, J?rn Engel wrote:
>
> > > 1) struct is unnamed and local to function
> > > 2) Variables do not change their type, the just sit in local-> now.
> > > I can just add 'local->' to each affected variable,
> > > without "it was an object, now it is a pointer" changes.
> > > No need to replace . with ->, remove &, etc.
> >
> > I'd have proposed the same, before reading further down in the patch.
> > Basically, the driver is full of duplication, so the exact same struct
> > can be used several times. Therefore, the downsides of your approach
> > seem to prevail.
>
> What downsides? I must admit I do not understand your answer here.
1. Read the patch. All of it.
2. Virtually the same identical variables are stuffed into the stack
frames of:
o mhz_mfc_config,
o mhz_setup,
o mot_setup,
o smc_setup,
o osi_setup and
o smc91c92_config.
That is six functions. If it were just one, I would definitely agree
with you. For two functions, well, it wouldn't really matter either
way. But should six functions all copy the exact same struct six
different times instead of referencing a single globally defined one?
Naa, that's barely an advantage.
> Instead, I'd do it like I described in previous mail:
If you would actually like to do something, please provide further
patches to that driver. It sucks. It sucks so bad, that it's hardly
possible to change anything without improving it. There are much
grosser things to clean up than the one we're discussing right now.
J?rn
--
He who knows others is wise.
He who knows himself is enlightened.
-- Lao Tsu