* [PATCH] IPMI driver update part 1, add per-channel IPMB addresses @ 2005-08-03 22:52 Corey Minyard 2005-08-04 5:59 ` Andrew Morton 0 siblings, 1 reply; 4+ messages in thread From: Corey Minyard @ 2005-08-03 22:52 UTC (permalink / raw) To: lkml, Andrew Morton [-- Attachment #1: Type: text/plain, Size: 1 bytes --] [-- Attachment #2: ipmi-per-channel-slave-address.patch --] [-- Type: unknown/unknown, Size: 13345 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses 2005-08-03 22:52 [PATCH] IPMI driver update part 1, add per-channel IPMB addresses Corey Minyard @ 2005-08-04 5:59 ` Andrew Morton 2005-08-04 13:24 ` Corey Minyard 0 siblings, 1 reply; 4+ messages in thread From: Andrew Morton @ 2005-08-04 5:59 UTC (permalink / raw) To: Corey Minyard; +Cc: linux-kernel 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... ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses 2005-08-04 5:59 ` Andrew Morton @ 2005-08-04 13:24 ` Corey Minyard 2005-08-04 13:35 ` Arjan van de Ven 0 siblings, 1 reply; 4+ messages in thread From: Corey Minyard @ 2005-08-04 13:24 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] IPMI driver update part 1, add per-channel IPMB addresses 2005-08-04 13:24 ` Corey Minyard @ 2005-08-04 13:35 ` Arjan van de Ven 0 siblings, 0 replies; 4+ messages in thread From: Arjan van de Ven @ 2005-08-04 13:35 UTC (permalink / raw) To: Corey Minyard; +Cc: Andrew Morton, linux-kernel > >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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-08-04 13:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2005-08-03 22:52 [PATCH] IPMI driver update part 1, add per-channel IPMB addresses Corey Minyard 2005-08-04 5:59 ` Andrew Morton 2005-08-04 13:24 ` Corey Minyard 2005-08-04 13:35 ` Arjan van de Ven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.