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 <farman@xxxxxxxxxxxxx>
> ---
> 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 <mjrosato@xxxxxxxxxxxxx>
