Re: [PATCH v2 1/7] vfio/ccw: create a parent struct

[Eric Farman <farman@linux.ibm.com>]

On Wed, 2022-11-02 at 16:01 +0100, Eric Farman wrote:
> Move the stuff associated with the mdev parent (and thus the
> subchannel struct) into its own struct, and leave the rest in
> the existing private structure.
> 
> The subchannel will point to the parent, and the parent will point
> to the private, for the areas where one or both are needed. Further
> separation of these structs will follow.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 96 ++++++++++++++++++++++++---
> --
>  drivers/s390/cio/vfio_ccw_ops.c     |  8 ++-
>  drivers/s390/cio/vfio_ccw_private.h | 20 ++++--
>  3 files changed, 100 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> b/drivers/s390/cio/vfio_ccw_drv.c
> index 7f5402fe857a..06022fb37b9d 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -36,10 +36,19 @@ debug_info_t *vfio_ccw_debug_trace_id;
>   */
>  int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         DECLARE_COMPLETION_ONSTACK(completion);
>         int iretry, ret = 0;
>  
> +       /*
> +        * Probably an impossible situation, after being called
> through
> +        * FSM callbacks. But in the event it did, register a warning
> +        * and return as if things were fine.
> +        */
> +       if (WARN_ON(!private))
> +               return 0;
> +
>         iretry = 255;
>         do {
>  
> @@ -121,7 +130,22 @@ static void vfio_ccw_crw_todo(struct work_struct
> *work)
>   */
>  static void vfio_ccw_sch_irq(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
> +
> +       /*
> +        * The subchannel should still be disabled at this point,
> +        * so an interrupt would be quite surprising. As with an
> +        * interrupt while the FSM is closed, let's attempt to
> +        * disable the subchannel again.
> +        */
> +       if (!private) {
> +               VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: unexpected
> interrupt\n",
> +                       sch->schid.cssid, sch->schid.ssid, sch-
> >schid.sch_no);
> +
> +               cio_disable_subchannel(sch);
> +               return;
> +       }
>  
>         inc_irq_stat(IRQIO_CIO);
>         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
> @@ -201,10 +225,19 @@ static void vfio_ccw_free_private(struct
> vfio_ccw_private *private)
>         mutex_destroy(&private->io_mutex);
>         kfree(private);
>  }
> +
> +static void vfio_ccw_free_parent(struct device *dev)
> +{
> +       struct vfio_ccw_parent *parent = container_of(dev, struct
> vfio_ccw_parent, dev);
> +
> +       kfree(parent);
> +}
> +
>  static int vfio_ccw_sch_probe(struct subchannel *sch)
>  {
>         struct pmcw *pmcw = &sch->schib.pmcw;
>         struct vfio_ccw_private *private;
> +       struct vfio_ccw_parent *parent;
>         int ret = -ENOMEM;
>  
>         if (pmcw->qf) {
> @@ -213,41 +246,62 @@ static int vfio_ccw_sch_probe(struct subchannel
> *sch)
>                 return -ENODEV;
>         }
>  
> +       parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> +       if (IS_ERR(parent))
> +               return PTR_ERR(parent);
> +
> +       dev_set_name(&parent->dev, "parent");
> +       parent->dev.parent = &sch->dev;
> +       parent->dev.release = &vfio_ccw_free_parent;
> +       ret = device_register(&parent->dev);
> +       if (ret)
> +               goto out_free;
> +
>         private = vfio_ccw_alloc_private(sch);
> -       if (IS_ERR(private))
> +       if (IS_ERR(private)) {
> +               put_device(&parent->dev);

This should've been device_unregister. (I could rearrange the code a
bit to avoid the mix of returns/gotos around here, but since the whole
series is trying to separate these two structs that seems unnecessary.)

>                 return PTR_ERR(private);
> +       }
>  
> -       dev_set_drvdata(&sch->dev, private);
> +       dev_set_drvdata(&sch->dev, parent);
> +       dev_set_drvdata(&parent->dev, private);
>  
> -       private->mdev_type.sysfs_name = "io";
> -       private->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
> -       private->mdev_types[0] = &private->mdev_type;
> -       ret = mdev_register_parent(&private->parent, &sch->dev,
> +       parent->mdev_type.sysfs_name = "io";
> +       parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
> +       parent->mdev_types[0] = &parent->mdev_type;
> +       ret = mdev_register_parent(&parent->parent, &sch->dev,
>                                    &vfio_ccw_mdev_driver,
> -                                  private->mdev_types, 1);
> +                                  parent->mdev_types, 1);
>         if (ret)
> -               goto out_free;
> +               goto out_unreg;
>  
>         VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
>                            sch->schid.cssid, sch->schid.ssid,
>                            sch->schid.sch_no);
>         return 0;
>  
> +out_unreg:
> +       device_unregister(&parent->dev);
>  out_free:
> +       dev_set_drvdata(&parent->dev, NULL);
>         dev_set_drvdata(&sch->dev, NULL);
>         vfio_ccw_free_private(private);
> +       put_device(&parent->dev);

