On Fri, 25 Jan 2019 16:00:38 -0500 Eric Farman <farman@xxxxxxxxxxxxx> wrote: > On 01/21/2019 06:03 AM, Cornelia Huck wrote: > > Add a region to the vfio-ccw device that can be used to submit > > asynchronous I/O instructions. ssch continues to be handled by the > > existing I/O region; the new region handles hsch and csch. > > > > Interrupt status continues to be reported through the same channels > > as for ssch. > > > > Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx> > > --- > > drivers/s390/cio/Makefile | 3 +- > > drivers/s390/cio/vfio_ccw_async.c | 91 ++++++++++++++++++++++ > > drivers/s390/cio/vfio_ccw_drv.c | 45 +++++++---- > > drivers/s390/cio/vfio_ccw_fsm.c | 114 +++++++++++++++++++++++++++- > > drivers/s390/cio/vfio_ccw_ops.c | 13 +++- > > drivers/s390/cio/vfio_ccw_private.h | 9 ++- > > include/uapi/linux/vfio.h | 2 + > > include/uapi/linux/vfio_ccw.h | 12 +++ > > 8 files changed, 269 insertions(+), 20 deletions(-) > > create mode 100644 drivers/s390/cio/vfio_ccw_async.c (...) > > @@ -149,7 +155,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch) > > cio_disable_subchannel(sch); > > out_free: > > dev_set_drvdata(&sch->dev, NULL); > > - kmem_cache_free(vfio_ccw_io_region, private->io_region); > > + if (private->cmd_region) > > + kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region); > > + if (private->io_region) > > + kmem_cache_free(vfio_ccw_io_region, private->io_region); > > Well, adding the check if private->xxx_region is non-NULL is fine. I'd > have made it a separate patch for io_region, but whatever. > > Since you're adding that check, you should add the same if statement in > vfio_ccw_sch_remove(). And you should certainly call > kmem_cache_free(private->cmd_region) there too. :) Ehm, yes :) > > > kfree(private); > > return ret; > > } (...) > > +static int fsm_do_halt(struct vfio_ccw_private *private) > > +{ > > + struct subchannel *sch; > > + unsigned long flags; > > + int ccode; > > + int ret; > > + > > + sch = private->sch; > > + > > + spin_lock_irqsave(sch->lock, flags); > > + > > + /* Issue "Halt Subchannel" */ > > + ccode = hsch(sch->schid); > > + > > + switch (ccode) { > > + case 0: > > + /* > > + * Initialize device status information > > + */ > > + sch->schib.scsw.cmd.actl |= SCSW_ACTL_HALT_PEND; > > + ret = 0; > > + private->state = VFIO_CCW_STATE_BUSY; > > + break; > > + case 1: /* Status pending */ > > + case 2: /* Busy */ > > + ret = -EBUSY; > > + break; > > + case 3: /* Device not operational */ > > + { > > + ret = -ENODEV; > > + break; > > + } > > Why does cc3 get braces, but no other case does? (Ditto for clear) > > (I guess the answer is "because fsm_io_helper does" but I didn't notice > that before either. :-) Yes, the power of copy/paste :) But it makes sense to avoid adding them. > > > + default: > > + ret = ccode; > > + } > > + spin_unlock_irqrestore(sch->lock, flags); > > + return ret; > > +} (...) > > +static void fsm_async_error(struct vfio_ccw_private *private, > > + enum vfio_ccw_event event) > > +{ > > + pr_err("vfio-ccw: FSM: halt/clear request from state:%d\n", > > + private->state); > > Had a little deja vu here. :) > > Can this message use private->cmd_region->command to tell us if it's a > halt, clear, or unknown? Instead of just saying "halt/clear" statically. Can do that; need to check if we need the mutex. > > > + private->cmd_region->ret_code = -EIO; > > +} > > +