[v2,1/7] vfio/ccw: create a parent struct
Commit Message
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@linux.ibm.com>
---
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(-)
Comments
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 <farman@linux.ibm.com>
> ---
> 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 <farman@linux.ibm.com>
>> ---
>> 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 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@linux.ibm.com>
> ---
> 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@linux.ibm.com>
@@ -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;
@@ -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,
@@ -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);