From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59120) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHAWV-000452-LP for qemu-devel@nongnu.org; Thu, 07 Jan 2016 08:22:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHAWQ-0005T7-Ka for qemu-devel@nongnu.org; Thu, 07 Jan 2016 08:22:39 -0500 Received: from e06smtp06.uk.ibm.com ([195.75.94.102]:55688) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHAWQ-0005Sv-C8 for qemu-devel@nongnu.org; Thu, 07 Jan 2016 08:22:34 -0500 Received: from localhost by e06smtp06.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 7 Jan 2016 13:22:32 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 0F5F11B0806E for ; Thu, 7 Jan 2016 13:23:12 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u07DMUwu7012838 for ; Thu, 7 Jan 2016 13:22:30 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u07DMTF9030135 for ; Thu, 7 Jan 2016 06:22:30 -0700 Date: Thu, 7 Jan 2016 14:22:26 +0100 From: Greg Kurz Message-ID: <20160107142226.6593ad69@bahia.local> In-Reply-To: <568D59FB.9040303@fr.ibm.com> References: <1452015002-28493-3-git-send-email-clg@fr.ibm.com> <20160106105539.4bee4e9f@bahia.local> <568D59FB.9040303@fr.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/8] ipmi: add get and set SENSOR_TYPE commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?Q8OpZHJpYw==?= Le Goater Cc: Corey Minyard , qemu-devel@nongnu.org, "Michael S. Tsirkin" On Wed, 6 Jan 2016 19:16:27 +0100 C=C3=A9dric Le Goater wrote: > On 01/06/2016 10:55 AM, Greg Kurz wrote: > > On Tue, 5 Jan 2016 18:29:56 +0100 > > C=C3=A9dric Le Goater wrote: > >=20 > >> Signed-off-by: C=C3=A9dric Le Goater > >> --- > >=20 > > Acked-by: Greg Kurz > >=20 > > Just some minor comments on the form below. > >=20 > >=20 > >> hw/ipmi/ipmi_bmc_sim.c | 51 +++++++++++++++++++++++++++++++++++++++++= +++++++-- > >> 1 file changed, 49 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/ipmi/ipmi_bmc_sim.c b/hw/ipmi/ipmi_bmc_sim.c > >> index 559e1398d669..061db8437479 100644 > >> --- a/hw/ipmi/ipmi_bmc_sim.c > >> +++ b/hw/ipmi/ipmi_bmc_sim.c > >> @@ -37,13 +37,15 @@ > >> #define IPMI_CMD_CHASSIS_CONTROL 0x02 > >> > >> #define IPMI_NETFN_SENSOR_EVENT 0x04 > >> -#define IPMI_NETFN_SENSOR_EVENT_MAXCMD 0x2e > >> +#define IPMI_NETFN_SENSOR_EVENT_MAXCMD 0x30 > >> > >=20 > > Maybe IPMI_NETFN_SENSOR_EVENT_MAXCMD should be defined... >=20 Just to clarify: my point is to ensure someone doesn't forget to bump MAXCMD when adding a new command. > These are maximums for each IPMI family of commands (netfn). See > the arrays at the end of the file : >=20 > static const IPMICmdHandler *_cmds[*_MAXCMD]=20 >=20 > You'll get the idea. >=20 I've looked and I have the impression that the *_MAXCMD macros are not even needed: since the *_cmds[] arrays are static, why not using ARRAY_SIZE() to set .cmd_nums in the *_netfn structs instead ? Maybe something to be done in a followup patch. > >> #define IPMI_CMD_SET_SENSOR_EVT_ENABLE 0x28 > >> #define IPMI_CMD_GET_SENSOR_EVT_ENABLE 0x29 > >> #define IPMI_CMD_REARM_SENSOR_EVTS 0x2a > >> #define IPMI_CMD_GET_SENSOR_EVT_STATUS 0x2b > >> #define IPMI_CMD_GET_SENSOR_READING 0x2d > >> +#define IPMI_CMD_SET_SENSOR_TYPE 0x2e > >> +#define IPMI_CMD_GET_SENSOR_TYPE 0x2f > >> > >=20 > > ... here ? > >=20 > >> /* #define IPMI_NETFN_APP 0x06 In ipmi.h */ > >> #define IPMI_NETFN_APP_MAXCMD 0x36 > >> @@ -1576,6 +1578,49 @@ static void get_sensor_reading(IPMIBmcSim *ibs, > >> return; > >> } > >> > >> +static void set_sensor_type(IPMIBmcSim *ibs, > >> + uint8_t *cmd, unsigned int cmd_len, > >> + uint8_t *rsp, unsigned int *rsp_len, > >> + unsigned int max_rsp_len) > >> +{ > >> + IPMISensor *sens; > >> + > >> + > >> + IPMI_CHECK_CMD_LEN(5); > >> + if ((cmd[2] > MAX_SENSORS) || > >=20 > > Parenthesis not needed here since > has precedence over || > >=20 > >> + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > >=20 > > Indentation ? >=20 > OK. I will fix that. >=20 > >> + rsp[2] =3D IPMI_CC_REQ_ENTRY_NOT_PRESENT; > >> + goto out; > >> + } > >> + sens =3D ibs->sensors + cmd[2]; > >> + sens->sensor_type =3D cmd[3]; > >> + sens->evt_reading_type_code =3D cmd[4] & 0x7f; > >=20 > > So evt_reading_type_code is 7bit ? Maybe worth to > > introduce a IPMI_SENSOR_TYPE_MASK define. >=20 > Yes. there are a few of these in the code. >=20 > >> + > >> + out: > >> + return; > >> +} > >> + > >> +static void get_sensor_type(IPMIBmcSim *ibs, > >> + uint8_t *cmd, unsigned int cmd_len, > >> + uint8_t *rsp, unsigned int *rsp_len, > >> + unsigned int max_rsp_len) > >> +{ > >> + IPMISensor *sens; > >> + > >> + > >> + IPMI_CHECK_CMD_LEN(3); > >> + if ((cmd[2] > MAX_SENSORS) || > >> + !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) { > >=20 > > Parenthesis and indentation ? >=20 > yep. copy & paste :) >=20 > Thanks, >=20 > C. >=20 > >> + rsp[2] =3D IPMI_CC_REQ_ENTRY_NOT_PRESENT; > >> + goto out; > >> + } > >> + sens =3D ibs->sensors + cmd[2]; > >> + IPMI_ADD_RSP_DATA(sens->sensor_type); > >> + IPMI_ADD_RSP_DATA(sens->evt_reading_type_code); > >> + out: > >> + return; > >> +} > >> + > >> static const IPMICmdHandler chassis_cmds[IPMI_NETFN_CHASSIS_MAXCMD] = =3D { > >> [IPMI_CMD_GET_CHASSIS_CAPABILITIES] =3D chassis_capabilities, > >> [IPMI_CMD_GET_CHASSIS_STATUS] =3D chassis_status, > >> @@ -1592,7 +1637,9 @@ sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD= ] =3D { > >> [IPMI_CMD_GET_SENSOR_EVT_ENABLE] =3D get_sensor_evt_enable, > >> [IPMI_CMD_REARM_SENSOR_EVTS] =3D rearm_sensor_evts, > >> [IPMI_CMD_GET_SENSOR_EVT_STATUS] =3D get_sensor_evt_status, > >> - [IPMI_CMD_GET_SENSOR_READING] =3D get_sensor_reading > >> + [IPMI_CMD_GET_SENSOR_READING] =3D get_sensor_reading, > >> + [IPMI_CMD_SET_SENSOR_TYPE] =3D set_sensor_type, > >> + [IPMI_CMD_GET_SENSOR_TYPE] =3D get_sensor_type, > >> }; > >> static const IPMINetfn sensor_event_netfn =3D { > >> .cmd_nums =3D IPMI_NETFN_SENSOR_EVENT_MAXCMD, > >=20 >=20 >=20