[Intel-xe] [PATCH v2 1/2] drm/xe/mmio: don't leak the forcewake ref

[Lucas De Marchi]

On Tue, Feb 14, 2023 at 05:32:23PM +0000, Matthew Auld wrote:
>Make sure we properly release the forcewake ref on all error paths.
>
>Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>---
> drivers/gpu/drm/xe/xe_mmio.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>index 8a953df2b468..497f643271ae 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.c
>+++ b/drivers/gpu/drm/xe/xe_mmio.c
>@@ -426,12 +426,16 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
> 	if (args->flags & DRM_XE_MMIO_WRITE) {
> 		switch (bits_flag) {
> 		case DRM_XE_MMIO_8BIT:
>-			return -EINVAL; /* TODO */
>+			ret = -EINVAL; /* TODO */
>+			goto exit;
> 		case DRM_XE_MMIO_16BIT:
>-			return -EINVAL; /* TODO */
>+			ret = -EINVAL; /* TODO */
>+			goto exit;
> 		case DRM_XE_MMIO_32BIT:
>-			if (XE_IOCTL_ERR(xe, args->value > U32_MAX))
>-				return -EINVAL;
>+			if (XE_IOCTL_ERR(xe, args->value > U32_MAX)) {
>+				ret = -EINVAL;
>+				goto exit;
>+			}
> 			xe_mmio_write32(to_gt(xe), args->addr, args->value);
> 			break;
> 		case DRM_XE_MMIO_64BIT:
>@@ -447,9 +451,11 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
> 	if (args->flags & DRM_XE_MMIO_READ) {
> 		switch (bits_flag) {
> 		case DRM_XE_MMIO_8BIT:
>-			return -EINVAL; /* TODO */
>+			ret = -EINVAL; /* TODO */
>+			break;
> 		case DRM_XE_MMIO_16BIT:
>-			return -EINVAL; /* TODO */
>+			ret = -EINVAL; /* TODO */
>+			break;


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>


Suggestion, up to you:  move the read and write to separate functions
and just surround the them with the forcewake. Or just moving the
default and adding a fallthrough would make it less verbose too. Up to
you.

-----------------8<----------------
diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
index ced757ba9ccb..10704d8ed8b8 100644
--- a/drivers/gpu/drm/xe/xe_mmio.c
+++ b/drivers/gpu/drm/xe/xe_mmio.c
@@ -383,6 +383,52 @@ static const i915_reg_t mmio_read_whitelist[] = {
  	RING_TIMESTAMP(RENDER_RING_BASE),
  };
  
+static int mmio_ioctl_read(struct xe_device *xe,
+			   struct drm_xe_mmio *args)
+{
+	switch (args->flags & DRM_XE_MMIO_BITS_MASK) {
+	case DRM_XE_MMIO_32BIT:
+		if (XE_IOCTL_ERR(xe, args->value > U32_MAX))
+			return -EINVAL;
+		xe_mmio_write32(to_gt(xe), args->addr, args->value);
+		break;
+	case DRM_XE_MMIO_64BIT:
+		xe_mmio_write64(to_gt(xe), args->addr, args->value);
+		break;
+
+	default:
+		drm_WARN(&xe->drm, 1, "Invalid MMIO bit size");
+		fallthrough;
+	case DRM_XE_MMIO_8BIT: /* TODO */
+	case DRM_XE_MMIO_16BIT: /* TODO */
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int mmio_ioctl_write(struct xe_device *xe,
+			    struct drm_xe_mmio *args)
+{
+	switch (args->flags & DRM_XE_MMIO_BITS_MASK) {
+	case DRM_XE_MMIO_32BIT:
+		args->value = xe_mmio_read32(to_gt(xe), args->addr);
+		break;
+	case DRM_XE_MMIO_64BIT:
+		args->value = xe_mmio_read64(to_gt(xe), args->addr);
+		break;
+
+	default:
+		drm_WARN(&xe->drm, 1, "Invalid MMIO bit size");
+		fallthrough;
+	case DRM_XE_MMIO_8BIT: /* TODO */
+	case DRM_XE_MMIO_16BIT: /* TODO */
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
  int xe_mmio_ioctl(struct drm_device *dev, void *data,
  		  struct drm_file *file)
  {
@@ -424,43 +470,13 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
  	xe_force_wake_get(gt_to_fw(&xe->gt[0]), XE_FORCEWAKE_ALL);
  
  	if (args->flags & DRM_XE_MMIO_WRITE) {
-		switch (bits_flag) {
-		case DRM_XE_MMIO_8BIT:
-			return -EINVAL; /* TODO */
-		case DRM_XE_MMIO_16BIT:
-			return -EINVAL; /* TODO */
-		case DRM_XE_MMIO_32BIT:
-			if (XE_IOCTL_ERR(xe, args->value > U32_MAX))
-				return -EINVAL;
-			xe_mmio_write32(to_gt(xe), args->addr, args->value);
-			break;
-		case DRM_XE_MMIO_64BIT:
-			xe_mmio_write64(to_gt(xe), args->addr, args->value);
-			break;
-		default:
-			drm_WARN(&xe->drm, 1, "Invalid MMIO bit size");
-			ret = -EINVAL;
+		ret = mmio_ioctl_write(xe, args);
+		if (ret)
  			goto exit;
-		}
  	}
  
-	if (args->flags & DRM_XE_MMIO_READ) {
-		switch (bits_flag) {
-		case DRM_XE_MMIO_8BIT:
-			return -EINVAL; /* TODO */
-		case DRM_XE_MMIO_16BIT:
-			return -EINVAL; /* TODO */
-		case DRM_XE_MMIO_32BIT:
-			args->value = xe_mmio_read32(to_gt(xe), args->addr);
-			break;
-		case DRM_XE_MMIO_64BIT:
-			args->value = xe_mmio_read64(to_gt(xe), args->addr);
-			break;
-		default:
-			drm_WARN(&xe->drm, 1, "Invalid MMIO bit size");
-			ret = -EINVAL;
-		}
-	}
+	if (args->flags & DRM_XE_MMIO_READ)
+		ret = mmio_ioctl_read(xe, args);
  
  exit:
  	xe_force_wake_put(gt_to_fw(&xe->gt[0]), XE_FORCEWAKE_ALL);
-----------------8<----------------

Lucas De Marchi