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

[Gavin Shan]

On Thu, Sep 29, 2016 at 11:44:05AM +0930, Joel Stanley wrote:
>On Wed, Sep 28, 2016 at 4:48 PM, Gavin Shan <gwshan at linux.vnet.ibm.com> wrote:
>> On Wed, Sep 28, 2016 at 03:20:32PM +0930, Joel Stanley wrote:
>>>> +#define NCSI_CHANNEL_MONITOR_START  0
>>>> +#define NCSI_CHANNEL_MONITOR_RETRY  1
>>>> +#define NCSI_CHANNEL_MONITOR_WAIT   2
>>>> +#define NCSI_CHANNEL_MONITOR_WAIT_MAX       5
>>>> +    } monitor;
>>>>      struct list_head            node;
>>>>      struct list_head            link;
>>>>  };
>>>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>>>> index 3019432..5813ebe 100644
>>>> --- a/net/ncsi/ncsi-manage.c
>>>> +++ b/net/ncsi/ncsi-manage.c
>>>> @@ -165,13 +165,13 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>>      struct ncsi_dev_priv *ndp = np->ndp;
>>>>      struct ncsi_cmd_arg nca;
>>>>      bool enabled;
>>>> -    unsigned int timeout;
>>>> +    unsigned int monitor_state;
>>>>      unsigned long flags;
>>>>      int state, ret;
>>>>
>>>>      spin_lock_irqsave(&nc->lock, flags);
>>>> -    timeout = nc->timeout;
>>>> -    enabled = nc->enabled;
>>>> +    monitor_state = nc->monitor.state;
>>>> +    enabled = nc->monitor.enabled;
>>>>      spin_unlock_irqrestore(&nc->lock, flags);
>>>>
>>>>      if (!enabled || !list_empty(&nc->link))
>>>> @@ -181,7 +181,9 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>>      if (state != NCSI_CHANNEL_INACTIVE && state !=
>>>> NCSI_CHANNEL_ACTIVE)
>>>>              return;
>>>>
>>>> -    if (!(timeout % 2)) {
>>>> +    switch (monitor_state) {
>>>> +    case NCSI_CHANNEL_MONITOR_START:
>>>> +    case NCSI_CHANNEL_MONITOR_RETRY:
>>>>              nca.ndp = ndp;
>>>>              nca.package = np->id;
>>>>              nca.channel = nc->id;
>>>> @@ -193,15 +195,19 @@ static void ncsi_channel_monitor(unsigned long
>>>> data)
>>>>                                 ret);
>>>>                      return;
>>>>              }
>>>> -    }
>>>>
>>>> -    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.

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 */

   monitor.state++;
   rescheule_timeout();

Thanks,
Gavin

>Cheers,
>
>Joel
>