Re: [PATCH v2 6/8] fpga: mgr: Use standard dev_release for class driver

From: Xu Yilun <yilun.xu@intel.com>
To: Russ Weight <russell.h.weight@intel.com>
Cc: mdf@kernel.org, linux-fpga@vger.kernel.org, trix@redhat.com,
	lgoncalv@redhat.com, hao.wu@intel.com, matthew.gerlach@intel.com,
	richard.gong@intel.com
Subject: Re: [PATCH v2 6/8] fpga: mgr: Use standard dev_release for class driver
Date: Thu, 10 Jun 2021 16:13:22 +0800	[thread overview]
Message-ID: <20210610081322.GE2007225@yilunxu-OptiPlex-7050> (raw)
In-Reply-To: <20210609221135.261837-7-russell.h.weight@intel.com>

Looks good to me.

Reviewed-by: Xu Yilun <yilun.xu@intel.com>

On Wed, Jun 09, 2021 at 03:11:33PM -0700, Russ Weight wrote:
> The FPGA manager class driver data structure is being treated as a
> managed resource instead of using the class.dev_release call-back
> function to release the class data structure. This change populates
> the class.dev_release function, changes the fpga_mgr_free() function
> to call put_device() and changes the fpga_mgr_unregister() function
> to call device_del() instead of device_unregister().
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v2:
>   - Moved the renaming of "dev" to "parent" to a separate patch
>   - Call fpga_mgr_free() in devm_fpga_mgr_release() instead of put_device()
> ---
>  drivers/fpga/fpga-mgr.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 42ddc0844781..9f6c3760b6ff 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -585,8 +585,10 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>  		return NULL;
>  
>  	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> -	if (id < 0)
> -		goto error_kfree;
> +	if (id < 0) {
> +		kfree(mgr);
> +		return NULL;
> +	}
>  
>  	mutex_init(&mgr->ref_mutex);
>  
> @@ -602,17 +604,12 @@ struct fpga_manager *fpga_mgr_create(struct device *parent, const char *name,
>  	mgr->dev.id = id;
>  
>  	ret = dev_set_name(&mgr->dev, "fpga%d", id);
> -	if (ret)
> -		goto error_device;
> +	if (ret) {
> +		put_device(&mgr->dev);
> +		return NULL;
> +	}
>  
>  	return mgr;
> -
> -error_device:
> -	ida_simple_remove(&fpga_mgr_ida, id);
> -error_kfree:
> -	kfree(mgr);
> -
> -	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_create);
>  
> @@ -622,8 +619,7 @@ EXPORT_SYMBOL_GPL(fpga_mgr_create);
>   */
>  void fpga_mgr_free(struct fpga_manager *mgr)
>  {
> -	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> -	kfree(mgr);
> +	put_device(&mgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_free);
>  
> @@ -692,16 +688,11 @@ int fpga_mgr_register(struct fpga_manager *mgr)
>  
>  	ret = device_add(&mgr->dev);
>  	if (ret)
> -		goto error_device;
> +		return ret;
>  
>  	dev_info(&mgr->dev, "%s registered\n", mgr->name);
>  
>  	return 0;
> -
> -error_device:
> -	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> -
> -	return ret;
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_register);
>  
> @@ -722,7 +713,7 @@ void fpga_mgr_unregister(struct fpga_manager *mgr)
>  	if (mgr->mops->fpga_remove)
>  		mgr->mops->fpga_remove(mgr);
>  
> -	device_unregister(&mgr->dev);
> +	device_del(&mgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>  
> @@ -781,6 +772,10 @@ EXPORT_SYMBOL_GPL(devm_fpga_mgr_register);
>  
>  static void fpga_mgr_dev_release(struct device *dev)
>  {
> +	struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> +	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
> +	kfree(mgr);
>  }
>  
>  static int __init fpga_mgr_class_init(void)
> -- 
> 2.25.1