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

[Corey Minyard]

Andrew Morton wrote:

> Corey Minyard <minyard@xxxxxxx> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/