[v3 2/6] drm/amdxcp: introduce new API amdgpu_xcp_drm_dev_free()

[Mario Limonciello]

On 1/8/2025 02:56, Jiang Liu wrote:
> Introduce new interface amdgpu_xcp_drm_dev_free() to free a specific
> drm_device crreated by amdgpu_xcp_drm_dev_alloc(), which will be used
> to do error recovery.
> 
> Signed-off-by: Jiang Liu <gerry at linux.alibaba.com>
> ---
>   drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c | 76 +++++++++++++++++----
>   drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h |  1 +
>   2 files changed, 63 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
> index faed84172dd4..fc92b5fe1040 100644
> --- a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
> +++ b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.c
> @@ -45,21 +45,32 @@ static const struct drm_driver amdgpu_xcp_driver = {
>   
>   static int8_t pdev_num;
>   static struct xcp_device *xcp_dev[MAX_XCP_PLATFORM_DEVICE];
> +static DEFINE_MUTEX(xcp_mutex);
>   
>   int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>   {
>   	struct platform_device *pdev;
>   	struct xcp_device *pxcp_dev;
>   	char dev_name[20];
> -	int ret;
> +	int ret, index;
>   
> -	if (pdev_num >= MAX_XCP_PLATFORM_DEVICE)
> -		return -ENODEV;
> +	mutex_lock(&xcp_mutex);

For "new" code what do you think about about using scoped guards like 
guard(mutex) instead of lock; goto label; unlock pattern?

I think it can generally keep the code cleaner, but you need to be 
careful because if you still need "other" goto cleanup patterns you can 
unintentionally break the compiled output.

> +	if (pdev_num >= MAX_XCP_PLATFORM_DEVICE) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
> +		if (!xcp_dev[index])
> +			break;
> +	}
>   
> -	snprintf(dev_name, sizeof(dev_name), "amdgpu_xcp_%d", pdev_num);
> +	snprintf(dev_name, sizeof(dev_name), "amdgpu_xcp_%d", index);
>   	pdev = platform_device_register_simple(dev_name, -1, NULL, 0);
> -	if (IS_ERR(pdev))
> -		return PTR_ERR(pdev);
> +	if (IS_ERR(pdev)) {
> +		ret = PTR_ERR(pdev);
> +		goto out_unregister;

Er, if you fail to register why would you unregister?  I think in this 
case with the current code you would 'goto out_unlock' instead.

The design pattern you might have been following was from platform 
drivers that do this, which is different:

platform_driver_register()
foo = platform_device_register_simple()
if (IS_ERR(foo))
	platform_driver_unregister()
return 0;

> +	}
>   
>   	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
>   		ret = -ENOMEM;
> @@ -72,10 +83,11 @@ int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>   		goto out_devres;
>   	}
>   
> -	xcp_dev[pdev_num] = pxcp_dev;
> -	xcp_dev[pdev_num]->pdev = pdev;
> +	xcp_dev[index] = pxcp_dev;
> +	xcp_dev[index]->pdev = pdev;
>   	*ddev = &pxcp_dev->drm;
>   	pdev_num++;
> +	mutex_unlock(&xcp_mutex);
>   
>   	return 0;
>   
> @@ -83,21 +95,57 @@ int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev)
>   	devres_release_group(&pdev->dev, NULL);
>   out_unregister:
>   	platform_device_unregister(pdev);
> +out_unlock:
> +	mutex_unlock(&xcp_mutex);
>   
>   	return ret;
>   }
>   EXPORT_SYMBOL(amdgpu_xcp_drm_dev_alloc);
>   
> +static void amdgpu_xcp_drm_dev_destroy(int index)
> +{
> +	struct platform_device *pdev;
> +
> +	pdev = xcp_dev[index]->pdev;
> +	devres_release_group(&pdev->dev, NULL);
> +	platform_device_unregister(pdev);
> +	xcp_dev[index] = NULL;
> +	pdev_num--;
> +}
> +
> +void amdgpu_xcp_drm_dev_free(struct drm_device *ddev)
> +{
> +	int index;
> +	struct xcp_device *pxcp_dev;
> +
> +	if (ddev == NULL)
> +		return;
> +
> +	pxcp_dev = container_of(ddev, struct xcp_device, drm);
> +
> +	mutex_lock(&xcp_mutex);

I think this is a good case for a guard(mutex) instead.

> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
> +		if (xcp_dev[index] == pxcp_dev) {
> +			amdgpu_xcp_drm_dev_destroy(index);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&xcp_mutex);
> +}
> +EXPORT_SYMBOL(amdgpu_xcp_drm_dev_free);
> +
>   void amdgpu_xcp_drv_release(void)
>   {
> -	for (--pdev_num; pdev_num >= 0; --pdev_num) {
> -		struct platform_device *pdev = xcp_dev[pdev_num]->pdev;
> +	int index;
>   
> -		devres_release_group(&pdev->dev, NULL);
> -		platform_device_unregister(pdev);
> -		xcp_dev[pdev_num] = NULL;
> +	mutex_lock(&xcp_mutex);

Another good case for guard(mutex)

> +	for (index = 0; index < MAX_XCP_PLATFORM_DEVICE; index++) {
> +		if (xcp_dev[index]) {
> +			amdgpu_xcp_drm_dev_destroy(index);
> +		}
>   	}
> -	pdev_num = 0;
> +	WARN_ON(pdev_num != 0);
> +	mutex_unlock(&xcp_mutex);
>   }
>   EXPORT_SYMBOL(amdgpu_xcp_drv_release);
>   
> diff --git a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
> index c1c4b679bf95..580a1602c8e3 100644
> --- a/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
> +++ b/drivers/gpu/drm/amd/amdxcp/amdgpu_xcp_drv.h
> @@ -25,5 +25,6 @@
>   #define _AMDGPU_XCP_DRV_H_
>   
>   int amdgpu_xcp_drm_dev_alloc(struct drm_device **ddev);
> +void amdgpu_xcp_drm_dev_free(struct drm_device *ddev);
>   void amdgpu_xcp_drv_release(void);
>   #endif /* _AMDGPU_XCP_DRV_H_ */