Re: [PATCH v2 1/7] vfio/ccw: create a parent struct

[Matthew Rosato <mjrosato@linux.ibm.com>]

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.