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

[Michael S. Tsirkin]

> Quoting Yosef Etigin <yosefe at voltaire.com>:
> Subject: Re: [PATCHv2 1/2] ipoib: handle pkey change events
> 
> Michael S. Tsirkin wrote:
> >>Quoting Yosef Etigin <yosefe at voltaire.com>:
> >>Subject: Re: [PATCHv2 1/2] ipoib: handle pkey change events
> >>
> >>Michael S. Tsirkin wrote:
> >>
> >>>>Quoting Yosef Etigin <yosefe at voltaire.com>:
> >>>>Subject: Re: [PATCHv2 1/2] ipoib: handle pkey change events
> >>>>
> >>>>Michael S. Tsirkin wrote:
> >>>>
> >>>>
> >>>>>>Quoting Yosef Etigin <yosefe at voltaire.com>:
> >>>>>>Subject: [PATCHv2 1/2] ipoib: handle pkey change events
> >>>>>>
> >>>>>>This issue was found during partitioning & SM fail over testing.
> >>>>>>
> >>>>>>* Added flush flag to ipoib_ib_dev_stop(), ipoib_ib_dev_down() alike
> >>>>>>* Rename the polling thread work to 'pkey_poll_task' to avoid ambiguity
> >>>>>>* Upon PKEY_CHANGE event, schedule a work that restarts the QP
> >>>>>>* Restart child interfaces before parent
> >>>>>
> >>>>>
> >>>>>What's the reason for this change?
> >>>>>Is this a separate bugfix?
> >>>>>You might want to put this info in the log.
> >>>>>
> >>>>
> >>>>The reason is that if the child are restarted after the parent, and the parent is
> >>>>not up, then the flush function returns immediately due to the INITIALLIZED bit test.
> >>>>Now I think that we might use a goto statement instead.
> >>>
> >>>
> >>>So ... what the problem? I still don't see it.
> >>>
> >>
> >>If I get pkey change event, I want to restart all active ifaces on my port. If
> >>the parent is not marked with IPOIB_FLAG_INITIALIZED, the function returns
> >>before it has a chance to recursively restart child ifaces.
> > 
> > 
> > So now, if restart_qp is set, you are going to open it, and it's not
> > initialized?
> >
> > BTW, if the interface is not initialized, is not QP in reset already?
> > So can't we just move the code that assign the pkey to the open call?
> > 
> No - I'm going to open its *child* interface. The problem is that parents "mask out"
> their children.

Aha, I see this now. You might want to explain this in the comment.
Something like:

/* Flush any child interfaces too -
 * they might be up even if the parent is down */

> > Another idea - won't it be cleaner to have a function ipoib_restart_qp
> > (functionally similiar to ib_dev_flush, but also changing the QP)
> > than adding a flag to ib_dev_flush?
> > 
> It might be, but we wanted to avoid code duplication (the only difference is 2-3 lines)

OK, it's a valid approach too.

-- 
MST