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

[Michael S. Tsirkin]

> Quoting Roland Dreier <rdreier at cisco.com>:
> Subject: Re: [RFC] IB/ipoib: Asynchronous events delivered without port parameter.
> 
>  >    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);
>  	}

Looks good.

-- 
MST