Hi all,
Here is an update to the vfio-ccw lifecycle changes that have been discussed
in various forms over the past year [1][2] or so, and which I dusted off
recently.
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]/
v1->v2:
- Rebase to 6.1-rc3
- Patch 1:
[EF] s/device_initialize/device_register/ and associated adjustments
[MR] Add WARN_ON(!private) in vfio_ccw_sch_quiesce()
[MR] Move struct vfio_ccw_parent to _private.h, instead of standalone file
- Patch 2:
[MR] Added r-b (Thank you!)
- Patch 3:
[MR] Update commit message to point to introduction of private->release_comp
[MR] Replace the remnants of vfio_ccw_alloc_private with a straight kzalloc
[MR] Added r-b (Thank you!)
- Patch 5:
[KT] Added r-b (Thank you!)
- Patch 6:
[JG] Make vfio_init_device static
[KT] Added r-b (Thank you!)
- Patch 7:
[JG, KT] Added r-b (Thank you!)
v1: 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 | 174 +++++++++++---------------
drivers/s390/cio/vfio_ccw_fsm.c | 27 ++--
drivers/s390/cio/vfio_ccw_ops.c | 107 +++++++++++-----
drivers/s390/cio/vfio_ccw_private.h | 37 ++++--
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 | 32 ++---
include/linux/vfio.h | 3 -
samples/vfio-mdev/mbochs.c | 1 -
samples/vfio-mdev/mdpy.c | 1 -
samples/vfio-mdev/mtty.c | 1 -
16 files changed, 197 insertions(+), 202 deletions(-)
--
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 | 15 +--------------
drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++++-------------
drivers/s390/cio/vfio_ccw_private.h | 2 ++
3 files changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index fd5720cbf005..041cc0860f0e 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -151,7 +151,7 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
}
-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;
@@ -179,7 +179,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;
@@ -200,14 +199,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
if (ret)
goto out_free;
- private = kzalloc(sizeof(*private), GFP_KERNEL);
- 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)
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);
return ret;
}
@@ -236,14 +226,11 @@ 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);
device_unregister(&parent->dev);
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 eb0b8cc210bb..e45d4acb109b 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -100,15 +100,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 = kzalloc(sizeof(*private), GFP_KERNEL);
+ 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,
@@ -122,6 +127,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;
}
@@ -131,15 +137,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);
}
@@ -156,6 +153,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
@@ -166,6 +164,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 55d636225cff..747aba5f5272 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -134,6 +134,8 @@ 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);
+void vfio_ccw_free_private(struct vfio_ccw_private *private);
+
extern struct mdev_driver vfio_ccw_mdev_driver;
/*
--
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 | 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);
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);
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);
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);
--
2.34.1
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 <[email protected]>
> ---
> 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);
On 11/2/22 3:29 PM, Eric Farman wrote:
> 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 <[email protected]>
>> ---
>> 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
...
>> 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.
>
Thanks, with these changes I no longer see refcount underflows. I'll continue reviewing with those changes presumed for v3.
On Wed, 2 Nov 2022 16:01:45 +0100
Eric Farman <[email protected]> wrote:
> Hi all,
>
> Here is an update to the vfio-ccw lifecycle changes that have been discussed
> in various forms over the past year [1][2] or so, and which I dusted off
> recently.
>
> 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.
Looks like another spin is pending, but the vfio core and collateral
changes in 6 and 7 look good to me. Would this go in through the vfio
or s390 tree? I'd be happy to merge or provide a branch, depending on
the route.
For 6 & 7:
Acked-by: Alex Williamson <[email protected]>
Thanks,
Alex
> Looking forward to the feedback.
>
> Thanks,
> Eric
>
> [1] https://lore.kernel.org/kvm/[email protected]/
> [2] https://lore.kernel.org/kvm/[email protected]/
>
> v1->v2:
> - Rebase to 6.1-rc3
> - Patch 1:
> [EF] s/device_initialize/device_register/ and associated adjustments
> [MR] Add WARN_ON(!private) in vfio_ccw_sch_quiesce()
> [MR] Move struct vfio_ccw_parent to _private.h, instead of standalone file
> - Patch 2:
> [MR] Added r-b (Thank you!)
> - Patch 3:
> [MR] Update commit message to point to introduction of private->release_comp
> [MR] Replace the remnants of vfio_ccw_alloc_private with a straight kzalloc
> [MR] Added r-b (Thank you!)
> - Patch 5:
> [KT] Added r-b (Thank you!)
> - Patch 6:
> [JG] Make vfio_init_device static
> [KT] Added r-b (Thank you!)
> - Patch 7:
> [JG, KT] Added r-b (Thank you!)
> v1: 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 | 174 +++++++++++---------------
> drivers/s390/cio/vfio_ccw_fsm.c | 27 ++--
> drivers/s390/cio/vfio_ccw_ops.c | 107 +++++++++++-----
> drivers/s390/cio/vfio_ccw_private.h | 37 ++++--
> 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 | 32 ++---
> include/linux/vfio.h | 3 -
> samples/vfio-mdev/mbochs.c | 1 -
> samples/vfio-mdev/mdpy.c | 1 -
> samples/vfio-mdev/mtty.c | 1 -
> 16 files changed, 197 insertions(+), 202 deletions(-)
>
On 11/2/22 11:01 AM, 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 | 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);
The error here would be a null ptr due to failed alloc, how about:
if (!parent)
return -ENOMEM;
> +
> + 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);
As you said earlier, unregister_device()
> 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);
if device_register(&parent->dev) failed above, you will goto out_free and call vfio_ccw_free_private before having done vfio_ccw_alloc_private (e.g. private==NULL). Doesn't look like vfio_ccw_free_private handles that -- Either check !parent here or add a check to vfio_ccw_free_private.
> + put_device(&parent->dev);
As you said in your other reply, this goes away
> 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);
As you said in your other reply, this goes away
The rest looks fine, with these changes you can have:
Reviewed-by: Matthew Rosato <[email protected]>
On 11/3/22 5:56 PM, Alex Williamson wrote:
> On Wed, 2 Nov 2022 16:01:45 +0100
> Eric Farman <[email protected]> wrote:
>
>> Hi all,
>>
>> Here is an update to the vfio-ccw lifecycle changes that have been discussed
>> in various forms over the past year [1][2] or so, and which I dusted off
>> recently.
>>
>> 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.
>
> Looks like another spin is pending, but the vfio core and collateral
> changes in 6 and 7 look good to me. Would this go in through the vfio
> or s390 tree? I'd be happy to merge or provide a branch, depending on
> the route.
>
> For 6 & 7:
> Acked-by: Alex Williamson <[email protected]>
>
> Thanks,
> Alex
LGTM with those few comments addressed -- @Eric please send a v3 and I think it's ready.
I would suggest vfio tree to reduce the chance of conflicts; this touches various vfio drivers (and main) with the last patches while the s390 hits are at least all contained to the vfio-ccw driver code.
On 11/2/22 11:01 AM, Eric Farman wrote:
> 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 | 15 +--------------
> drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++++-------------
> drivers/s390/cio/vfio_ccw_private.h | 2 ++
> 3 files changed, 16 insertions(+), 27 deletions(-)
>
...
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index eb0b8cc210bb..e45d4acb109b 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -100,15 +100,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 = kzalloc(sizeof(*private), GFP_KERNEL);
> + if (!private)
> + return -ENOMEM;
Ha, looks like you time traveled and took my advice :)
In fact it looks like some of my other comments from patch 1 get cleaned up here too -- but would still be good to make those changes in patch 1 for completeness/bisect.
Reviewed-by: Matthew Rosato <[email protected]>
On Thu, 2022-11-03 at 19:43 -0400, Matthew Rosato wrote:
> On 11/3/22 5:56 PM, Alex Williamson wrote:
> > On Wed, 2 Nov 2022 16:01:45 +0100
> > Eric Farman <[email protected]> wrote:
> >
> > > Hi all,
> > >
> > > Here is an update to the vfio-ccw lifecycle changes that have
> > > been discussed
> > > in various forms over the past year [1][2] or so, and which I
> > > dusted off
> > > recently.
> > >
> > > 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.
> >
> > Looks like another spin is pending, but the vfio core and
> > collateral
> > changes in 6 and 7 look good to me. Would this go in through the
> > vfio
> > or s390 tree? I'd be happy to merge or provide a branch, depending
> > on
> > the route.
> >
> > For 6 & 7:
> > Acked-by: Alex Williamson <[email protected]>
> >
> > Thanks,
> > Alex
>
> LGTM with those few comments addressed -- @Eric please send a v3 and
> I think it's ready.
Will do that now; thanks Matt.
>
> I would suggest vfio tree to reduce the chance of conflicts; this
> touches various vfio drivers (and main) with the last patches while
> the s390 hits are at least all contained to the vfio-ccw driver code.
>
Agreed. Thanks to you both.
On Thu, 2022-11-03 at 19:22 -0400, Matthew Rosato wrote:
> On 11/2/22 11:01 AM, Eric Farman wrote:
> > 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 | 15 +--------------
> > drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++++------------
> > -
> > drivers/s390/cio/vfio_ccw_private.h | 2 ++
> > 3 files changed, 16 insertions(+), 27 deletions(-)
> >
>
> ...
>
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index eb0b8cc210bb..e45d4acb109b 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -100,15 +100,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 = kzalloc(sizeof(*private), GFP_KERNEL);
> > + if (!private)
> > + return -ENOMEM;
>
> Ha, looks like you time traveled and took my advice :)
Ha, I forgot I did this in the future. :)
>
> In fact it looks like some of my other comments from patch 1 get
> cleaned up here too -- but would still be good to make those changes
> in patch 1 for completeness/bisect.
Agreed, I'll pull those down to patch 1; thanks.
>
> Reviewed-by: Matthew Rosato <[email protected]>
>