Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC driver

From: Corey Minyard
Date: Wed Jan 17 2018 - 21:58:16 EST


On 01/17/2018 06:16 PM, Wang, Haiyue wrote:


On 2018-01-17 23:59, Corey Minyard wrote:
On 01/17/2018 08:31 AM, Wang, Haiyue wrote:


On 2018-01-17 06:12, Corey Minyard wrote:
On 01/16/2018 02:59 PM, Corey Minyard wrote:
On 01/16/2018 05:43 AM, Haiyue Wang wrote:
The KCS (Keyboard Controller Style) interface is used to perform in-band
IPMI communication between a server host and its BMC (BaseBoard Management
Controllers).

This driver exposes the KCS interface on ASpeed SOCs (AST2400 and AST2500)
as a character device. Such SOCs are commonly used as BMCs and this driver
implements the BMC side of the KCS interface.

I thought we were going to unify the BMC ioctl interface? My preference would be to
create a file named include/uapi/linux/ipmi-bmc.h and add the following:

#define __IPMI_BMC_IOCTL_MAGICÂÂÂ 0xb1
#define IPMI_BMC_IOCTL_SMS_SET_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)

to make it the same as BT. Then in bt-bmc.h, set BT_BMC_IOCTL_SMS_ATN to
IPMI_BMC_IOCTL_SMS_SET_ATN. Then add the KCS ioctls in ipmi-bmc.h and
use that. That way we stay backward compatible but we are unified.

Since more KCS interfaces may come around, can you make the name more
specific? (I made this same error on bt-bmc,c, it should probably be renamed.)

How about these IOCTL definitions ? Is it more specific ?

#define IPMI_BMC_IOCTL_SET_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x00)
#define IPMI_BMC_IOCTL_CLEAR_SMS_ATN _IO(__IPMI_BMC_IOCTL_MAGIC, 0x01)
#define IPMI_BMC_IOCTL_FORCE_ABORT _IO(__IPMI_BMC_IOCTL_MAGIC, 0x02)


Those look good to me. If you could do the switchover to ipmi-bmc.h in a separate
patch, that would be cleaner. Then add the clear atn and force abort ioctls in the
patch to add the new driver.

Sound good?

-corey

If I understood correctly, still use KCS_BMC_IOCTL_xxx in kcs_bmc.h currently, then add a
patch for ipmi-bmc.h, and modify the bt_bmc.h together. Right ?


No, not exactly. Just add ipmi-bmc.h and put the ioctls you define above in it. No need for
kcs_bmc.h at all. We can then switch bt-bmc.c over to use the new ioctls later and remove
bt_bmc.h when all the software gets switched over.

-corey

Haiyue