On Mon, Sep 20, 2021 at 02:19:18PM +0200, Cornelia Huck wrote: > On Thu, Sep 09 2021, Jason Gunthorpe <jgg@xxxxxxxxxx> 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
