Microsemi / switchtec-kernel

A kernel module for the Microsemi PCIe switch
GNU General Public License v2.0
45 stars 31 forks source link

Fix concurrent/overlapped MWs setup failure issue #49

Closed wesleywesley closed 4 years ago

wesleywesley commented 5 years ago

When setup MW or reqest ID mapping table, the state machine named "NT Partition State/Control Transition" is be followed. But not in case of multiple MW/request ID mapping table setup concurrently situation, the latter of the two will fail for the reason of its operation based on a false NT Partition State input, which actually belong to the former setup.

This issue is easy to be duplicated on driver introduced "force link down" new feature: One aspect, the host which received the force link message from peer host, reconfigure the LUT windows for peer is trigged, but before its completion, setup MW required by NTB client ntb_transport is immediately followed. The latter of the two has to lock the "configure channel" before the real setup operation, but the locked state it read out from "NT STAT" register is the state transition result by the former lock command.

Fix this corner case by add a specific mutex.

Signed-off-by: Wesley Sheng wesley.sheng@microchip.com

ghost commented 5 years ago

I think it's not good. If host is reconfiguring the shared memory window, it means the transport link is not ready, and the NTB client should not set up the MW.

Joey

lsgunth commented 5 years ago

Is this a duplicate of #48? I feel like there's a better solution than adding a lock there (and we'd probably have to add that lock in a few other places too)... See my comment in #48.

wesleywesley commented 5 years ago

Yes, this patch fix the same issue as #48

When review the #48 , I have to reject it for 2 major points:

  1. It based on false assumptions Namely the two MW setup operations (A: force link down trigged reinit LUT for peer; B: NTB client ntb_transport required DW setup) be scheduled in fixed sequence: A is always ahead of B. But that's not the real case: A is be scheduled into system work queue, take an example: when the queue is heavy load, B may run ahead of A, in this case (not a corner case) #48 will be non effective.

  2. Mess up responsibility between NTB client and NTB hardware driver The link_sta(self_shared->link_sta) variable should be manipulated by client through NTB dev ops, while NTB hardware driver responsible for correct setting of link_is_up, now based on link_sta of both self and peer. Same consideration for link messages, that's MSG_LINK_UP/DOWN by client instead of by hardware driver itself. This division of responsibility make things clear and easy to understood.

lsgunth commented 5 years ago

Yes, the other patch didn't seem like the correct solution either. I would rather see us serialize the link management stuff behind a single work queue item so that the link can't go up while/before we are setting up the shared mw.