smc91c92_cs: Reduce stack usage in smc91c92_event()

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.

 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.

 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);
}

/*======================================================================

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.

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

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

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.

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

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.

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