2.5.64: i2c-proc kills machine at boot

2.5.64: i2c-proc kills machine at boot

[Pavel Machek]

Hi!

If I turn #ifdef DEBUG in i2c_register_entry() into #if 1, it prints 

i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n

but boots.
								Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: 2.5.64: i2c-proc kills machine at boot

[=?unknown-8bit?Q?J=F6rn?= Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 1732 bytes --]

On Tue, 11 March 2003 11:47:22 +0100, Pavel Machek wrote:
> 
> If I turn #ifdef DEBUG in i2c_register_entry() into #if 1, it prints 
> 
> i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n
> 
> but boots.

That file need a lot of work anyway. On the shitlist of top stack
users, it holds ranks 3 and 9-11. Impressive.

$ make checkstack|grep i2o_proc
0xc0167db8 i2o_proc_read_ddm_table:                      sub    $0xb40,%esp
0xc0169ec8 i2o_proc_read_lan_mcast_addr:                 sub    $0x814,%esp
0xc016a56c i2o_proc_read_lan_alt_addr:                   sub    $0x814,%esp
0xc01681a0 i2o_proc_read_groups:                         sub    $0x810,%esp
0xc0168520 i2o_proc_read_users:                          sub    $0x20c,%esp
0xc0168630 i2o_proc_read_priv_msgs:                      sub    $0x18c,%esp
0xc016aaa4 i2o_proc_read_lan_hist_stats:                 sub    $0x160,%esp
0xc01689a4 i2o_proc_read_ddm_identity:                   sub    $0x130,%esp
0xc016835c i2o_proc_read_phys_device:                    sub    $0x10c,%esp
0xc0168740 i2o_proc_read_authorized_users:               sub    $0x10c,%esp

BTW: It depends on CONFIG_PCI, but doesn't state it. The patch below
should fix this.

It also isn't listed in the current MAINTAINERS file. Is i2o currently
unmaintained?

Jörn

-- 
The only real mistake is the one from which we learn nothing.
-- John Powell

--- drivers/message/i2o/Kconfig	Mon Feb 24 20:05:05 2003
+++ foo	Wed Mar 12 13:46:57 2003
@@ -65,7 +65,7 @@
 
 config I2O_PROC
 	tristate "I2O /proc support"
-	depends on I2O
+	depends on I2O && PCI
 	help
 	  If you say Y here and to "/proc file system support", you will be
 	  able to read I2O related information from the virtual directory

Re: 2.5.64: i2c-proc kills machine at boot

[Vojtech Pavlik]

On Wed, Mar 12, 2003 at 01:56:31PM +0100, Jörn Engel wrote:
> On Tue, 11 March 2003 11:47:22 +0100, Pavel Machek wrote:
> > 
> > If I turn #ifdef DEBUG in i2c_register_entry() into #if 1, it prints 
> > 
> > i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n
> > 
> > but boots.
> 
> That file need a lot of work anyway.

Pavel's talking about i2c, you about i2o.

> On the shitlist of top stack
> users, it holds ranks 3 and 9-11. Impressive.
> 
> $ make checkstack|grep i2o_proc
> 0xc0167db8 i2o_proc_read_ddm_table:                      sub    $0xb40,%esp
> 0xc0169ec8 i2o_proc_read_lan_mcast_addr:                 sub    $0x814,%esp
> 0xc016a56c i2o_proc_read_lan_alt_addr:                   sub    $0x814,%esp
> 0xc01681a0 i2o_proc_read_groups:                         sub    $0x810,%esp
> 0xc0168520 i2o_proc_read_users:                          sub    $0x20c,%esp
> 0xc0168630 i2o_proc_read_priv_msgs:                      sub    $0x18c,%esp
> 0xc016aaa4 i2o_proc_read_lan_hist_stats:                 sub    $0x160,%esp
> 0xc01689a4 i2o_proc_read_ddm_identity:                   sub    $0x130,%esp
> 0xc016835c i2o_proc_read_phys_device:                    sub    $0x10c,%esp
> 0xc0168740 i2o_proc_read_authorized_users:               sub    $0x10c,%esp
> 
> BTW: It depends on CONFIG_PCI, but doesn't state it. The patch below
> should fix this.
> 
> It also isn't listed in the current MAINTAINERS file. Is i2o currently
> unmaintained?
> 
> Jörn
> 
> -- 
> The only real mistake is the one from which we learn nothing.
> -- John Powell
> 
> --- drivers/message/i2o/Kconfig	Mon Feb 24 20:05:05 2003
> +++ foo	Wed Mar 12 13:46:57 2003
> @@ -65,7 +65,7 @@
>  
>  config I2O_PROC
>  	tristate "I2O /proc support"
> -	depends on I2O
> +	depends on I2O && PCI
>  	help
>  	  If you say Y here and to "/proc file system support", you will be
>  	  able to read I2O related information from the virtual directory
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Vojtech Pavlik
SuSE Labs