While this...

>         return ret;
>  }
>  
>  static void vfio_ccw_sch_remove(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>  
> -       mdev_unregister_parent(&private->parent);
> +       mdev_unregister_parent(&parent->parent);
>  
> +       device_unregister(&parent->dev);
>         dev_set_drvdata(&sch->dev, NULL);
>  
>         vfio_ccw_free_private(private);
> +       put_device(&parent->dev);

...and this shouldn't even be there. Sorry for the brain fog.

>  
>         VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
>                            sch->schid.cssid, sch->schid.ssid,
> @@ -256,7 +310,11 @@ static void vfio_ccw_sch_remove(struct
> subchannel *sch)
>  
>  static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
> +
> +       if (WARN_ON(!private))
> +               return;
>  
>         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> @@ -274,7 +332,8 @@ static void vfio_ccw_sch_shutdown(struct
> subchannel *sch)
>   */
>  static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         unsigned long flags;
>         int rc = -EAGAIN;
>  
> @@ -287,8 +346,10 @@ static int vfio_ccw_sch_event(struct subchannel
> *sch, int process)
>  
>         rc = 0;
>  
> -       if (cio_update_schib(sch))
> -               vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> +       if (cio_update_schib(sch)) {
> +               if (private)
> +                       vfio_ccw_fsm_event(private,
> VFIO_CCW_EVENT_NOT_OPER);
> +       }
>  
>  out_unlock:
>         spin_unlock_irqrestore(sch->lock, flags);
> @@ -326,7 +387,8 @@ static void vfio_ccw_queue_crw(struct
> vfio_ccw_private *private,
>  static int vfio_ccw_chp_event(struct subchannel *sch,
>                               struct chp_link *link, int event)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>         int retry = 255;
>  
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> b/drivers/s390/cio/vfio_ccw_ops.c
> index 6ae4d012d800..dc084883d872 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -55,7 +55,9 @@ static int vfio_ccw_mdev_init_dev(struct
> vfio_device *vdev)
>  
>  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(mdev-
> >dev.parent);
> +       struct subchannel *sch = to_subchannel(mdev->dev.parent);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         int ret;
>  
>         if (private->state == VFIO_CCW_STATE_NOT_OPER)
> @@ -100,7 +102,9 @@ static void vfio_ccw_mdev_release_dev(struct
> vfio_device *vdev)
>  
>  static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(mdev-
> >dev.parent);
> +       struct subchannel *sch = to_subchannel(mdev->dev.parent);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>  
>         VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
>                            private->sch->schid.cssid,
> diff --git a/drivers/s390/cio/vfio_ccw_private.h
> b/drivers/s390/cio/vfio_ccw_private.h
> index bd5fb81456af..1f598d58d969 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -67,6 +67,21 @@ struct vfio_ccw_crw {
>         struct crw              crw;
>  };
>  
> +/**
> + * struct vfio_ccw_parent
> + *
> + * @dev: embedded device struct
> + * @parent: parent data structures for mdevs created
> + * @mdev_type(s): identifying information for mdevs created
> + */
> +struct vfio_ccw_parent {
> +       struct device           dev;
> +
> +       struct mdev_parent      parent;
> +       struct mdev_type        mdev_type;
> +       struct mdev_type        *mdev_types[1];
> +};
> +
>  /**
>   * struct vfio_ccw_private
>   * @vdev: Embedded VFIO device
> @@ -89,7 +104,6 @@ struct vfio_ccw_crw {
>   * @io_work: work for deferral process of I/O handling
>   * @crw_work: work for deferral process of CRW handling
>   * @release_comp: synchronization helper for vfio device release
> - * @parent: parent data structures for mdevs created
>   */
>  struct vfio_ccw_private {
>         struct vfio_device vdev;
> @@ -116,10 +130,6 @@ struct vfio_ccw_private {
>         struct work_struct      crw_work;
>  
>         struct completion       release_comp;
> -
> -       struct mdev_parent      parent;
> -       struct mdev_type        mdev_type;
> -       struct mdev_type        *mdev_types[1];
>  } __aligned(8);
>  
>  int vfio_ccw_sch_quiesce(struct subchannel *sch);