Re: [PATCH v2 3/9] vfio/ccw: Convert to use vfio_register_group_dev()

[Eric Farman <farman@linux.ibm.com>]

On Thu, 2021-09-09 at 16:38 -0300, Jason Gunthorpe wrote:
> This is a more complicated conversion because vfio_ccw is sharing the
> vfio_device between both the mdev_device, its vfio_device and the
> css_driver.
> 
> The mdev is a singleton, and the reason for this sharing is so the
> extra
> css_driver function callbacks to be delivered to the vfio_device
> implementation.
> 
> This keeps things as they are, with the css_driver allocating the
> singleton, not the mdev_driver. Following patches work to clean this
> further.
> 
> Embed the vfio_device in the vfio_ccw_private and instantiate it as a
> vfio_device when the mdev probes. The drvdata of both the css_device
> and
> the mdev_device point at the private, and container_of is used to get
> it
> back from the vfio_device.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     |  21 ++++--
>  drivers/s390/cio/vfio_ccw_ops.c     | 107 +++++++++++++++++---------
> --
>  drivers/s390/cio/vfio_ccw_private.h |   5 ++
>  3 files changed, 85 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> b/drivers/s390/cio/vfio_ccw_drv.c
> index 1e8d3151e5480e..396e815f81f8a4 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -469,7 +469,7 @@ static int __init vfio_ccw_sch_init(void)
>  	vfio_ccw_work_q = create_singlethread_workqueue("vfio-ccw");
>  	if (!vfio_ccw_work_q) {
>  		ret = -ENOMEM;
> -		goto out_err;
> +		goto out_regions;
>  	}
>  
>  	vfio_ccw_io_region =
> kmem_cache_create_usercopy("vfio_ccw_io_region",
> @@ -478,7 +478,7 @@ static int __init vfio_ccw_sch_init(void)
>  					sizeof(struct ccw_io_region),
> NULL);
>  	if (!vfio_ccw_io_region) {
>  		ret = -ENOMEM;
> -		goto out_err;
> +		goto out_regions;
>  	}
>  
>  	vfio_ccw_cmd_region =
> kmem_cache_create_usercopy("vfio_ccw_cmd_region",
> @@ -487,7 +487,7 @@ static int __init vfio_ccw_sch_init(void)
>  					sizeof(struct ccw_cmd_region),
> NULL);
>  	if (!vfio_ccw_cmd_region) {
>  		ret = -ENOMEM;
> -		goto out_err;
> +		goto out_regions;
>  	}
>  
>  	vfio_ccw_schib_region =
> kmem_cache_create_usercopy("vfio_ccw_schib_region",
> @@ -497,7 +497,7 @@ static int __init vfio_ccw_sch_init(void)
>  
>  	if (!vfio_ccw_schib_region) {
>  		ret = -ENOMEM;
> -		goto out_err;
> +		goto out_regions;
>  	}
>  
>  	vfio_ccw_crw_region =
> kmem_cache_create_usercopy("vfio_ccw_crw_region",
> @@ -507,19 +507,25 @@ static int __init vfio_ccw_sch_init(void)
>  
>  	if (!vfio_ccw_crw_region) {
>  		ret = -ENOMEM;
> -		goto out_err;
> +		goto out_regions;
>  	}
>  
> +	ret = mdev_register_driver(&vfio_ccw_mdev_driver);
> +	if (ret)
> +		goto out_regions;
> +
>  	isc_register(VFIO_CCW_ISC);
>  	ret = css_driver_register(&vfio_ccw_sch_driver);
>  	if (ret) {
>  		isc_unregister(VFIO_CCW_ISC);
> -		goto out_err;
> +		goto out_driver;
>  	}
>  
>  	return ret;
>  
> -out_err:
> +out_driver:
> +	mdev_unregister_driver(&vfio_ccw_mdev_driver);
> +out_regions:
>  	vfio_ccw_destroy_regions();
>  	destroy_workqueue(vfio_ccw_work_q);
>  	vfio_ccw_debug_exit();
> @@ -528,6 +534,7 @@ static int __init vfio_ccw_sch_init(void)
>  
>  static void __exit vfio_ccw_sch_exit(void)
>  {
> +	mdev_unregister_driver(&vfio_ccw_mdev_driver);

Wouldn't it be better to mirror the unwind-init case, such that the
above goes...

>  	css_driver_unregister(&vfio_ccw_sch_driver);
>  	isc_unregister(VFIO_CCW_ISC);

...here?

>  	vfio_ccw_destroy_regions();
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c 

...snip...

Besides that, looks fine to me.