Re: 2.5.64: i2c-proc kills machine at boot

[=?unknown-8bit?Q?J=F6rn?= Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 202 bytes --]

On Wed, 12 March 2003 14:31:57 +0100, Vojtech Pavlik wrote:
> 
> Pavel's talking about i2c, you about i2o.

Doh! Thanks for pointing that out.

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

Re: 2.5.64: i2c-proc kills machine at boot

[Alan Cox]

On Wed, 2003-03-12 at 12:56, =?unknown-8bit?Q?J=F6rn?= Engel wrote:
> On Tue, 11 March 2003 11:47:22 +0100, Pavel Machek wrote:
> > 
> > If I turn #ifdef DEBUG in i2c_register_entry() into #if 1, it prints 
> > 
> > i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n
> > 
> > but boots.
> 
> That file need a lot of work anyway. On the shitlist of top stack
> users, it holds ranks 3 and 9-11. Impressive.

His problem is i2c not i2o. Also the i2o proc stuff while ugly isnt a
deep call nest or in irq context so not a big problem. It does want
fixing but thats a seperate matter

> It also isn't listed in the current MAINTAINERS file. Is i2o currently
> unmaintained?

Its kind of mine. Maintained is an overly strong word for it however, but I 
do take patches 8)

Re: 2.5.64: i2c-proc kills machine at boot

[=?unknown-8bit?Q?J=F6rn?= Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 6845 bytes --]

On Wed, 12 March 2003 16:03:19 +0000, Alan Cox wrote:
> 
> > It also isn't listed in the current MAINTAINERS file. Is i2o currently
> > unmaintained?
> 
> Its kind of mine. Maintained is an overly strong word for it however, but I 
> do take patches 8)

All right. The following is against 2.5.64, compiles and reduces the
worst stack offender to 0x190 bytes. It is untested though, I don't
have any hardware for it.

Jörn

-- 
"Translations are and will always be problematic. They inflict violence 
upon two languages." (translation from German)

--- linux-2.5.64/drivers/message/i2o/i2o_proc.c	Mon Feb 24 20:05:35 2003
+++ linux-2.5.64-i2o/drivers/message/i2o/i2o_proc.c	Wed Mar 12 17:09:26 2003
@@ -836,29 +836,33 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_exec_execute_ddm_table ddm_table[MAX_I2O_MODULES];
-	} result;
+	} *result;
 
 	i2o_exec_execute_ddm_table ddm_table;
 
 	spin_lock(&i2o_proc_lock);
 	len = 0;
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				c, ADAPTER_TID, 
 				0x0003, -1,
 				NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0x0003 Executing DDM List");
 		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "Tid   Module_type     Vendor Mod_id  Module_name             Vrs  Data_size Code_size\n");
-	ddm_table=result.ddm_table[0];
+	ddm_table=result->ddm_table[0];
 
-	for(i=0; i < result.row_count; ddm_table=result.ddm_table[++i])
+	for(i=0; i < result->row_count; ddm_table=result->ddm_table[++i])
 	{
 		len += sprintf(buf+len, "0x%03x ", ddm_table.ddm_tid & 0xFFF);
 
@@ -884,7 +888,8 @@
 	}
 
 	spin_unlock(&i2o_proc_lock);
