[v1,4/7] vfio/ccw: move private to mdev lifecycle
Commit Message
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 <farman@linux.ibm.com>
---
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(-)
Comments
> From: Eric Farman <farman@linux.ibm.com>
> 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 <farman@linux.ibm.com>
> > 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.
@@ -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",
@@ -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)
@@ -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;
/*