[PATCH v4.7 6/7] net/ncsi: Rework the channel monitoring

[Joel Stanley]

On Thu, Sep 29, 2016 at 12:12 PM, Gavin Shan <gwshan at linux.vnet.ibm.com> wrote:
>>>>>
>>>>> -    if (timeout + 1 >= 3) {
>>>>> +            break;
>>>>> +    case NCSI_CHANNEL_MONITOR_WAIT ...
>>>>> NCSI_CHANNEL_MONITOR_WAIT_MAX:
>>>>
>>>>It's not clear why you have the MAX_WAIT state if it's doing the same
>>>>thing as WAIT.
>>>>
>>>
>>> @nc->monitor.state tracks two types of information: (A) the current state
>>> of the channel monitor; (B) the time since last GLS reponse was received
>>> successfully. The general idea behind is: we have two GLS commands sent
>>> and at least one response should be received in 5 seconds, which is
>>> represented by NCSI_CHANNEL_MONITOR_WAIT_MAX.
>>
>>I follow that. But the code path for MAX_WAIT appears to be there same
>>as WAIT. There's nowhere that we do if (nc->monitor.state ==
>>NCSI_CHANNEL_MONITOR_WAIT_MAX) explode();
>>
>>Is that intentional? If so, why do we need the two separate states?
>>
>
> Yes, there are 4 waiting states (2/3/4/5) as below.
>
> #define NCSI_CHANNEL_MONITOR_WAIT      2
> #define NCSI_CHANNEL_MONITOR_WAIT_MAX  5
>
> I want to use "switch...case" in the code as it's a state machine.
> Also, I would like to have two macros to specify the range of
> waiting state, which makes the code clearer. Yes, it's intentional.

Okay. I guess my confusion was around never explicitly doing anything
in WAIT state. We say in retry state until we transition to MAX.

>
> Instead, we can use "if...else if...else". It's not good enough
> to have this in the state machine driven code, but it's a personal
> preference. In this case, we need less defined states:
>
>    #define NCSI_CHANNEL_MONITOR_RETRY   1
>    #define NCSI_CHANNEL_MONITOR_TIMEOUT 6
>
>    if (monitor.state <= NCSI_CHANNEL_MONITOR_RETRY)
>        /* send GLS packet */
>    else if (monitor.state < NCSI_CHANNEL_MONITOR_TIMEOUT)
>        /* break */
>    else
>         /* failover */

I think this is clearer. However, now that I understand what your
patch is doing, either approach is ok.

Cheers,

Joel

>
>    monitor.state++;
>    rescheule_timeout();
>
> Thanks,
> Gavin
>
>>Cheers,
>>
>>Joel
>>
>