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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6391cfa-0ea2-f8a6-5ac2-961de59ad3cc@acm.org>
Date:   Wed, 17 Jan 2018 20:58:07 -0600
From:   Corey Minyard <minyard@....org>
To:     "Wang, Haiyue" <haiyue.wang@...ux.intel.com>, joel@....id.au,
        openbmc@...ts.ozlabs.org, openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org
Cc:     andriy.shevchenko@...el.com
Subject: Re: [PATCH arm/aspeed/ast2500 v1] ipmi: add an Aspeed KCS IPMI BMC
 driver

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