Re: [PATCH] ipmi: add SET_SENSOR_READING command

[Cédric Le Goater]

On 22/11/2019 15:28, Corey Minyard wrote:
> On Mon, Nov 18, 2019 at 10:24:29AM +0100, Cédric Le Goater wrote:
>> SET_SENSOR_READING is a complex IPMI command (see IPMI spec 35.17)
>> which enables the host software to set the reading value and the event
>> status of sensors supporting it.
>>
>> Below is a proposal for all the operations (reading, assert, deassert,
>> event data) with the following limitations :
>>
>>  - No event are generated for threshold-based sensors.
>>  - The case in which the BMC needs to generate its own events is not
>>    supported.
> 
> Ok, I've included this in my tree.  I made one small change mentioned
> below.  Beyond that, I think you could make this function shorter, but I
> think that would actually make it harder to understand.  Breaking it
> into multiple functions doesn't make sense to me, either.
> 
> If you are including this in the ppc tree:
> 
> Acked-by: Corey Minyard <address@hidden>
> 
> with the change below and I can remove it from mine.

I don't think there is a strong need to have it in the PPC tree. It's 
a stand alone function adding an extra IPMI command.


>> Signed-off-by: Cédric Le Goater <address@hidden>
>> Reviewed-by: Corey Minyard <address@hidden>
>> ---
>> +
>> +    switch (do_gen_event) {
>> +    case SENSOR_GEN_EVENT_DATA: {
>> +        unsigned int bit = evd1 & 0xf;
>> +        uint16_t mask = (1 << bit);
>> +
>> +        if (sens->assert_states & mask & sens->assert_enable) {
>> +            gen_event(ibs, cmd[2], 0, evd1, evd2, evd3);
>> +        }
>> +
>> +        if (sens->deassert_states & mask & sens->deassert_enable) {
>> +            gen_event(ibs, cmd[2], 1, evd1, evd2, evd3);
>> +        }
>> +    }
>> +        break;
> 
> I moved this break statement above the brace before it to keep the
> indention consistent.  It just screwed with my brain too much :).
>
> I looked and there is nothing in the coding style about this, and I
> found this done in three different ways:
> 
>   case x: {  /* in vl.c */
>       ....
>       break;
>   }
>   case y: /* in thunk.c */
>       {
>            ....
>       }
>       break;
>   case z: /* In vl.c */
>   {
>       ....
>       break;
>   }
> 
> Oddly enough, I didn't find anything about this in the Linux coding
> style document, either (I was curious).  One could argue, I suppose,
> that according to the "Block structure" section in the qemu style and
> the similar section in the Linux style that the first is correct,
> but then case statements violate the "Every indented statement is
> braced" statement in the qemu style.  This has always bugged me in
> C, sorry for the diatribe on this.

Thanks,

C. 

> 
> -corey
> 
>> +    case SENSOR_GEN_EVENT_BMC: