[openib-general] [RFC] IB/ipoib: Asynchronous events delivered without port parameter.

[Moni Levy]

On 2/27/07, Roland Dreier <rdreier at cisco.com> wrote:
>  >    I did a short code review of the ipoib code concentrating on
>  > partitioning support and I mentioned that the asynchronous events
>  > handler in the ipoib code does not take the port number reported in
>  > the event record into consideration. The effect of that is that all of
>  > the ib# devices related to that specific HCA are flushed when it seems
>  > to me that only the relevant port one should be. Is that done on
>  > purpose, or am I missing something ?
>
> I don't think there's any particular reason the code is that way
> except for the oversight never being corrected.  But it looks trivial
> to fix, like the patch below.  Does that look right to you?
>
>  > p.s. I'm working on a patch that should solve another issue caused by
>  > PKEY reordering & ipoib behavior and the above issue further
>  > complicates things for me.
>
> Why not fix the issue first then?
>
> commit a27cbe878203076247c1b5287f5ab59ed143b560
> Author: Roland Dreier <rolandd at cisco.com>
> Date:   Tue Feb 27 07:37:49 2007 -0800
>
>     IPoIB: Only handle async events for one port
>
>     An asynchronous event carries the port number that the event occurred
>     on, so there's no reason for an IPoIB interface to process an event
>     associated with a different local HCA port.
>
>     Signed-off-by: Roland Dreier <rolandd at cisco.com>
>
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
> index 3cb551b..7f3ec20 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_verbs.c
> @@ -259,12 +259,13 @@ void ipoib_event(struct ib_event_handler *handler,
>         struct ipoib_dev_priv *priv =
>                 container_of(handler, struct ipoib_dev_priv, event_handler);
>
> -       if (record->event == IB_EVENT_PORT_ERR    ||
> -           record->event == IB_EVENT_PKEY_CHANGE ||
> -           record->event == IB_EVENT_PORT_ACTIVE ||
> -           record->event == IB_EVENT_LID_CHANGE  ||
> -           record->event == IB_EVENT_SM_CHANGE   ||
> -           record->event == IB_EVENT_CLIENT_REREGISTER) {
> +       if ((record->event == IB_EVENT_PORT_ERR    ||
> +            record->event == IB_EVENT_PKEY_CHANGE ||
> +            record->event == IB_EVENT_PORT_ACTIVE ||
> +            record->event == IB_EVENT_LID_CHANGE  ||
> +            record->event == IB_EVENT_SM_CHANGE   ||
> +            record->event == IB_EVENT_CLIENT_REREGISTER) &&
> +           record->element.port_num == priv->port) {
>                 ipoib_dbg(priv, "Port state change event\n");
>                 queue_work(ipoib_workqueue, &priv->flush_task);
>         }
>

That's exactly what I intended to post.

--Moni