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

[Yosef Etigin]

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.

> 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)