[ofa-general] Re: [PATCHv3 2/2] ipoib: handle pkey change events

[Michael S. Tsirkin]

> Quoting Or Gerlitz <ogerlitz at voltaire.com>:
> Subject: Re: [ofa-general] Re: [PATCHv3 2/2] ipoib: handle pkey change events
> 
> Michael S. Tsirkin wrote:
> >>@@ -260,7 +249,6 @@ void ipoib_event(struct ib_event_handler
> >> 		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   ||
> >>@@ -268,5 +256,9 @@ void ipoib_event(struct ib_event_handler
> >> 	    record->element.port_num == priv->port) {
> >> 		ipoib_dbg(priv, "Port state change event\n");
> >> 		queue_work(ipoib_workqueue, &priv->flush_task);
> >>+	} else if (record->event == IB_EVENT_PKEY_CHANGE &&
> >>+		   record->element.port_num == priv->port) {
> >>+		ipoib_dbg(priv, "pkey change event on port:%d\n", 
> >>priv->port);
> >>+		queue_work(ipoib_workqueue, &priv->pkey_event_task);
> >> 	}
> >> }
> >
> >BTW, should we maybe do:
> >if (record->element.port_num != priv->port)
> >	return;
> >
> >and then we won't have to do this test for each event type?
> 
> Just make sure that all the events covered by this check are port 
> affiliated, ie don't have a wider scope.
> 
> Or.


Well, we currently have:

void ipoib_event(struct ib_event_handler *handler,
                 struct ib_event *record)
{
        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) &&
            record->element.port_num == priv->port) {
                ipoib_dbg(priv, "Port state change event\n");
                queue_work(ipoib_workqueue, &priv->flush_task);
        }
}

So this would not change anything, just clean up code a little.


-- 
MST