Hi all,
There have been discussions and attempts [1][2] to rework the vfio-ccw device
lifecycle to better align with the needs/expectations of vfio and mdev.
While those languished, commit cb9ff3f3b84c ("vfio: Add helpers for unifying
vfio_device life cycle") implemented a couple of tricks that help vfio and the
drivers that interact with it, while keeping vfio-ccw as-is. A handful of
commits titled "vfio/xxx: Use the new device life cycle helpers" implemented
those tricks for each of the drivers.
This series attempts to address the oddities/shortcomings of vfio-ccw, such
that vfio-ccw can use the same helpers as everyone else, and the tricks that
were implemented by the other drivers can be removed. It is built on 6.1-rc1,
and thus includes the various changes [3][4] that have occurred in and around
these parts.
Patches 1-5 rework the behavior of the vfio-ccw driver's private struct.
In summary, the mdev pieces are split out of vfio_ccw_private and into a
new vfio_ccw_parent struct that will continue to follow today's lifecycle.
The remainder (bulk) of the private struct moves to follow the mdev
probe/remove pair. There's opportunity for further separation of the
things in the private struct, which would simplify some of the vfio-ccw
code, but it got too hairy as I started that. Once vfio-ccw is no longer
considered unique, those cleanups can happen at our leisure.
Patch 6 removes the trickery where vfio-ccw uses vfio_init_device instead of
vfio_alloc_device, and thus removes vfio_init_device from the outside world.
Patch 7 removes vfio_free_device from vfio-ccw and the other drivers (hello,
CC list!), letting it be handled by vfio_device_release directly.
Looking forward to the feedback.
Thanks,
Eric
[1] https://lore.kernel.org/kvm/[email protected]/
[2] https://lore.kernel.org/kvm/[email protected]/
[3] https://lore.kernel.org/kvm/[email protected]/
[4] https://lore.kernel.org/kvm/[email protected]/
Eric Farman (7):
vfio/ccw: create a parent struct
vfio/ccw: remove private->sch
vfio/ccw: move private initialization to callback
vfio/ccw: move private to mdev lifecycle
vfio/ccw: remove release completion
vfio/ccw: replace vfio_init_device with _alloc_
vfio: Remove vfio_free_device
drivers/gpu/drm/i915/gvt/kvmgt.c | 1 -
drivers/s390/cio/vfio_ccw_chp.c | 5 +-
drivers/s390/cio/vfio_ccw_drv.c | 182 ++++++++++----------------
drivers/s390/cio/vfio_ccw_fsm.c | 27 ++--
drivers/s390/cio/vfio_ccw_ops.c | 108 ++++++++++-----
drivers/s390/cio/vfio_ccw_parent.h | 28 ++++
drivers/s390/cio/vfio_ccw_private.h | 22 ++--
drivers/s390/crypto/vfio_ap_ops.c | 6 -
drivers/vfio/fsl-mc/vfio_fsl_mc.c | 1 -
drivers/vfio/pci/vfio_pci_core.c | 1 -
drivers/vfio/platform/vfio_amba.c | 1 -
drivers/vfio/platform/vfio_platform.c | 1 -
drivers/vfio/vfio_main.c | 25 +---
include/linux/vfio.h | 1 -
samples/vfio-mdev/mbochs.c | 1 -
samples/vfio-mdev/mdpy.c | 1 -
samples/vfio-mdev/mtty.c | 1 -
17 files changed, 204 insertions(+), 208 deletions(-)
create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
--
2.34.1
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 <[email protected]>
---
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);
- 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);
--
2.34.1
Now that the mdev parent data is split out into its own struct,
it is safe to move the remaining private data to follow the
mdev probe/remove lifecycle. The mdev parent data will remain
where it is, and follow the subchannel and the css driver
interfaces.
Signed-off-by: Eric Farman <[email protected]>
---
drivers/s390/cio/vfio_ccw_drv.c | 17 ++---------------
drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++++-------------
drivers/s390/cio/vfio_ccw_private.h | 3 +++
3 files changed, 18 insertions(+), 28 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index cc9ed2fd970f..686a9b9f6731 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -146,7 +146,7 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
}
-static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
+struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
{
struct vfio_ccw_private *private;
@@ -157,7 +157,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
return private;
}
-static void vfio_ccw_free_private(struct vfio_ccw_private *private)
+void vfio_ccw_free_private(struct vfio_ccw_private *private)
{
struct vfio_ccw_crw *crw, *temp;
@@ -185,7 +185,6 @@ static void vfio_ccw_free_parent(struct device *dev)
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;
@@ -202,14 +201,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
parent->dev.release = &vfio_ccw_free_parent;
device_initialize(&parent->dev);
- private = vfio_ccw_alloc_private(sch);
- if (IS_ERR(private)) {
- put_device(&parent->dev);
- return PTR_ERR(private);
- }
-
dev_set_drvdata(&sch->dev, parent);
- dev_set_drvdata(&parent->dev, private);
parent->mdev_type.sysfs_name = "io";
parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
@@ -226,9 +218,7 @@ 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;
}
@@ -236,13 +226,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
static void vfio_ccw_sch_remove(struct subchannel *sch)
{
struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
- struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
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",
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 626b8eb3507b..3e627b236241 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
{
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);
+ struct vfio_ccw_private *private;
int ret;
- if (private->state == VFIO_CCW_STATE_NOT_OPER)
- return -ENODEV;
+ private = vfio_ccw_alloc_private(sch);
+ if (!private)
+ return -ENOMEM;
ret = vfio_init_device(&private->vdev, &mdev->dev, &vfio_ccw_dev_ops);
- if (ret)
+ if (ret) {
+ kfree(private);
return ret;
+ }
+
+ dev_set_drvdata(&parent->dev, private);
VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
sch->schid.cssid,
@@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
return 0;
err_put_vdev:
+ dev_set_drvdata(&parent->dev, NULL);
vfio_put_device(&private->vdev);
return ret;
}
@@ -132,15 +138,6 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
struct vfio_ccw_private *private =
container_of(vdev, struct vfio_ccw_private, vdev);
- /*
- * We cannot free vfio_ccw_private here because it includes
- * parent info which must be free'ed by css driver.
- *
- * Use a workaround by memset'ing the core device part and
- * then notifying the remove path that all active references
- * to this device have been released.
- */
- memset(vdev, 0, sizeof(*vdev));
complete(&private->release_comp);
}
@@ -157,6 +154,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
vfio_unregister_group_dev(&private->vdev);
+ dev_set_drvdata(&parent->dev, NULL);
vfio_put_device(&private->vdev);
/*
* Wait for all active references on mdev are released so it
@@ -167,6 +165,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
* cycle.
*/
wait_for_completion(&private->release_comp);
+
+ vfio_ccw_free_private(private);
}
static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index b35940057073..c1959b8bfe86 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -119,6 +119,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch);
void vfio_ccw_sch_io_todo(struct work_struct *work);
void vfio_ccw_crw_todo(struct work_struct *work);
+struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch);
+void vfio_ccw_free_private(struct vfio_ccw_private *private);
+
extern struct mdev_driver vfio_ccw_mdev_driver;
/*
--
2.34.1
On Wed, Oct 19, 2022 at 06:21:28PM +0200, Eric Farman wrote:
> This series attempts to address the oddities/shortcomings of vfio-ccw, such
> that vfio-ccw can use the same helpers as everyone else, and the tricks that
> were implemented by the other drivers can be removed. It is built on 6.1-rc1,
> and thus includes the various changes [3][4] that have occurred in and around
> these parts.
It looks good to me if you are happy with the ccw part
Thanks,
Jason
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 <[email protected]>
> ---
> 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);
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 <[email protected]>
> > ---
> > 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 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?
>
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.
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 <[email protected]>
> ---
> 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?
> From: Eric Farman <[email protected]>
> Sent: Thursday, October 20, 2022 12:22 AM
>
> @@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct
> mdev_device *mdev)
> {
> 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);
> + struct vfio_ccw_private *private;
> int ret;
>
> - if (private->state == VFIO_CCW_STATE_NOT_OPER)
> - return -ENODEV;
Not familiar with ccw but just want to double confirm this removal
is intentional w/o side-effect?
> + private = vfio_ccw_alloc_private(sch);
> + if (!private)
> + return -ENOMEM;
>
> ret = vfio_init_device(&private->vdev, &mdev->dev,
> &vfio_ccw_dev_ops);
> - if (ret)
> + if (ret) {
> + kfree(private);
> return ret;
> + }
> +
> + dev_set_drvdata(&parent->dev, private);
>
> VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> sch->schid.cssid,
> @@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device
> *mdev)
> return 0;
>
> err_put_vdev:
> + dev_set_drvdata(&parent->dev, NULL);
No need to set drvdata to NULL, iiuc
On Tue, 2022-11-01 at 09:08 +0000, Tian, Kevin wrote:
> > From: Eric Farman <[email protected]>
> > Sent: Thursday, October 20, 2022 12:22 AM
> >
> > @@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device *mdev)
> > {
> > 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);
> > + struct vfio_ccw_private *private;
> > int ret;
> >
> > - if (private->state == VFIO_CCW_STATE_NOT_OPER)
> > - return -ENODEV;
>
> Not familiar with ccw but just want to double confirm this removal
> is intentional w/o side-effect?
Right, it's intentional and fine. The concern previously was re-probing
the mdev when a device had gone into a non-recoverable state and the
private data was left hanging around. With the private now being
cleaned up in mdev_remove instead of the subchannel side, that's no
longer an issue.
>
> > + private = vfio_ccw_alloc_private(sch);
> > + if (!private)
> > + return -ENOMEM;
> >
> > ret = vfio_init_device(&private->vdev, &mdev->dev,
> > &vfio_ccw_dev_ops);
> > - if (ret)
> > + if (ret) {
> > + kfree(private);
> > return ret;
> > + }
> > +
> > + dev_set_drvdata(&parent->dev, private);
> >
> > VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> > sch->schid.cssid,
> > @@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device
> > *mdev)
> > return 0;
> >
> > err_put_vdev:
> > + dev_set_drvdata(&parent->dev, NULL);
>
> No need to set drvdata to NULL, iiuc
Fair.