-
+out:
+	kfree(result);
 	return len;
 }
 
@@ -1047,32 +1052,36 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_group_info group[256];
-	} result;
+	} *result;
 
 	spin_lock(&i2o_proc_lock);
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	len = 0;
 
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid, 0xF000, -1, NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len = i2o_report_query_status(buf+len, token, "0xF000 Params Descriptor");
 		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "#  Group   FieldCount RowCount Type   Add Del Clear\n");
 
-	for (i=0; i < result.row_count; i++)
+	for (i=0; i < result->row_count; i++)
 	{
 		len += sprintf(buf+len, "%-3d", i);
-		len += sprintf(buf+len, "0x%04X ", result.group[i].group_number);
-		len += sprintf(buf+len, "%10d ", result.group[i].field_count);
-		len += sprintf(buf+len, "%8d ",  result.group[i].row_count);
+		len += sprintf(buf+len, "0x%04X ", result->group[i].group_number);
+		len += sprintf(buf+len, "%10d ", result->group[i].field_count);
+		len += sprintf(buf+len, "%8d ",  result->group[i].row_count);
 
-		properties = result.group[i].properties;
+		properties = result->group[i].properties;
 		if (properties & 0x1)	len += sprintf(buf+len, "Table  ");
 				else	len += sprintf(buf+len, "Scalar ");
 		if (properties & 0x2)	len += sprintf(buf+len, " + ");
@@ -1085,11 +1094,12 @@
 		len += sprintf(buf+len, "\n");
 	}
 
-	if (result.more_flag)
+	if (result->more_flag)
 		len += sprintf(buf+len, "There is more...\n");
 
 	spin_unlock(&i2o_proc_lock);
-
+out:
+	kfree(result);
 	return len;
 }
 
@@ -1220,36 +1230,42 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_user_table user[64];
-	} result;
+	} *result;
 
 	spin_lock(&i2o_proc_lock);
 	len = 0;
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid,
 				0xF003, -1, NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0xF003 User Table");
 		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "#  Instance UserTid ClaimType\n");
 
-	for(i=0; i < result.row_count; i++)
+	for(i=0; i < result->row_count; i++)
 	{
 		len += sprintf(buf+len, "%-3d", i);
-		len += sprintf(buf+len, "%#8x ", result.user[i].instance);
-		len += sprintf(buf+len, "%#7x ", result.user[i].user_tid);
-		len += sprintf(buf+len, "%#9x\n", result.user[i].claim_type);
+		len += sprintf(buf+len, "%#8x ", result->user[i].instance);
+		len += sprintf(buf+len, "%#7x ", result->user[i].user_tid);
+		len += sprintf(buf+len, "%#9x\n", result->user[i].claim_type);
 	}
 
-	if (result.more_flag)
+	if (result->more_flag)
 		len += sprintf(buf+len, "There is more...\n");
 
 	spin_unlock(&i2o_proc_lock);
+out:
+	kfree(result);
 	return len;
 }
 
@@ -2264,24 +2280,28 @@
 		u16 row_count;
 		u16 more_flag;
 		u8  mc_addr[256][8];
-	} result;	
+	} *result;	
 
 	spin_lock(&i2o_proc_lock);	
 	len = 0;
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid, 0x0002, -1, 
-				NULL, 0, &result, sizeof(result));
+				NULL, 0, result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0x002 LAN Multicast MAC Address");
 		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
-	for (i = 0; i < result.row_count; i++)
+	for (i = 0; i < result->row_count; i++)
 	{
-		memcpy(mc_addr, result.mc_addr[i], 8);
+		memcpy(mc_addr, result->mc_addr[i], 8);
 
 		len += sprintf(buf+len, "MC MAC address[%d]: "
 			       "%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X\n",
@@ -2291,6 +2311,8 @@
 	}
 
 	spin_unlock(&i2o_proc_lock);
+out:
+	kfree(result);
 	return len;
 }
 
