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

[Joel Stanley]

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?

Cheers,

Joel