On Mon, Sep 20, 2021 at 02:19:18PM +0200, Cornelia Huck wrote:
On Thu, Sep 09 2021, Jason Gunthorpe jgg@nvidia.com wrote:
The subchannel should be left in a quiescent state unless the VFIO device FD is opened. When the FD is opened bring the chanel to active and allow the VFIO device to operate. When the device FD is closed then quiesce the channel.
To make this work the FSM needs to handle the transitions to/from open and closed so everything is sequenced. Rename state NOT_OPER to BROKEN and use it wheneven the driver has malfunctioned. STANDBY becomes CLOSED. The normal case FSM looks like: CLOSED -> IDLE -> PROCESS/PENDING* -> IDLE -> CLOSED
With a possible branch off to BROKEN from any state. Once the device is in BROKEN it cannot be recovered other than be reloading the driver.
Hm, not sure whether it is a good idea to conflate "something went wrong" and "device is not operational".
Yes, but that is exactly what this FSM is currently doing, NO_OPER is a dumping ground for all kinds of wonky stuff, and what exactly it is supposed to mean or do is unclear.
while the former case could mean all kind of things, but the subchannel will likely stay around. I think NOT_OPER was always meant to be a transitional state.
Then these sorts of failures should recover the device and FSM back to an appropriate operational state and keep going - but I'm not going to attempt to guess when each of the conditions are recoverable or not.
Delete the triply redundant calls to vfio_ccw_sch_quiesce(). vfio_ccw_mdev_close_device() always leaves the subchannel quiescent. vfio_ccw_mdev_remove() cannot return until vfio_ccw_mdev_close_device() completes and vfio_ccw_sch_remove() cannot return until vfio_ccw_mdev_remove() completes. Have the FSM code take care of calling cp_free() when appropriate.
I remember some serialization issues wrt cp_free() etc. coming up every now and than; that might need extra care (I'm taking a look.)
I'm not too surprised, things like NOT_OPER just exiting the usual FSM logic mean stuff couldn't be properly sequenced.
The new logic puts a cp_free in each of arcs entering the terminal states broken/closed and all the flows that would get us to vfio_ccw_mdev_remove() will enter one of those states.
It is quite possible this patch needs someone who actually understand this HW to polish it up - the point was to show how ccw should be cleanly structured. I'd like to go ahead with the other patches and leave this for the ccw maintainers if it is needs significant work. The other patches are what are blocking the core code cleanups.
Jason