@@ -2495,24 +2517,28 @@
 		u16 row_count;
 		u16 more_flag;
 		u8  alt_addr[256][8];
-	} result;	
+	} *result;	
 
 	spin_lock(&i2o_proc_lock);	
 	len = 0;
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid,
-				0x0006, -1, NULL, 0, &result, sizeof(result));
+				0x0006, -1, NULL, 0, result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token, "0x0006 LAN Alternate Address (optional)");
 		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
-	for (i=0; i < result.row_count; i++)
+	for (i=0; i < result->row_count; i++)
 	{
-		memcpy(alt_addr,result.alt_addr[i],8);
+		memcpy(alt_addr,result->alt_addr[i],8);
 		len += sprintf(buf+len, "Alternate address[%d]: "
 			       "%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X\n",
 			       i, alt_addr[0], alt_addr[1], alt_addr[2],
@@ -2521,6 +2547,8 @@
 	}
 
 	spin_unlock(&i2o_proc_lock);
+out:
+	kfree(result);
 	return len;
 }
 

Re: 2.5.64: i2c-proc kills machine at boot

[Joern Engel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 7027 bytes --]

On Wed, 12 March 2003 11:06:50 -0800, Andrew Morton wrote:
> 
> > ...
> >  	spin_lock(&i2o_proc_lock);
> >  	len = 0;
> >  
> > +	result = kmalloc(sizeof(*result), GFP_KERNEL);
> > +	if(!result)
> > +		return -ENOMEM;
> > +
> 
> - sleeping allocation inside spinlock
> - forgotten unlock

Correct. This should fix it.

Jörn

-- 
If System.PrivateProfileString("",
"HKEY_CURRENT_USER\Software\Microsoft\Office\9.0\Word\Security", "Level") <>
"" Then  CommandBars("Macro").Controls("Security...").Enabled = False
-- from the Melissa-source

--- linux-2.5.64/drivers/message/i2o/i2o_proc.c	Mon Feb 24 20:05:35 2003
+++ linux-2.5.64-i2o/drivers/message/i2o/i2o_proc.c	Wed Mar 12 20:19:40 2003
@@ -836,10 +836,14 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_exec_execute_ddm_table ddm_table[MAX_I2O_MODULES];
-	} result;
+	} *result;
 
 	i2o_exec_execute_ddm_table ddm_table;
 
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
+
 	spin_lock(&i2o_proc_lock);
 	len = 0;
 
@@ -847,18 +851,17 @@
 				c, ADAPTER_TID, 
 				0x0003, -1,
 				NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0x0003 Executing DDM List");
-		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "Tid   Module_type     Vendor Mod_id  Module_name             Vrs  Data_size Code_size\n");
-	ddm_table=result.ddm_table[0];
+	ddm_table=result->ddm_table[0];
 
-	for(i=0; i < result.row_count; ddm_table=result.ddm_table[++i])
+	for(i=0; i < result->row_count; ddm_table=result->ddm_table[++i])
 	{
 		len += sprintf(buf+len, "0x%03x ", ddm_table.ddm_tid & 0xFFF);
 
@@ -882,9 +885,9 @@
 
 		len += sprintf(buf+len, "\n");
 	}
-
+out:
 	spin_unlock(&i2o_proc_lock);
-
+	kfree(result);
 	return len;
 }
 
@@ -1047,7 +1050,11 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_group_info group[256];
-	} result;
+	} *result;
+
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
 
 	spin_lock(&i2o_proc_lock);
 
@@ -1055,24 +1062,23 @@
 
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid, 0xF000, -1, NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len = i2o_report_query_status(buf+len, token, "0xF000 Params Descriptor");
-		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "#  Group   FieldCount RowCount Type   Add Del Clear\n");
 
