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: <b0642bbc-b660-eb08-b965-d454053d59c3@linux.ibm.com>
Date:   Thu, 3 Nov 2022 19:10:33 -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>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <20221102150152.2521475-2-farman@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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>