On 10/19/22 12:21 PM, 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
> @@ -20,6 +20,7 @@
> #include "chp.h"
> #include "ioasm.h"
> #include "css.h"
> +#include "vfio_ccw_parent.h"
> #include "vfio_ccw_private.h"
>
> struct workqueue_struct *vfio_ccw_work_q;
> @@ -36,7 +37,8 @@ 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;
>
> @@ -51,19 +53,21 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
> break;
> }
>
> - /*
> - * Flush all I/O and wait for
> - * cancel/halt/clear completion.
> - */
> - private->completion = &completion;
> - spin_unlock_irq(sch->lock);
> + if (private) {
Is it valid to ever reach this code with private == NULL? If no, then this should probably be a WARN_ON upfront?
> + /*
> + * Flush all I/O and wait for
> + * cancel/halt/clear completion.
> + */
> + private->completion = &completion;
> + spin_unlock_irq(sch->lock);
>
> - if (ret == -EBUSY)
> - wait_for_completion_timeout(&completion, 3*HZ);
> + if (ret == -EBUSY)
> + wait_for_completion_timeout(&completion, 3*HZ);
>
> - private->completion = NULL;
> - flush_workqueue(vfio_ccw_work_q);
> - spin_lock_irq(sch->lock);
> + private->completion = NULL;
> + flush_workqueue(vfio_ccw_work_q);
> + spin_lock_irq(sch->lock);
> + }
> ret = cio_disable_subchannel(sch);
> } while (ret == -EBUSY);
>
.. snip ..
> 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];
> +};
Structure itself seems fine, but any reason we need a new file for it?