-	for (i=0; i < result.row_count; i++)
+	for (i=0; i < result->row_count; i++)
 	{
 		len += sprintf(buf+len, "%-3d", i);
-		len += sprintf(buf+len, "0x%04X ", result.group[i].group_number);
-		len += sprintf(buf+len, "%10d ", result.group[i].field_count);
-		len += sprintf(buf+len, "%8d ",  result.group[i].row_count);
+		len += sprintf(buf+len, "0x%04X ", result->group[i].group_number);
+		len += sprintf(buf+len, "%10d ", result->group[i].field_count);
+		len += sprintf(buf+len, "%8d ",  result->group[i].row_count);
 
-		properties = result.group[i].properties;
+		properties = result->group[i].properties;
 		if (properties & 0x1)	len += sprintf(buf+len, "Table  ");
 				else	len += sprintf(buf+len, "Scalar ");
 		if (properties & 0x2)	len += sprintf(buf+len, " + ");
@@ -1085,11 +1091,11 @@
 		len += sprintf(buf+len, "\n");
 	}
 
-	if (result.more_flag)
+	if (result->more_flag)
 		len += sprintf(buf+len, "There is more...\n");
-
+out:
 	spin_unlock(&i2o_proc_lock);
-
+	kfree(result);
 	return len;
 }
 
@@ -1220,7 +1226,11 @@
 		u16 row_count;
 		u16 more_flag;
 		i2o_user_table user[64];
-	} result;
+	} *result;
+
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
 
 	spin_lock(&i2o_proc_lock);
 	len = 0;
@@ -1228,28 +1238,28 @@
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid,
 				0xF003, -1, NULL, 0,
-				&result, sizeof(result));
+				result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0xF003 User Table");
-		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
 	len += sprintf(buf+len, "#  Instance UserTid ClaimType\n");
 
-	for(i=0; i < result.row_count; i++)
+	for(i=0; i < result->row_count; i++)
 	{
 		len += sprintf(buf+len, "%-3d", i);
-		len += sprintf(buf+len, "%#8x ", result.user[i].instance);
-		len += sprintf(buf+len, "%#7x ", result.user[i].user_tid);
-		len += sprintf(buf+len, "%#9x\n", result.user[i].claim_type);
+		len += sprintf(buf+len, "%#8x ", result->user[i].instance);
+		len += sprintf(buf+len, "%#7x ", result->user[i].user_tid);
+		len += sprintf(buf+len, "%#9x\n", result->user[i].claim_type);
 	}
 
-	if (result.more_flag)
+	if (result->more_flag)
 		len += sprintf(buf+len, "There is more...\n");
-
+out:
 	spin_unlock(&i2o_proc_lock);
+	kfree(result);
 	return len;
 }
 
@@ -2264,24 +2274,27 @@
 		u16 row_count;
 		u16 more_flag;
 		u8  mc_addr[256][8];
-	} result;	
+	} *result;	
+
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
 
 	spin_lock(&i2o_proc_lock);	
 	len = 0;
 
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid, 0x0002, -1, 
-				NULL, 0, &result, sizeof(result));
+				NULL, 0, result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token,"0x002 LAN Multicast MAC Address");
-		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
-	for (i = 0; i < result.row_count; i++)
+	for (i = 0; i < result->row_count; i++)
 	{
-		memcpy(mc_addr, result.mc_addr[i], 8);
+		memcpy(mc_addr, result->mc_addr[i], 8);
 
 		len += sprintf(buf+len, "MC MAC address[%d]: "
 			       "%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X\n",
@@ -2289,8 +2302,9 @@
 			       mc_addr[3], mc_addr[4], mc_addr[5],
 			       mc_addr[6], mc_addr[7]);
 	}
-
+out:
 	spin_unlock(&i2o_proc_lock);
+	kfree(result);
 	return len;
 }
 
@@ -2495,32 +2509,36 @@
 		u16 row_count;
 		u16 more_flag;
 		u8  alt_addr[256][8];
