Re: [PATCH] drm/i915/selftests: Allocate inode/file dynamically

[Arnd Bergmann <arnd@xxxxxxxx>]

On Tue, Apr 18, 2017 at 3:12 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> Avoid having too large a stack by creating the fake struct inode/file on
> the heap instead.
>
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file':
> drivers/gpu/drm/i915/selftests/mock_drm.c:46:1: error: the frame size of 1328 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/selftests/mock_drm.c: In function 'mock_file_free':
> drivers/gpu/drm/i915/selftests/mock_drm.c:54:1: error: the frame size of 1312 bytes is larger than 1280 bytes [-Werror=frame-larger-than=]
>
> Reported-by: Arnd Bergmann <arnd@xxxxxxxx>
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>

This is clearly an improvement over the current state and it gets rid of
the warning, so I'm fine with this getting merged.

Acked-by: Arnd Bergmann <arnd@xxxxxxxx>

A nicer solution might be a wrapper around drm_open_helper() that
does not require a file pointer like this (completely untested, probably
wrong) patch would:

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 3783b659cd38..4956c3945e7e 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -189,25 +189,38 @@ static int drm_cpu_valid(void)
  */
 static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 {
- struct drm_device *dev = minor->dev;
  struct drm_file *priv;
  int ret;

  if (filp->f_flags & O_EXCL)
  return -EBUSY; /* No exclusive opens */
+
+ priv = drm_open_dev(minor);
+ ret = PTR_ERR_OR_ZERO(priv);
+ if (ret)
+ return ret;
+
+ filp->private_data = priv;
+ priv->filp = filp;
+}
+
+struct drm_file *drm_open_dev(struct drm_minor *minor)
+{
+ struct drm_device *dev = minor->dev;
+ struct drm_file *priv;
+ int ret;
+
  if (!drm_cpu_valid())
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
  if (dev->switch_power_state != DRM_SWITCH_POWER_ON &&
dev->switch_power_state != DRM_SWITCH_POWER_DYNAMIC_OFF)
- return -EINVAL;
+ return ERR_PTR(-EINVAL);

  DRM_DEBUG("pid = %d, minor = %d\n", task_pid_nr(current), minor->index);

  priv = kzalloc(sizeof(*priv), GFP_KERNEL);
  if (!priv)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);

- filp->private_data = priv;
- priv->filp = filp;
  priv->pid = get_pid(task_pid(current));
  priv->minor = minor;

@@ -268,7 +281,7 @@ static int drm_open_helper(struct file *filp,
struct drm_minor *minor)
  }
 #endif

- return 0;
+ return priv;

 out_close:
  if (dev->driver->postclose)
@@ -280,8 +293,7 @@ static int drm_open_helper(struct file *filp,
struct drm_minor *minor)
  drm_gem_release(dev, priv);
  put_pid(priv->pid);
  kfree(priv);
- filp->private_data = NULL;
- return ret;
+ return ERR_PTR(ret);
 }

 static void drm_events_release(struct drm_file *file_priv)

If anyone cares more about that than I do, this could be a follow-up patch.

   Arnd
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx