[Skiboot] [PATCH] HDAT: Add IPMI sensor data under /bmc node

[Vasant Hegde]

On 06/26/2017 12:55 PM, Oliver wrote:
> On Mon, Jun 26, 2017 at 3:57 PM, Joel Stanley <joel at jms.id.au> wrote:
>> On Fri, Jun 23, 2017 at 9:09 PM, Vasant Hegde
>> <hegdevasant at linux.vnet.ibm.com> wrote:
>>> Add IPMI sensor data under /bmc node.

.../...

>>>
>>> +static void add_ipmi_sensors(struct dt_node *bmc_node)
>>> +{
>>> +       int i;
>
>>> +       const void *hdif_sensor;
>
> Shouldn't this be a struct HDIF_common_hdr *?

Yes.

.../...

>>
>>> +       if (!hdif_sensor) {
>>> +               prlog(PR_DEBUG, "SENSORS: Invalid data\n");
>
> Can this say "missing IPMI sensor Mappings tuple" or something else
> that's a bit more descriptive?

Sure. Will fix in v2.

.../...

>>
>>> +                       continue;
>>> +               }
>>> +
>>> +               /* We support only < MAX_IPMI_SENSORS sensors */
>>> +               if (ipmi_sensors->data[i].type > 0xfe)
>>
>> use MAX_IPMI_SENSORS.
>>
>> ie.
>>
>> if (!(ipmi_sensors->data[i].type < MAX_IPMI_SENSORS))
>>
>
> Given sensor type is a single byte field we should just check that
> it's not 0xff.  Maybe #define that as IPMI_SENSOR_INVALID or some
> such.

Better to use  MAX_IPMI_SENSORS so that any changes in count reflects everywhere.

.../...

>>
>>> +
>>> +/* Idata index 0 : Sensor mapping data */
>>> +#define IPMI_SENSORS_IDATA_SENSORS     0
>>> +
>>> +struct ipmi_sensors_data {
>>> +       __be32  slca_index;
>>> +       uint8_t type;
>>> +       uint8_t id;
>>> +       __be16  reserved;
>>> +};
>>
>> Perhaps do this so you don't get padding:
>>
>> struct ipmi_sensors_data {
>>        __be32  slca_index;
>>        uint8_t type;
>>        uint8_t id;
>>        uint8_t reserved[2];
>> };
>>
>> And add a packed annotation so that we don't get holes.
> Just using the packed annotation should solve both problems.

Yes. I missed to add it in v1. Will fix in v2.

-Vasant