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.
+ 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_ */
