[PATCH] IPMI driver update part 1, add per-channel IPMB addresses

[PATCH] IPMI driver update part 1, add per-channel IPMB addresses

[Corey Minyard]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: ipmi-per-channel-slave-address.patch --]
[-- Type: unknown/unknown, Size: 13345 bytes --]

Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses

[Andrew Morton]

Corey Minyard <minyard@acm.org> wrote:
>
> ipmi-per-channel-slave-address.patch  unknown/unknown (13533 bytes)]

Could you fix up the mimetype, please?  It makes it hard for various email
clients.

> IPMI allows multiple IPMB channels on a single interface, and
> each channel might have a different IPMB address.  However, the
> driver has only one IPMB address that it uses for everything.
> This patch adds new IOCTLS and a new internal interface for
> setting per-channel IPMB addresses and LUNs.  New systems are
> coming out with support for multiple IPMB channels, and they
> are broken without this patch.
> 
> ...
> +	for (i=0; i<IPMI_MAX_CHANNELS; i++)

Preferred coding style is actually

	for (i = 0; i < IPMI_MAX_CHANNELS; i++)

but we've kinda lost that fight in drivers :(

> +#define IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 24, struct ipmi_channel_lun_address_set)
> +#define IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 25, struct ipmi_channel_lun_address_set)
> +#define IPMICTL_SET_MY_CHANNEL_LUN_CMD	   _IOR(IPMI_IOC_MAGIC, 26, struct ipmi_channel_lun_address_set)
> +#define IPMICTL_GET_MY_CHANNEL_LUN_CMD	   _IOR(IPMI_IOC_MAGIC, 27, struct ipmi_channel_lun_address_set)

Are these all OK wrt compat handling?

>  	case IPMICTL_SET_MY_ADDRESS_CMD:
>  	{
>  		unsigned int val;
> ...
>  	case IPMICTL_GET_MY_ADDRESS_CMD:
>  	{
> -		unsigned int val;
> +		unsigned int  val;
> +		unsigned char rval;
> ...
>  	case IPMICTL_GET_MY_LUN_CMD:
>  	{
> -		unsigned int val;
> +		unsigned int  val;
> +		unsigned char rval;
> +
> ...
> +	case IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD:
> +	{
> +		struct ipmi_channel_lun_address_set val;
> ...
> +	case IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD:
> +	{
> +		struct ipmi_channel_lun_address_set val;
> ...
> +	case IPMICTL_SET_MY_CHANNEL_LUN_CMD:
> +	{
> +		struct ipmi_channel_lun_address_set val;
> ...
> +	case IPMICTL_GET_MY_CHANNEL_LUN_CMD:
> +	{
> +		struct ipmi_channel_lun_address_set val;
> ...
>  	case IPMICTL_SET_TIMING_PARMS_CMD:
>  	{
>  		struct ipmi_timing_parms parms;
> 

Be aware that this function will use more stack space than it needs to: gcc
will create a separate stack slot for all the above locals.

Hence it would be better to declare them all at the start of the function. 
Faster, too - less dcache footprint.

Maybe not as nice from a purist point of view, but it does allow you to
lose those braces in the switch statement...

Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses

[Corey Minyard]

Andrew Morton wrote:

>Corey Minyard <minyard@acm.org> wrote:
>  
>
>>ipmi-per-channel-slave-address.patch  unknown/unknown (13533 bytes)]
>>    
>>
>
>Could you fix up the mimetype, please?  It makes it hard for various email
>clients.
>  
>
Dang, you switch to a new mail client and everything is screwed up.  Sorry.

>  
>
>>IPMI allows multiple IPMB channels on a single interface, and
>>each channel might have a different IPMB address.  However, the
>>driver has only one IPMB address that it uses for everything.
>>This patch adds new IOCTLS and a new internal interface for
>>setting per-channel IPMB addresses and LUNs.  New systems are
>>coming out with support for multiple IPMB channels, and they
>>are broken without this patch.
>>
>>...
>>+	for (i=0; i<IPMI_MAX_CHANNELS; i++)
>>    
>>
>
>Preferred coding style is actually
>
>	for (i = 0; i < IPMI_MAX_CHANNELS; i++)
>
>but we've kinda lost that fight in drivers :(
>  
>
Ok, I'll see what I can do.  It's the wrong way all over the driver 
right now.

>  
>
>>+#define IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 24, struct ipmi_channel_lun_address_set)
>>+#define IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD _IOR(IPMI_IOC_MAGIC, 25, struct ipmi_channel_lun_address_set)
>>+#define IPMICTL_SET_MY_CHANNEL_LUN_CMD	   _IOR(IPMI_IOC_MAGIC, 26, struct ipmi_channel_lun_address_set)
>>+#define IPMICTL_GET_MY_CHANNEL_LUN_CMD	   _IOR(IPMI_IOC_MAGIC, 27, struct ipmi_channel_lun_address_set)
>>    
>>
>
>Are these all OK wrt compat handling?
>  
>
Yes, it is a structure of an unsigned short and an unsigned char, so it 
should be ok.

>  
>
>> 	case IPMICTL_SET_MY_ADDRESS_CMD:
>> 	{
>> 		unsigned int val;
>>...
>> 	case IPMICTL_GET_MY_ADDRESS_CMD:
>> 	{
>>-		unsigned int val;
>>+		unsigned int  val;
>>+		unsigned char rval;
>>...
>> 	case IPMICTL_GET_MY_LUN_CMD:
>> 	{
>>-		unsigned int val;
>>+		unsigned int  val;
>>+		unsigned char rval;
>>+
>>...
>>+	case IPMICTL_SET_MY_CHANNEL_ADDRESS_CMD:
>>+	{
>>+		struct ipmi_channel_lun_address_set val;
>>...
>>+	case IPMICTL_GET_MY_CHANNEL_ADDRESS_CMD:
>>+	{
>>+		struct ipmi_channel_lun_address_set val;
>>...
>>+	case IPMICTL_SET_MY_CHANNEL_LUN_CMD:
>>+	{
>>+		struct ipmi_channel_lun_address_set val;
>>...
>>+	case IPMICTL_GET_MY_CHANNEL_LUN_CMD:
>>+	{
>>+		struct ipmi_channel_lun_address_set val;
>>...
>> 	case IPMICTL_SET_TIMING_PARMS_CMD:
>> 	{
>> 		struct ipmi_timing_parms parms;
>>
>>    
>>
>
>Be aware that this function will use more stack space than it needs to: gcc
>will create a separate stack slot for all the above locals.
>
>Hence it would be better to declare them all at the start of the function. 
>Faster, too - less dcache footprint.
>
>Maybe not as nice from a purist point of view, but it does allow you to
>lose those braces in the switch statement...
>  
>
Hmm, I assumed that gcc would optimize and allocate the stack as it 
needed it without waste.  Ok, easy enough to fix.

Thanks,

-Corey

Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses

[Arjan van de Ven]

> >Be aware that this function will use more stack space than it needs to: gcc
> >will create a separate stack slot for all the above locals.
> >
> >Hence it would be better to declare them all at the start of the function. 
> >Faster, too - less dcache footprint.
> >
> >Maybe not as nice from a purist point of view, but it does allow you to
> >lose those braces in the switch statement...
> >  
> >
> Hmm, I assumed that gcc would optimize and allocate the stack as it 
> needed it without waste.  Ok, easy enough to fix.

latest gcc does the right thing; older gcc don't though.