[linux,dev-4.13] net/ncsi: Don't take any action on HNCDSC AEN

Message ID 20171220030251.19337-1-sam@mendozajonas.com
State Accepted, archived
Headers show
Series
  • [linux,dev-4.13] net/ncsi: Don't take any action on HNCDSC AEN
Related show

Commit Message

Samuel Mendoza-Jonas Dec. 20, 2017, 3:02 a.m.
The current HNCDSC handler takes the status flag from the AEN packet and
will update or change the current channel based on this flag and the
current channel status.

However the flag from the HNCDSC packet merely represents the host link
state. While the state of the host interface is potentially interesting
information it should not affect the state of the NCSI link. Indeed the
NCSI specification makes no mention of any recommended action related to
the host network controller driver state.

Update the HNCDSC handler to record the host network driver status but
take no other action.

Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
Acked-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
(cherry picked from commit 75e8e15635e08f2598ecd20f4f71f4d043dd6e68)
---
 net/ncsi/ncsi-aen.c | 35 +++--------------------------------
 1 file changed, 3 insertions(+), 32 deletions(-)

Comments

Joel Stanley Jan. 16, 2018, 8:26 p.m. | #1
On Tue, Dec 19, 2017 at 9:02 PM, Samuel Mendoza-Jonas
<sam@mendozajonas.com> wrote:
> The current HNCDSC handler takes the status flag from the AEN packet and
> will update or change the current channel based on this flag and the
> current channel status.
>
> However the flag from the HNCDSC packet merely represents the host link
> state. While the state of the host interface is potentially interesting
> information it should not affect the state of the NCSI link. Indeed the
> NCSI specification makes no mention of any recommended action related to
> the host network controller driver state.
>
> Update the HNCDSC handler to record the host network driver status but
> take no other action.
>
> Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> Acked-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (cherry picked from commit 75e8e15635e08f2598ecd20f4f71f4d043dd6e68)

Applied to dev-4.13.

I think we discussed this and decided it was not required for dev-4.10?

Cheers,

Joel

> ---
>  net/ncsi/ncsi-aen.c | 35 +++--------------------------------
>  1 file changed, 3 insertions(+), 32 deletions(-)
>
> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 67e708e98ccf..e7b05de1e6d1 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c
> @@ -143,43 +143,14 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
>         if (!nc)
>                 return -ENODEV;
>
> -       /* If the channel is active one, we need reconfigure it */
>         spin_lock_irqsave(&nc->lock, flags);
>         ncm = &nc->modes[NCSI_MODE_LINK];
>         hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
>         ncm->data[3] = ntohl(hncdsc->status);
> -       netdev_info(ndp->ndev.dev, "NCSI: HNCDSC AEN - channel %u state %s\n",
> -                   nc->id, ncm->data[3] & 0x3 ? "up" : "down");
> -       if (!list_empty(&nc->link) ||
> -           nc->state != NCSI_CHANNEL_ACTIVE) {
> -               spin_unlock_irqrestore(&nc->lock, flags);
> -               return 0;
> -       }
> -
> -       spin_unlock_irqrestore(&nc->lock, flags);
> -       if (!(ndp->flags & NCSI_DEV_HWA) && !(ncm->data[3] & 0x1))
> -               ndp->flags |= NCSI_DEV_RESHUFFLE;
> -
> -       /* If this channel is the active one and the link doesn't
> -        * work, we have to choose another channel to be active one.
> -        * The logic here is exactly similar to what we do when link
> -        * is down on the active channel.
> -        *
> -        * On the other hand, we need configure it when host driver
> -        * state on the active channel becomes ready.
> -        */
> -       ncsi_stop_channel_monitor(nc);
> -
> -       spin_lock_irqsave(&nc->lock, flags);
> -       nc->state = (ncm->data[3] & 0x1) ? NCSI_CHANNEL_INACTIVE :
> -                                          NCSI_CHANNEL_ACTIVE;
>         spin_unlock_irqrestore(&nc->lock, flags);
> -
> -       spin_lock_irqsave(&ndp->lock, flags);
> -       list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> -       spin_unlock_irqrestore(&ndp->lock, flags);
> -
> -       ncsi_process_next_channel(ndp);
> +       netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> +                     "NCSI: host driver %srunning on channel %u\n",
> +                     ncm->data[3] & 0x1 ? "" : "not ", nc->id);
>
>         return 0;
>  }
> --
> 2.15.1
>
Samuel Mendoza-Jonas Jan. 16, 2018, 11:41 p.m. | #2
On Tue, 2018-01-16 at 14:26 -0600, Joel Stanley wrote:
> On Tue, Dec 19, 2017 at 9:02 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > The current HNCDSC handler takes the status flag from the AEN packet and
> > will update or change the current channel based on this flag and the
> > current channel status.
> > 
> > However the flag from the HNCDSC packet merely represents the host link
> > state. While the state of the host interface is potentially interesting
> > information it should not affect the state of the NCSI link. Indeed the
> > NCSI specification makes no mention of any recommended action related to
> > the host network controller driver state.
> > 
> > Update the HNCDSC handler to record the host network driver status but
> > take no other action.
> > 
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> > Acked-by: Jeremy Kerr <jk@ozlabs.org>
> > Signed-off-by: David S. Miller <davem@davemloft.net>
> > (cherry picked from commit 75e8e15635e08f2598ecd20f4f71f4d043dd6e68)
> 
> Applied to dev-4.13.
> 
> I think we discussed this and decided it was not required for dev-4.10?

