Re: [Qemu-devel] [PATCH 2/8] ipmi: add get and set SENSOR_TYPE commands

[Greg Kurz]

On Wed, 6 Jan 2016 19:16:27 +0100
Cédric Le Goater <address@hidden> wrote:

> On 01/06/2016 10:55 AM, Greg Kurz wrote:
> > On Tue,  5 Jan 2016 18:29:56 +0100
> > Cédric Le Goater <address@hidden> wrote:
> > 
> >> Signed-off-by: Cédric Le Goater <address@hidden>
> >> ---
> > 
> > Acked-by: Greg Kurz <address@hidden>
> > 
> > Just some minor comments on the form below.
> > 
> > 
> >>  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
> >>
> > 
> > Maybe IPMI_NETFN_SENSOR_EVENT_MAXCMD should be defined...
> 

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 :
> 
>       static const IPMICmdHandler *_cmds[*_MAXCMD] 
> 
> You'll get the idea.
> 

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
> >>
> > 
> > ... here ?
> > 
> >>  /* #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) ||
> > 
> > Parenthesis not needed here since > has precedence over ||
> > 
> >> +            !IPMI_SENSOR_GET_PRESENT(ibs->sensors + cmd[2])) {
> > 
> > Indentation ?
> 
> OK. I will fix that.
> 
> >> +        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> >> +        goto out;
> >> +    }
> >> +    sens = ibs->sensors + cmd[2];
> >> +    sens->sensor_type = cmd[3];
> >> +    sens->evt_reading_type_code = cmd[4] & 0x7f;
> > 
> > So evt_reading_type_code is 7bit ? Maybe worth to
> > introduce a IPMI_SENSOR_TYPE_MASK define.
> 
> Yes. there are a few of these in the code.
> 
> >> +
> >> + 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])) {
> > 
> > Parenthesis and indentation ?
> 
> yep. copy & paste :)
> 
> Thanks,
> 
> C.
> 
> >> +        rsp[2] = IPMI_CC_REQ_ENTRY_NOT_PRESENT;
> >> +        goto out;
> >> +    }
> >> +    sens = 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] = {
> >>      [IPMI_CMD_GET_CHASSIS_CAPABILITIES] = chassis_capabilities,
> >>      [IPMI_CMD_GET_CHASSIS_STATUS] = chassis_status,
> >> @@ -1592,7 +1637,9 @@ sensor_event_cmds[IPMI_NETFN_SENSOR_EVENT_MAXCMD] = {
> >>      [IPMI_CMD_GET_SENSOR_EVT_ENABLE] = get_sensor_evt_enable,
> >>      [IPMI_CMD_REARM_SENSOR_EVTS] = rearm_sensor_evts,
> >>      [IPMI_CMD_GET_SENSOR_EVT_STATUS] = get_sensor_evt_status,
> >> -    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading
> >> +    [IPMI_CMD_GET_SENSOR_READING] = get_sensor_reading,
> >> +    [IPMI_CMD_SET_SENSOR_TYPE] = set_sensor_type,
> >> +    [IPMI_CMD_GET_SENSOR_TYPE] = get_sensor_type,
> >>  };
> >>  static const IPMINetfn sensor_event_netfn = {
> >>      .cmd_nums = IPMI_NETFN_SENSOR_EVENT_MAXCMD,
> > 
> 
>