[Intel-gfx] [PATCH v1 1/7] vfio/ccw: create a parent struct
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
---
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) {
+ /*
+* 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);
@@ -121,7 +125,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 +220,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,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);
+
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);
Re: [Intel-gfx] [PATCH v1 1/7] vfio/ccw: create a parent struct
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
> ---
> 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)
> +
Re: [Intel-gfx] [PATCH v1 1/7] vfio/ccw: create a parent struct
On Fri, 2022-10-28 at 12:51 -0400, Matthew Rosato wrote:
> 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
> > ---
> > 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?
Hrm, the caller jumps from private -> subchannel, so it would be weird
if we couldn't then go back the other way. Probably impossible, I'll
unwind these whitespace changes and put the WARN_ON on top. Thanks for
the tip.
>
> > + /*
> > + * 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(&comple
> > tion, 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 ..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
> > +
> > +/**
> > + * 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_typemdev_type;
> > + struct mdev_type*mdev_types[1];
> > +};
>
> Structure itself seems fine, but any reason we need a new file for
> it?
>
Not really. I could leave it in _private.h, but that file is just a
dumping ground for everything so I thought this would be a good
opportunity to start to cleaning that up. But it wouldn't bother me to
leave that whole process to another day too.
Re: [Intel-gfx] [PATCH v1 1/7] vfio/ccw: create a parent struct
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
> ---
> 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 ..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
> +
> +/**
> + * 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_typemdev_type;
> + struct mdev_type*mdev_types[1];
> +};
Structure itself seems fine, but any reason we need a new file for it?
