Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <431304f4-cbe9-6453-cd3a-0843972ca368@linux.ibm.com>
Date:   Wed, 2 Nov 2022 16:00:42 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH v2 1/7] vfio/ccw: create a parent struct
Content-Language: en-US
To:     Eric Farman <farman@linux.ibm.com>,
        Alex Williamson <alex.williamson@redhat.com>,
        Cornelia Huck <cohuck@redhat.com>,
        Jason Gunthorpe <jgg@nvidia.com>,
        Kevin Tian <kevin.tian@intel.com>, Yi Liu <yi.l.liu@intel.com>
Cc:     Zhenyu Wang <zhenyuw@linux.intel.com>,
        Zhi Wang <zhi.a.wang@intel.com>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Halil Pasic <pasic@linux.ibm.com>,
        Vineeth Vijayan <vneethv@linux.ibm.com>,
        Peter Oberparleiter <oberpar@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Alexander Gordeev <agordeev@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Tony Krowiak <akrowiak@linux.ibm.com>,
        Jason Herne <jjherne@linux.ibm.com>,
        Harald Freudenberger <freude@linux.ibm.com>,
        Diana Craciun <diana.craciun@oss.nxp.com>,
        Eric Auger <eric.auger@redhat.com>,
        Kirti Wankhede <kwankhede@nvidia.com>,
        Abhishek Sahu <abhsahu@nvidia.com>,
        Yishai Hadas <yishaih@nvidia.com>,
        intel-gvt-dev@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
        kvm@vger.kernel.org
References: <20221102150152.2521475-1-farman@linux.ibm.com>
 <20221102150152.2521475-2-farman@linux.ibm.com>
 <df037cd2e564acaa14c5a3358fd3386df29ad61e.camel@linux.ibm.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <df037cd2e564acaa14c5a3358fd3386df29ad61e.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.