Re: [PATCH 2.6.12-rc6] char: Add Dell Systems Management Base driver

[Brian Gerst <bgerst@didntduck.org>]

Doug Warzecha wrote:
> This patch adds the Dell Systems Management Base driver.
> 
> The Dell Systems Management Base driver is a character driver that
> provides support needed by Dell systems management software to manage
> certain Dell systems.  The driver implements ioctls for Dell systems
> management software to use to communicate with the driver.
> 
> This driver has been tested with Dell systems management software
> on a variety of Dell systems.
> 
> By making a contribution to this project, I certify that:
> The contribution was created in whole or in part by me and
> I have the right to submit it under the open source license
> indicated in the file.
> 
> Signed-off-by: Doug Warzecha <Douglas_Warzecha@dell.com>
> ---

> +	/* generate SMI */
> +	asm("pushl %ebx");
> +	asm("pushl %ecx");
> +	asm("rep" : : "b" (command_buffer_phys_addr));
> +	asm("rep" : : "c" (ci_cmd->signature));
> +	outb(ci_cmd->command_code, ci_cmd->command_address);
> +	asm("popl %ecx");
> +	asm("popl %ebx");

This is wrong.  GCC doesn't guarantee that any registers or stack state 
is preserved between asm statements.  What you really want is:

asm("outb %b0,%w1" : :
	"a" (ci_cmd->command_code),
	"d" (ci_cmd->command_address),
	"b" (command_buffer_phys_addr),
	"c" (ci_cmd->signature));

This way you don't need to explicitly push/pop %ebx and %ecx, since GCC 
will know they are being used.  If the SMI changes any of the registers 
then they need to be changed to outputs so GCC knows that they are 
clobbered.

--
				Brian Gerst