> 2025年1月9日 00:04，Mario Limonciello <mario.limonciello@xxxxxxx> 写道：
>
> 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@xxxxxxxxxxxxxxxxx>
>> ---
>> 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.
Thanks for introducing this new utility, which makes me feel like writing rust code:)
>
>> + 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;
Will fix it in next version.
>
>> + }
>> 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_ */
