* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®