-	} result;	
+	} *result;	
+
+	result = kmalloc(sizeof(*result), GFP_KERNEL);
+	if(!result)
+		return -ENOMEM;
 
 	spin_lock(&i2o_proc_lock);	
 	len = 0;
 
 	token = i2o_query_table(I2O_PARAMS_TABLE_GET,
 				d->controller, d->lct_data.tid,
-				0x0006, -1, NULL, 0, &result, sizeof(result));
+				0x0006, -1, NULL, 0, result, sizeof(*result));
 
 	if (token < 0) {
 		len += i2o_report_query_status(buf+len, token, "0x0006 LAN Alternate Address (optional)");
-		spin_unlock(&i2o_proc_lock);
-		return len;
+		goto out;
 	}
 
-	for (i=0; i < result.row_count; i++)
+	for (i=0; i < result->row_count; i++)
 	{
-		memcpy(alt_addr,result.alt_addr[i],8);
+		memcpy(alt_addr,result->alt_addr[i],8);
 		len += sprintf(buf+len, "Alternate address[%d]: "
 			       "%02X:%02X:%02X:%02X:%02X:%02X:%02X:%02X\n",
 			       i, alt_addr[0], alt_addr[1], alt_addr[2],
 			       alt_addr[3], alt_addr[4], alt_addr[5],
 			       alt_addr[6], alt_addr[7]);
 	}
-
+out:
 	spin_unlock(&i2o_proc_lock);
+	kfree(result);
 	return len;
 }
 

Re: 2.5.64: i2c-proc kills machine at boot

[Alan Cox]

On Wed, 2003-03-12 at 16:14, =?unknown-8bit?Q?J=F6rn?= Engel wrote:
> On Wed, 12 March 2003 16:03:19 +0000, Alan Cox wrote:
> > 
> > > It also isn't listed in the current MAINTAINERS file. Is i2o currently
> > > unmaintained?
> > 
> > Its kind of mine. Maintained is an overly strong word for it however, but I 
> > do take patches 8)
> 
> All right. The following is against 2.5.64, compiles and reduces the
> worst stack offender to 0x190 bytes. It is untested though, I don't
> have any hardware for it.

I have hardware however so I'll give it a check

Re: 2.5.64: i2c-proc kills machine at boot

[Christoph Hellwig]

On Tue, Mar 11, 2003 at 11:47:22AM +0100, Pavel Machek wrote:
> Hi!
> 
> If I turn #ifdef DEBUG in i2c_register_entry() into #if 1, it prints 
> 
> i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n
> 
> but boots.

The following patch should fix it (and make the code actually readable..):


--- 1.16/drivers/i2c/i2c-proc.c	Thu Feb 20 15:02:00 2003
+++ edited/drivers/i2c/i2c-proc.c	Wed Mar 12 15:25:38 2003
@@ -35,8 +35,6 @@
 #include <linux/i2c-proc.h>
 #include <asm/uaccess.h>
 
