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@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
...
>> 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.