I think so too, but 2017 was a very long time ago :)

> 
> Cheers,
> 
> Joel
> 
> > ---
> >  net/ncsi/ncsi-aen.c | 35 +++--------------------------------
> >  1 file changed, 3 insertions(+), 32 deletions(-)
> > 
> > diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> > index 67e708e98ccf..e7b05de1e6d1 100644
> > --- a/net/ncsi/ncsi-aen.c
> > +++ b/net/ncsi/ncsi-aen.c
> > @@ -143,43 +143,14 @@ static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
> >         if (!nc)
> >                 return -ENODEV;
> > 
> > -       /* If the channel is active one, we need reconfigure it */
> >         spin_lock_irqsave(&nc->lock, flags);
> >         ncm = &nc->modes[NCSI_MODE_LINK];
> >         hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
> >         ncm->data[3] = ntohl(hncdsc->status);
> > -       netdev_info(ndp->ndev.dev, "NCSI: HNCDSC AEN - channel %u state %s\n",
> > -                   nc->id, ncm->data[3] & 0x3 ? "up" : "down");
> > -       if (!list_empty(&nc->link) ||
> > -           nc->state != NCSI_CHANNEL_ACTIVE) {
> > -               spin_unlock_irqrestore(&nc->lock, flags);
> > -               return 0;
> > -       }
> > -
> > -       spin_unlock_irqrestore(&nc->lock, flags);
> > -       if (!(ndp->flags & NCSI_DEV_HWA) && !(ncm->data[3] & 0x1))
> > -               ndp->flags |= NCSI_DEV_RESHUFFLE;
> > -
> > -       /* If this channel is the active one and the link doesn't
> > -        * work, we have to choose another channel to be active one.
> > -        * The logic here is exactly similar to what we do when link
> > -        * is down on the active channel.
> > -        *
> > -        * On the other hand, we need configure it when host driver
> > -        * state on the active channel becomes ready.
> > -        */
> > -       ncsi_stop_channel_monitor(nc);
> > -
> > -       spin_lock_irqsave(&nc->lock, flags);
> > -       nc->state = (ncm->data[3] & 0x1) ? NCSI_CHANNEL_INACTIVE :
> > -                                          NCSI_CHANNEL_ACTIVE;
> >         spin_unlock_irqrestore(&nc->lock, flags);
> > -
> > -       spin_lock_irqsave(&ndp->lock, flags);
> > -       list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> > -       spin_unlock_irqrestore(&ndp->lock, flags);
> > -
> > -       ncsi_process_next_channel(ndp);
> > +       netdev_printk(KERN_DEBUG, ndp->ndev.dev,
> > +                     "NCSI: host driver %srunning on channel %u\n",
> > +                     ncm->data[3] & 0x1 ? "" : "not ", nc->id);
> > 
> >         return 0;
> >  }
> > --
> > 2.15.1
> >

Patch

diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index 67e708e98ccf..e7b05de1e6d1 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -143,43 +143,14 @@  static int ncsi_aen_handler_hncdsc(struct ncsi_dev_priv *ndp,
 	if (!nc)
 		return -ENODEV;
 
-	/* If the channel is active one, we need reconfigure it */
 	spin_lock_irqsave(&nc->lock, flags);
 	ncm = &nc->modes[NCSI_MODE_LINK];
 	hncdsc = (struct ncsi_aen_hncdsc_pkt *)h;
 	ncm->data[3] = ntohl(hncdsc->status);
-	netdev_info(ndp->ndev.dev, "NCSI: HNCDSC AEN - channel %u state %s\n",
-		    nc->id, ncm->data[3] & 0x3 ? "up" : "down");
-	if (!list_empty(&nc->link) ||
-	    nc->state != NCSI_CHANNEL_ACTIVE) {
-		spin_unlock_irqrestore(&nc->lock, flags);
-		return 0;
-	}
-
-	spin_unlock_irqrestore(&nc->lock, flags);
-	if (!(ndp->flags & NCSI_DEV_HWA) && !(ncm->data[3] & 0x1))
-		ndp->flags |= NCSI_DEV_RESHUFFLE;
-
-	/* If this channel is the active one and the link doesn't
-	 * work, we have to choose another channel to be active one.
-	 * The logic here is exactly similar to what we do when link
-	 * is down on the active channel.
-	 *
-	 * On the other hand, we need configure it when host driver
-	 * state on the active channel becomes ready.
-	 */
-	ncsi_stop_channel_monitor(nc);
-
-	spin_lock_irqsave(&nc->lock, flags);
-	nc->state = (ncm->data[3] & 0x1) ? NCSI_CHANNEL_INACTIVE :
-					   NCSI_CHANNEL_ACTIVE;
 	spin_unlock_irqrestore(&nc->lock, flags);
-
-	spin_lock_irqsave(&ndp->lock, flags);
-	list_add_tail_rcu(&nc->link, &ndp->channel_queue);
-	spin_unlock_irqrestore(&ndp->lock, flags);
-
-	ncsi_process_next_channel(ndp);
+	netdev_printk(KERN_DEBUG, ndp->ndev.dev,
+		      "NCSI: host driver %srunning on channel %u\n",
+		      ncm->data[3] & 0x1 ? "" : "not ", nc->id);
 
 	return 0;
 }