-static int i2c_create_name(char **name, const char *prefix,
-			       struct i2c_adapter *adapter, int addr);
 static int i2c_parse_reals(int *nrels, void *buffer, int bufsize,
 			       long *results, int magnitude);
 static int i2c_write_reals(int nrels, void *buffer, size_t *bufsize,
@@ -54,15 +52,6 @@
 
 static struct i2c_client *i2c_clients[SENSORS_ENTRY_MAX];
 
-static ctl_table sysctl_table[] = {
-	{CTL_DEV, "dev", NULL, 0, 0555},
-	{0},
-	{DEV_SENSORS, "sensors", NULL, 0, 0555},
-	{0},
-	{0, NULL, NULL, 0, 0555},
-	{0}
-};
-
 static ctl_table i2c_proc_dev_sensors[] = {
 	{SENSORS_CHIPS, "chips", NULL, 0, 0644, NULL, &i2c_proc_chips,
 	 &i2c_sysctl_chips},
@@ -87,36 +76,40 @@
    (for a LM78 chip on the ISA bus at port 0x310), or lm75-i2c-3-4e (for
    a LM75 chip on the third i2c bus at address 0x4e).  
    name is allocated first. */
-static int i2c_create_name(char **name, const char *prefix,
-			struct i2c_adapter *adapter, int addr)
+static char *generate_name(struct i2c_client *client, const char *prefix)
 {
-	char name_buffer[50];
-	int id, i, end;
-	if (i2c_is_isa_adapter(adapter))
+	struct i2c_adapter *adapter = client->adapter;
+	int addr = client->addr;
+	char name_buffer[50], *name;
+
+	if (i2c_is_isa_adapter(adapter)) {
 		sprintf(name_buffer, "%s-isa-%04x", prefix, addr);
-	else if (!adapter->algo->smbus_xfer && !adapter->algo->master_xfer) {
-		/* dummy adapter, generate prefix */
+	} else if (adapter->algo->smbus_xfer || adapter->algo->master_xfer) {
+		int id = i2c_adapter_id(adapter);
+		if (id < 0)
+			return ERR_PTR(-ENOENT);
+		sprintf(name_buffer, "%s-i2c-%d-%02x", prefix, id, addr);
+	} else {	/* dummy adapter, generate prefix */
+		int end, i;
+
 		sprintf(name_buffer, "%s-", prefix);
 		end = strlen(name_buffer);
-		for(i = 0; i < 32; i++) {
-			if(adapter->algo->name[i] == ' ')
+
+		for (i = 0; i < 32; i++) {
+			if (adapter->algo->name[i] == ' ')
 				break;
 			name_buffer[end++] = tolower(adapter->algo->name[i]);
 		}
+
 		name_buffer[end] = 0;
 		sprintf(name_buffer + end, "-%04x", addr);
-	} else {
-		if ((id = i2c_adapter_id(adapter)) < 0)
-			return -ENOENT;
-		sprintf(name_buffer, "%s-i2c-%d-%02x", prefix, id, addr);
-	}
-	*name = kmalloc(strlen(name_buffer) + 1, GFP_KERNEL);
-	if (!*name) {
-		printk (KERN_WARNING "i2c_create_name: not enough memory\n");
-		return -ENOMEM;
 	}
-	strcpy(*name, name_buffer);
-	return 0;
+
+	name = kmalloc(strlen(name_buffer) + 1, GFP_KERNEL);
+	if (unlikely(!name))
+		return ERR_PTR(-ENOMEM);
+	strcpy(name, name_buffer);
+	return name;
 }
 
 /* This rather complex function must be called when you want to add an entry
@@ -127,93 +120,80 @@
    If any driver wants subdirectories within the newly created directory,
    this function must be updated!  */
 int i2c_register_entry(struct i2c_client *client, const char *prefix,
-			   ctl_table * ctl_template)
+		       struct ctl_table *leaf)
 {
-	int i, res, len, id;
-	ctl_table *new_table, *client_tbl, *tbl;
-	char *name;
-	struct ctl_table_header *new_header;
-
-	if ((res = i2c_create_name(&name, prefix, client->adapter,
-				       client->addr))) return res;
-
-	for (id = 0; id < SENSORS_ENTRY_MAX; id++)
-		if (!i2c_entries[id]) {
-			break;
-		}
-	if (id == SENSORS_ENTRY_MAX) {
-		kfree(name);
-		return -ENOMEM;
-	}
-
-	id += 256;
-	
-	len = 0;
-	while (ctl_template[len].procname)
-		len++;
-	if (!(new_table = kmalloc(sizeof(sysctl_table) + sizeof(ctl_table) * (len + 1), 
-				  GFP_KERNEL))) {
-		kfree(name);
-		return -ENOMEM;
-	}
-
-	memcpy(new_table, sysctl_table, sizeof(sysctl_table));
-	tbl = new_table; /* sys/ */
-	tbl = tbl->child = tbl + 2; /* dev/ */
-	tbl = tbl->child = tbl + 2; /* sensors/ */	
-       	client_tbl = tbl->child = tbl + 2; /* XX-chip-YY-ZZ/ */
-
-	client_tbl->procname = name;
-	client_tbl->ctl_name = id;
-	client_tbl->child = client_tbl + 2;
-
-	/* Next the client sysctls. --km */
-	tbl = client_tbl->child;
-	memcpy(tbl, ctl_template, sizeof(ctl_table) * (len+1));
-	for (i = 0; i < len; i++)
-		tbl[i].extra2 = client;
-
-	if (!(new_header = register_sysctl_table(new_table, 0))) {
-		printk(KERN_ERR "i2c-proc.o: error: sysctl interface not supported by kernel!\n");
-		kfree(new_table);
-		kfree(name);
-		return -EPERM;
-	}
-
- 	i2c_entries[id - 256] = new_header;
-
-	i2c_clients[id - 256] = client;
-
-#ifdef DEBUG
-	if (!new_header || !new_header->ctl_table ||
-	    !new_header->ctl_table->child ||
-	    !new_header->ctl_table->child->child ||
-	    !new_header->ctl_table->child->child->de ) {
-		printk
-		    (KERN_ERR "i2c-proc.o: NULL pointer when trying to install fill_inode fix!\n");
-		return id;
-	}
-#endif				/* DEBUG */
-	client_tbl->de->owner = client->driver->owner;
-	return id;
+	struct { struct ctl_table root[2], dev[2], sensors[2]; } *tbl;
+	struct ctl_table_header *hdr;
+	struct ctl_table *tmp;
+	const char *name;
+	int id;
+
+	name = generate_name(client, prefix);
+	if (IS_ERR(name))
+		return PTR_ERR(name);
+
+	for (id = 0; id < SENSORS_ENTRY_MAX; id++) {
+		if (!i2c_entries[id])
+			goto free_slot;
+	}
+
+	goto out_free_name;
+
+ free_slot:
+	tbl = kmalloc(sizeof(*tbl), GFP_KERNEL);
+	if (unlikely(!tbl))
+		goto out_free_name;
+	memset(tbl, 0, sizeof(*tbl));
+
+	for (tmp = leaf; tmp->ctl_name; tmp++)
+		tmp->extra2 = client;
+
+	tbl->sensors->ctl_name = id+256;
+	tbl->sensors->procname = name;
+	tbl->sensors->mode = 0555;
+	tbl->sensors->child = leaf;
+
+	tbl->dev->ctl_name = DEV_SENSORS;
+	tbl->dev->procname = "sensors";
+	tbl->dev->mode = 0555;
+	tbl->dev->child = tbl->sensors;
+
+	tbl->root->ctl_name = CTL_DEV;
+	tbl->root->procname = "dev";
+	tbl->root->mode = 0555;
+	tbl->root->child = tbl->dev;
+
+	hdr = register_sysctl_table(tbl->root, 0);
+	if (unlikely(!hdr))
+		goto out_free_tbl;
+
+	i2c_entries[id] = hdr;
+	i2c_clients[id] = client;
+
+	return (id + 256);	/* XXX(hch) why?? */
+
+ out_free_tbl:
+	kfree(tbl);
+ out_free_name:
+	kfree(name);
+	return -ENOMEM;
 }
 
 void i2c_deregister_entry(int id)
 {
-	ctl_table *table;
-	char *temp;
+	id -= 256;
 
-	id -= 256;	
 	if (i2c_entries[id]) {
-		table = i2c_entries[id]->ctl_table;
-		unregister_sysctl_table(i2c_entries[id]);
-		/* 2-step kfree needed to keep gcc happy about const points */
-		(const char *) temp = table[4].procname;
-		kfree(temp);
-		kfree(table);
-		i2c_entries[id] = NULL;
-		i2c_clients[id] = NULL;
+		struct ctl_table_header *hdr = i2c_entries[id];
+		struct ctl_table *tbl = hdr->ctl_table;
+
+		unregister_sysctl_table(hdr);
+		kfree(tbl->child->child->procname);
+		kfree(tbl); /* actually the whole anonymous struct */
 	}
+
+	i2c_entries[id] = NULL;
+	i2c_clients[id] = NULL;
 }
 
 static int i2c_proc_chips(ctl_table * ctl, int write, struct file *filp,