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;
> > +}
> > +
