Re: [PATCH v1 1/7] vfio/ccw: create a parent struct
From: Eric Farman
Date: Thu Oct 27 2022 - 16:33:25 EST
On Wed, 2022-10-19 at 18:21 +0200, 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@xxxxxxxxxxxxx>
> ---
> drivers/s390/cio/vfio_ccw_drv.c | 104 ++++++++++++++++++++------
> --
> drivers/s390/cio/vfio_ccw_ops.c | 9 ++-
> drivers/s390/cio/vfio_ccw_parent.h | 28 ++++++++
> drivers/s390/cio/vfio_ccw_private.h | 5 --
> 4 files changed, 112 insertions(+), 34 deletions(-)
> create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
>
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> b/drivers/s390/cio/vfio_ccw_drv.c
> index 7f5402fe857a..634760ca0dea 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
...snip...
> @@ -213,18 +241,28 @@ 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);
> +
> + parent->dev.release = &vfio_ccw_free_parent;
> + device_initialize(&parent->dev);
> +
Oops. This should either be device_register, or I needed a device_add
somewhere along the way. There's no need to separate them, so the
former is probably better, but I still need some additional logic to
link this back to the css driver.
> private = vfio_ccw_alloc_private(sch);
> - if (IS_ERR(private))
> + if (IS_ERR(private)) {
> + put_device(&parent->dev);
> 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;
>
> @@ -234,20 +272,24 @@ static int vfio_ccw_sch_probe(struct subchannel
> *sch)
> return 0;
>
> out_free:
> + dev_set_drvdata(&parent->dev, NULL);
> dev_set_drvdata(&sch->dev, NULL);
> vfio_ccw_free_private(private);
> + put_device(&parent->dev);
> 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);
>
> dev_set_drvdata(&sch->dev, NULL);
>
> vfio_ccw_free_private(private);
> + put_device(&parent->dev);
>
> VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
> sch->schid.cssid, sch->schid.ssid,
> @@ -256,7 +298,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 +320,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 +334,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 +375,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..b1cd89d900ab 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -14,6 +14,7 @@
> #include <linux/nospec.h>
> #include <linux/slab.h>
>
> +#include "vfio_ccw_parent.h"
> #include "vfio_ccw_private.h"
>
> static const struct vfio_device_ops vfio_ccw_dev_ops;
> @@ -55,7 +56,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 +103,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_parent.h
> b/drivers/s390/cio/vfio_ccw_parent.h
> new file mode 100644
> index 000000000000..834c00077802
> --- /dev/null
> +++ b/drivers/s390/cio/vfio_ccw_parent.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * MDEV Parent contents for vfio_ccw driver
> + *
> + * Copyright IBM Corp. 2022
> + */
> +
> +#ifndef _VFIO_CCW_PARENT_H_
> +#define _VFIO_CCW_PARENT_H_
> +
> +#include <linux/mdev.h>
> +
> +/**
> + * 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];
> +};
> +
> +#endif
> diff --git a/drivers/s390/cio/vfio_ccw_private.h
> b/drivers/s390/cio/vfio_ccw_private.h
> index bd5fb81456af..673e9a81a172 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -89,7 +89,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 +115,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);