[PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

A struct file is a bit too large to put on the kernel stack in general
and triggers a warning for low settings of CONFIG_FRAME_WARN:

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=]

It's also slightly dangerous to leave a reference to a stack object
in the drm_file structure after leaving the stack frame.
This changes the code to just allocate the object dynamically
and release it when we are done with it.

Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
index 113dec05c7dc..18514065c93d 100644
--- a/drivers/gpu/drm/i915/selftests/mock_drm.c
+++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
@@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
 struct drm_file *mock_file(struct drm_i915_private *i915)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = {};
+	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
 	struct drm_file *file;
 	int err;
 
-	err = drm_open(&inode, &filp);
+	err = drm_open(&inode, filp);
 	if (unlikely(err))
 		return ERR_PTR(err);
 
-	file = filp.private_data;
+	file = filp->private_data;
 	file->authenticated = true;
 	return file;
 }
@@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
 void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = { .private_data = file };
+	struct file *filp = file->filp;
 
-	drm_release(&inode, &filp);
+	drm_release(&inode, filp);
+
+	kfree(filp);
 }
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

A struct file is a bit too large to put on the kernel stack in general
and triggers a warning for low settings of CONFIG_FRAME_WARN:

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=]

It's also slightly dangerous to leave a reference to a stack object
in the drm_file structure after leaving the stack frame.
This changes the code to just allocate the object dynamically
and release it when we are done with it.

Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
index 113dec05c7dc..18514065c93d 100644
--- a/drivers/gpu/drm/i915/selftests/mock_drm.c
+++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
@@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
 struct drm_file *mock_file(struct drm_i915_private *i915)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = {};
+	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
 	struct drm_file *file;
 	int err;
 
-	err = drm_open(&inode, &filp);
+	err = drm_open(&inode, filp);
 	if (unlikely(err))
 		return ERR_PTR(err);
 
-	file = filp.private_data;
+	file = filp->private_data;
 	file->authenticated = true;
 	return file;
 }
@@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
 void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
 {
 	struct inode inode = fake_inode(i915);
-	struct file filp = { .private_data = file };
+	struct file *filp = file->filp;
 
-	drm_release(&inode, &filp);
+	drm_release(&inode, filp);
+
+	kfree(filp);
 }
-- 
2.9.0

[PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Arnd Bergmann]

We get a warning with gcc-7 about a pointless comparison when
using a linear memmap:

drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]

Splitting out the comparison into a separate function avoids the warning
and makes it slightly more obvious what happens.

Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/selftests/scatterlist.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/selftests/scatterlist.c b/drivers/gpu/drm/i915/selftests/scatterlist.c
index eb2cda8e2b9f..c072b0f9180b 100644
--- a/drivers/gpu/drm/i915/selftests/scatterlist.c
+++ b/drivers/gpu/drm/i915/selftests/scatterlist.c
@@ -189,6 +189,11 @@ static unsigned int random(unsigned long n,
 	return 1 + (prandom_u32_state(rnd) % 1024);
 }
 
+static inline bool page_contiguous(struct page *first, struct page *last,  unsigned long npages)
+{
+	return first + npages == last;
+}
+
 static int alloc_table(struct pfn_table *pt,
 		       unsigned long count, unsigned long max,
 		       npages_fn_t npages_fn,
@@ -216,7 +221,8 @@ static int alloc_table(struct pfn_table *pt,
 		unsigned long npages = npages_fn(n, count, rnd);
 
 		/* Nobody expects the Sparse Memmap! */
-		if (pfn_to_page(pfn + npages) != pfn_to_page(pfn) + npages) {
+		if (!page_contiguous(pfn_to_page(pfn),
+				     pfn_to_page(pfn + npages), npages)) {
 			sg_free_table(&pt->st);
 			return -ENOSPC;
 		}
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Arnd Bergmann]

We get a warning with gcc-7 about a pointless comparison when
using a linear memmap:

drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]

Splitting out the comparison into a separate function avoids the warning
and makes it slightly more obvious what happens.

Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/selftests/scatterlist.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/selftests/scatterlist.c b/drivers/gpu/drm/i915/selftests/scatterlist.c
index eb2cda8e2b9f..c072b0f9180b 100644
--- a/drivers/gpu/drm/i915/selftests/scatterlist.c
+++ b/drivers/gpu/drm/i915/selftests/scatterlist.c
@@ -189,6 +189,11 @@ static unsigned int random(unsigned long n,
 	return 1 + (prandom_u32_state(rnd) % 1024);
 }
 
+static inline bool page_contiguous(struct page *first, struct page *last,  unsigned long npages)
+{
+	return first + npages == last;
+}
+
 static int alloc_table(struct pfn_table *pt,
 		       unsigned long count, unsigned long max,
 		       npages_fn_t npages_fn,
@@ -216,7 +221,8 @@ static int alloc_table(struct pfn_table *pt,
 		unsigned long npages = npages_fn(n, count, rnd);
 
 		/* Nobody expects the Sparse Memmap! */
-		if (pfn_to_page(pfn + npages) != pfn_to_page(pfn) + npages) {
+		if (!page_contiguous(pfn_to_page(pfn),
+				     pfn_to_page(pfn + npages), npages)) {
 			sg_free_table(&pt->st);
 			return -ENOSPC;
 		}
-- 
2.9.0

Re: [PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Chris Wilson]

On Mon, Mar 20, 2017 at 10:40:28AM +0100, Arnd Bergmann wrote:
> We get a warning with gcc-7 about a pointless comparison when
> using a linear memmap:
> 
> drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
> drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]
> 
> Splitting out the comparison into a separate function avoids the warning
> and makes it slightly more obvious what happens.

I worried whether gcc will simply get smarter and apply its warning more
consistently, but I agree the helper is good documentation.
 
> Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Chris Wilson]

On Mon, Mar 20, 2017 at 10:40:28AM +0100, Arnd Bergmann wrote:
> We get a warning with gcc-7 about a pointless comparison when
> using a linear memmap:
> 
> drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
> drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]
> 
> Splitting out the comparison into a separate function avoids the warning
> and makes it slightly more obvious what happens.

I worried whether gcc will simply get smarter and apply its warning more
consistently, but I agree the helper is good documentation.
 
> Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Chris Wilson]

On Tue, Mar 21, 2017 at 09:56:52AM +0000, Chris Wilson wrote:
> On Mon, Mar 20, 2017 at 10:40:28AM +0100, Arnd Bergmann wrote:
> > We get a warning with gcc-7 about a pointless comparison when
> > using a linear memmap:
> > 
> > drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
> > drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]
> > 
> > Splitting out the comparison into a separate function avoids the warning
> > and makes it slightly more obvious what happens.
> 
> I worried whether gcc will simply get smarter and apply its warning more
> consistently, but I agree the helper is good documentation.
>  
> > Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And applied.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm/i915: split out check for noncontiguous pfn range

[Chris Wilson]

On Tue, Mar 21, 2017 at 09:56:52AM +0000, Chris Wilson wrote:
> On Mon, Mar 20, 2017 at 10:40:28AM +0100, Arnd Bergmann wrote:
> > We get a warning with gcc-7 about a pointless comparison when
> > using a linear memmap:
> > 
> > drivers/gpu/drm/i915/selftests/scatterlist.c: In function 'alloc_table':
> > drivers/gpu/drm/i915/selftests/scatterlist.c:219:66: error: self-comparison always evaluates to false [-Werror=tautological-compare]
> > 
> > Splitting out the comparison into a separate function avoids the warning
> > and makes it slightly more obvious what happens.
> 
> I worried whether gcc will simply get smarter and apply its warning more
> consistently, but I agree the helper is good documentation.
>  
> > Fixes: 935a2f776aa5 ("drm/i915: Add some selftests for sg_table manipulation")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

And applied.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Arnd Bergmann]

The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
to shrink the i915 kernel module by around 1000 bytes. However, the
downside is a size regression with CONFIG_KASAN, as I found from stack size
warnings with gcc-7.0.1:

before:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]

after:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]

I also checked the module sizes and got

before:
   text	   data	    bss	    dec	    hex	filename
2704592	 412025	  11104	3127721	 2fb9a9	drivers/gpu/drm/i915/i915.o

after:
   text	   data	    bss	    dec	    hex	filename
2718538	 412025	  11104	3141667	 2ff023	drivers/gpu/drm/i915/i915.o

showing a significant code size growth. This reverts the patch to avoid
the warning and get back to the better code with CONFIG_KASAN. If someone
has another idea to avoid the warning while keeping the more efficient
code for the non-KASAN case, that would obviously be better.

Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/i915_reg.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..aa732eccdeb5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,8 +48,6 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
 }
 
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
-
 #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
 #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
 #define _PLANE(plane, a, b) _PIPE(plane, a, b)
@@ -58,11 +56,14 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
 #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
 #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
-#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
+#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
+			       (pipe) == PIPE_B ? (b) : (c))
 #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
-#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
+#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
+			       (port) == PORT_B ? (b) : (c))
 #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
-#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
+#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
+			     (phy) == DPIO_PHY1 ? (b) : (c))
 #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
 
 #define _MASKED_FIELD(mask, value) ({					   \
-- 
2.9.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Arnd Bergmann]

The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
to shrink the i915 kernel module by around 1000 bytes. However, the
downside is a size regression with CONFIG_KASAN, as I found from stack size
warnings with gcc-7.0.1:

before:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]

after:
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]

I also checked the module sizes and got

before:
   text	   data	    bss	    dec	    hex	filename
2704592	 412025	  11104	3127721	 2fb9a9	drivers/gpu/drm/i915/i915.o

after:
   text	   data	    bss	    dec	    hex	filename
2718538	 412025	  11104	3141667	 2ff023	drivers/gpu/drm/i915/i915.o

showing a significant code size growth. This reverts the patch to avoid
the warning and get back to the better code with CONFIG_KASAN. If someone
has another idea to avoid the warning while keeping the more efficient
code for the non-KASAN case, that would obviously be better.

Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/i915_reg.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 04c8f69fcc62..aa732eccdeb5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -48,8 +48,6 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
 }
 
-#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
-
 #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
 #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
 #define _PLANE(plane, a, b) _PIPE(plane, a, b)
@@ -58,11 +56,14 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
 #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
 #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
-#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
+#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
+			       (pipe) == PIPE_B ? (b) : (c))
 #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
-#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
+#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
+			       (port) == PORT_B ? (b) : (c))
 #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
-#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
+#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
+			     (phy) == DPIO_PHY1 ? (b) : (c))
 #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
 
 #define _MASKED_FIELD(mask, value) ({					   \
-- 
2.9.0

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Jani Nikula]

On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> to shrink the i915 kernel module by around 1000 bytes.

To be clear, it was not at all intended to be an optimization, nothing
of the sort. The intention was to make it easier and less error prone to
add more parameters to said macros. The text size shring was just a
bonus.

> However, the
> downside is a size regression with CONFIG_KASAN, as I found from stack size
> warnings with gcc-7.0.1:

In his review of the original change, Chris provided this comparison
https://godbolt.org/g/YCK1od

How does CONFIG_KASAN change this? Would be nice to see how the
generated code blows up.


BR,
Jani.


>
> before:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> after:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
>
> I also checked the module sizes and got
>
> before:
>    text	   data	    bss	    dec	    hex	filename
> 2704592	 412025	  11104	3127721	 2fb9a9	drivers/gpu/drm/i915/i915.o
>
> after:
>    text	   data	    bss	    dec	    hex	filename
> 2718538	 412025	  11104	3141667	 2ff023	drivers/gpu/drm/i915/i915.o
>
> showing a significant code size growth. This reverts the patch to avoid
> the warning and get back to the better code with CONFIG_KASAN. If someone
> has another idea to avoid the warning while keeping the more efficient
> code for the non-KASAN case, that would obviously be better.
>
> Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..aa732eccdeb5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,8 +48,6 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>  }
>  
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> -
>  #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
>  #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
>  #define _PLANE(plane, a, b) _PIPE(plane, a, b)
> @@ -58,11 +56,14 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
>  #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
>  #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
> -#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
> +#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
> +			       (pipe) == PIPE_B ? (b) : (c))
>  #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
> -#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
> +#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
> +			       (port) == PORT_B ? (b) : (c))
>  #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
> -#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
> +#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
> +			     (phy) == DPIO_PHY1 ? (b) : (c))
>  #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
>  
>  #define _MASKED_FIELD(mask, value) ({					   \

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Jani Nikula]

On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
> to shrink the i915 kernel module by around 1000 bytes.

To be clear, it was not at all intended to be an optimization, nothing
of the sort. The intention was to make it easier and less error prone to
add more parameters to said macros. The text size shring was just a
bonus.

> However, the
> downside is a size regression with CONFIG_KASAN, as I found from stack size
> warnings with gcc-7.0.1:

In his review of the original change, Chris provided this comparison
https://godbolt.org/g/YCK1od

How does CONFIG_KASAN change this? Would be nice to see how the
generated code blows up.


BR,
Jani.


>
> before:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 176 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 224 bytes is larger than 100 bytes [-Werror=frame-larger-than=]
>
> after:
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_get_hw_state':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1644:1: error: the frame size of 1016 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
> drivers/gpu/drm/i915/intel_dpll_mgr.c: In function 'bxt_ddi_pll_enable':
> drivers/gpu/drm/i915/intel_dpll_mgr.c:1548:1: error: the frame size of 1960 bytes is larger than 1000 bytes [-Werror=frame-larger-than=]
>
> I also checked the module sizes and got
>
> before:
>    text	   data	    bss	    dec	    hex	filename
> 2704592	 412025	  11104	3127721	 2fb9a9	drivers/gpu/drm/i915/i915.o
>
> after:
>    text	   data	    bss	    dec	    hex	filename
> 2718538	 412025	  11104	3141667	 2ff023	drivers/gpu/drm/i915/i915.o
>
> showing a significant code size growth. This reverts the patch to avoid
> the warning and get back to the better code with CONFIG_KASAN. If someone
> has another idea to avoid the warning while keeping the more efficient
> code for the non-KASAN case, that would obviously be better.
>
> Fixes: ce64645d86ac ("drm/i915: use variadic macros and arrays to choose port/pipe based registers")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 04c8f69fcc62..aa732eccdeb5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -48,8 +48,6 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  	return !i915_mmio_reg_equal(reg, INVALID_MMIO_REG);
>  }
>  
> -#define _PICK(__index, ...) (((const u32 []){ __VA_ARGS__ })[__index])
> -
>  #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
>  #define _MMIO_PIPE(pipe, a, b) _MMIO(_PIPE(pipe, a, b))
>  #define _PLANE(plane, a, b) _PIPE(plane, a, b)
> @@ -58,11 +56,14 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define _MMIO_TRANS(tran, a, b) _MMIO(_TRANS(tran, a, b))
>  #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
>  #define _MMIO_PORT(port, a, b) _MMIO(_PORT(port, a, b))
> -#define _PIPE3(pipe, ...) _PICK(pipe, __VA_ARGS__)
> +#define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
> +			       (pipe) == PIPE_B ? (b) : (c))
>  #define _MMIO_PIPE3(pipe, a, b, c) _MMIO(_PIPE3(pipe, a, b, c))
> -#define _PORT3(port, ...) _PICK(port, __VA_ARGS__)
> +#define _PORT3(port, a, b, c) ((port) == PORT_A ? (a) : \
> +			       (port) == PORT_B ? (b) : (c))
>  #define _MMIO_PORT3(pipe, a, b, c) _MMIO(_PORT3(pipe, a, b, c))
> -#define _PHY3(phy, ...) _PICK(phy, __VA_ARGS__)
> +#define _PHY3(phy, a, b, c) ((phy) == DPIO_PHY0 ? (a) : \
> +			     (phy) == DPIO_PHY1 ? (b) : (c))
>  #define _MMIO_PHY3(phy, a, b, c) _MMIO(_PHY3(phy, a, b, c))
>  
>  #define _MASKED_FIELD(mask, value) ({					   \

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 11:08 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> to shrink the i915 kernel module by around 1000 bytes.
>
> To be clear, it was not at all intended to be an optimization, nothing
> of the sort. The intention was to make it easier and less error prone to
> add more parameters to said macros. The text size shring was just a
> bonus.
>
>> However, the
>> downside is a size regression with CONFIG_KASAN, as I found from stack size
>> warnings with gcc-7.0.1:
>
> In his review of the original change, Chris provided this comparison
> https://godbolt.org/g/YCK1od
>
> How does CONFIG_KASAN change this? Would be nice to see how the
> generated code blows up.
>
I don't know how to generate a URL for it, but after adding this to the
command line for gcc-7,

-fsanitize=kernel-address -fasan-shadow-offset=0xdfff900000000000
--param asan-stack=1 --param asan-globals=1 --param
asan-instrumentation-with-call-threshold=10000
-fsanitize-address-use-after-scope

the code turned from really nice into the log series of checks below.
Without  -fsanitize-address-use-after-scope (which didn't exist before gcc-7),
it's less bad but still exceeds the (arbitrary) 1536 byte limit.

     Arnd

.LC0:
        .string "2 32 4 1 i 96 24 9 <unknown> "
main:
.LASANPC0:
        pushq   %r12
        pushq   %rbp
        movabsq $-2305966154516004864, %rdx
        pushq   %rbx
        subq    $160, %rsp
        movq    %rsp, %rbp
        leaq    96(%rsp), %rbx
        movq    $1102416563, (%rsp)
        shrq    $3, %rbp
        movq    $.LC0, 8(%rsp)
        movq    $.LASANPC0, 16(%rsp)
        leaq    0(%rbp,%rdx), %rax
        movl    $-235802127, (%rax)
        movl    $-218959356, 4(%rax)
        movl    $-218959118, 8(%rax)
        movl    $-234881024, 12(%rax)
        movl    $-202116109, 16(%rax)
        movq    %rbx, %rax
        movl    $1, 32(%rsp)
        shrq    $3, %rax
        movzbl  (%rax,%rdx), %eax
        testb   %al, %al
        je      .L2
        cmpb    $3, %al
        jle     .L53
.L2:
        leaq    4(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $0, 96(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L3
        testb   %dl, %dl
        jne     .L54
.L3:
        leaq    8(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $1, 100(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %eax
        testb   %al, %al
        je      .L4
        cmpb    $3, %al
        jle     .L55
.L4:
        leaq    12(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $2, 104(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L5
        testb   %dl, %dl
        jne     .L56
.L5:
        leaq    16(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $3, 108(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %eax
        testb   %al, %al
        je      .L6
        cmpb    $3, %al
        jle     .L57
.L6:
        leaq    20(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $4, 112(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L7
        testb   %dl, %dl
        jne     .L58
.L7:
        movslq  32(%rsp), %r12
        movabsq $-2305966154516004864, %rax
        movl    $5, 116(%rsp)
        leaq    (%rbx,%r12,4), %rdi
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L8
        testb   %dl, %dl
        jne     .L59
.L8:
        pxor    %xmm0, %xmm0
        movabsq $-2305966154516004864, %rdx
        movl    96(%rsp,%r12,4), %eax
        movl    $0, 16(%rdx,%rbp)
        movups  %xmm0, 0(%rbp,%rdx)
        addq    $160, %rsp
        popq    %rbx
        popq    %rbp
        popq    %r12
        ret
.L59:
        call    __asan_report_load4_noabort
        jmp     .L8
.L58:
        call    __asan_report_store4_noabort
        jmp     .L7
.L57:
        call    __asan_report_store4_noabort
        jmp     .L6
.L56:
        call    __asan_report_store4_noabort
        jmp     .L5
.L55:
        call    __asan_report_store4_noabort
        jmp     .L4
.L54:
        call    __asan_report_store4_noabort
        jmp     .L3
.L53:
        movq    %rbx, %rdi
        call    __asan_report_store4_noabort
        jmp     .L2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 11:08 AM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
> On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
>> The varargs macro trick in _PIPE3/_PHY3/_PORT3 was meant as an optimization
>> to shrink the i915 kernel module by around 1000 bytes.
>
> To be clear, it was not at all intended to be an optimization, nothing
> of the sort. The intention was to make it easier and less error prone to
> add more parameters to said macros. The text size shring was just a
> bonus.
>
>> However, the
>> downside is a size regression with CONFIG_KASAN, as I found from stack size
>> warnings with gcc-7.0.1:
>
> In his review of the original change, Chris provided this comparison
> https://godbolt.org/g/YCK1od
>
> How does CONFIG_KASAN change this? Would be nice to see how the
> generated code blows up.
>
I don't know how to generate a URL for it, but after adding this to the
command line for gcc-7,

-fsanitize=kernel-address -fasan-shadow-offset=0xdfff900000000000
--param asan-stack=1 --param asan-globals=1 --param
asan-instrumentation-with-call-threshold=10000
-fsanitize-address-use-after-scope

the code turned from really nice into the log series of checks below.
Without  -fsanitize-address-use-after-scope (which didn't exist before gcc-7),
it's less bad but still exceeds the (arbitrary) 1536 byte limit.

     Arnd

.LC0:
        .string "2 32 4 1 i 96 24 9 <unknown> "
main:
.LASANPC0:
        pushq   %r12
        pushq   %rbp
        movabsq $-2305966154516004864, %rdx
        pushq   %rbx
        subq    $160, %rsp
        movq    %rsp, %rbp
        leaq    96(%rsp), %rbx
        movq    $1102416563, (%rsp)
        shrq    $3, %rbp
        movq    $.LC0, 8(%rsp)
        movq    $.LASANPC0, 16(%rsp)
        leaq    0(%rbp,%rdx), %rax
        movl    $-235802127, (%rax)
        movl    $-218959356, 4(%rax)
        movl    $-218959118, 8(%rax)
        movl    $-234881024, 12(%rax)
        movl    $-202116109, 16(%rax)
        movq    %rbx, %rax
        movl    $1, 32(%rsp)
        shrq    $3, %rax
        movzbl  (%rax,%rdx), %eax
        testb   %al, %al
        je      .L2
        cmpb    $3, %al
        jle     .L53
.L2:
        leaq    4(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $0, 96(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L3
        testb   %dl, %dl
        jne     .L54
.L3:
        leaq    8(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $1, 100(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %eax
        testb   %al, %al
        je      .L4
        cmpb    $3, %al
        jle     .L55
.L4:
        leaq    12(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $2, 104(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L5
        testb   %dl, %dl
        jne     .L56
.L5:
        leaq    16(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $3, 108(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %eax
        testb   %al, %al
        je      .L6
        cmpb    $3, %al
        jle     .L57
.L6:
        leaq    20(%rbx), %rdi
        movabsq $-2305966154516004864, %rax
        movl    $4, 112(%rsp)
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L7
        testb   %dl, %dl
        jne     .L58
.L7:
        movslq  32(%rsp), %r12
        movabsq $-2305966154516004864, %rax
        movl    $5, 116(%rsp)
        leaq    (%rbx,%r12,4), %rdi
        movq    %rdi, %rdx
        shrq    $3, %rdx
        movzbl  (%rdx,%rax), %edx
        movq    %rdi, %rax
        andl    $7, %eax
        addl    $3, %eax
        cmpb    %dl, %al
        jl      .L8
        testb   %dl, %dl
        jne     .L59
.L8:
        pxor    %xmm0, %xmm0
        movabsq $-2305966154516004864, %rdx
        movl    96(%rsp,%r12,4), %eax
        movl    $0, 16(%rdx,%rbp)
        movups  %xmm0, 0(%rbp,%rdx)
        addq    $160, %rsp
        popq    %rbx
        popq    %rbp
        popq    %r12
        ret
.L59:
        call    __asan_report_load4_noabort
        jmp     .L8
.L58:
        call    __asan_report_store4_noabort
        jmp     .L7
.L57:
        call    __asan_report_store4_noabort
        jmp     .L6
.L56:
        call    __asan_report_store4_noabort
        jmp     .L5
.L55:
        call    __asan_report_store4_noabort
        jmp     .L4
.L54:
        call    __asan_report_store4_noabort
        jmp     .L3
.L53:
        movq    %rbx, %rdi
        call    __asan_report_store4_noabort
        jmp     .L2

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Jani Nikula]

On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> I don't know how to generate a URL for it, but after adding this to the
> command line for gcc-7,
>
> -fsanitize=kernel-address -fasan-shadow-offset=0xdfff900000000000
> --param asan-stack=1 --param asan-globals=1 --param
> asan-instrumentation-with-call-threshold=10000
> -fsanitize-address-use-after-scope
>
> the code turned from really nice into the log series of checks below.
> Without  -fsanitize-address-use-after-scope (which didn't exist before gcc-7),
> it's less bad but still exceeds the (arbitrary) 1536 byte limit.

It seems to be the combination of --param asan-stack=1 and
-fsanitize-address-use-after-scope that really blows up the code [1]. I
filed a GCC bug on it, mostly to see what they say [2]. I don't know,
maybe they think it's expected. *shrug*.


BR,
Jani.

[1] https://godbolt.org/g/hgS817
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] [RFC] Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"

[Jani Nikula]

On Mon, 20 Mar 2017, Arnd Bergmann <arnd@arndb.de> wrote:
> I don't know how to generate a URL for it, but after adding this to the
> command line for gcc-7,
>
> -fsanitize=kernel-address -fasan-shadow-offset=0xdfff900000000000
> --param asan-stack=1 --param asan-globals=1 --param
> asan-instrumentation-with-call-threshold=10000
> -fsanitize-address-use-after-scope
>
> the code turned from really nice into the log series of checks below.
> Without  -fsanitize-address-use-after-scope (which didn't exist before gcc-7),
> it's less bad but still exceeds the (arbitrary) 1536 byte limit.

It seems to be the combination of --param asan-stack=1 and
-fsanitize-address-use-after-scope that really blows up the code [1]. I
filed a GCC bug on it, mostly to see what they say [2]. I don't know,
maybe they think it's expected. *shrug*.


BR,
Jani.

[1] https://godbolt.org/g/hgS817
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80114


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Daniel Vetter]

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);

Aside: We should have a FIXME and/or merge David Herrmanns code to allow a
drm_file without a real struct file for kernel-internal use ...
-Daniel

>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;
>  	file->authenticated = true;
>  	return file;
>  }
> @@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
>  void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = { .private_data = file };
> +	struct file *filp = file->filp;
>  
> -	drm_release(&inode, &filp);
> +	drm_release(&inode, filp);
> +
> +	kfree(filp);
>  }
> -- 
> 2.9.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Daniel Vetter]

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);

Aside: We should have a FIXME and/or merge David Herrmanns code to allow a
drm_file without a real struct file for kernel-internal use ...
-Daniel

>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;
>  	file->authenticated = true;
>  	return file;
>  }
> @@ -48,7 +48,9 @@ struct drm_file *mock_file(struct drm_i915_private *i915)
>  void mock_file_free(struct drm_i915_private *i915, struct drm_file *file)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = { .private_data = file };
> +	struct file *filp = file->filp;
>  
> -	drm_release(&inode, &filp);
> +	drm_release(&inode, filp);
> +
> +	kfree(filp);
>  }
> -- 
> 2.9.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: allocate mock file pointer dynamically

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: allocate mock file pointer dynamically
URL   : https://patchwork.freedesktop.org/series/21533/
State : success

== Summary ==

Series 21533v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21533/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                dmesg-warn -> PASS       (fi-bxt-t5700) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 463s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 534s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 559s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 499s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 496s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 434s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 433s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 515s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 495s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 478s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 480s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 591s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 477s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 519s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 553s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 421s

6aefe8d3baa2d94246dd20ba33c77758b6de9ccc drm-tip: 2017y-03m-20d-07h-21m-17s UTC integration manifest
7c8d5c6 Revert "drm/i915: use variadic macros and arrays to choose port/pipe based registers"
5530a66 drm/i915: split out check for noncontiguous pfn range
5b90d89 drm/i915: allocate mock file pointer dynamically

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4227/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Joonas Lahtinen]

On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

<SNIP>

> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
> >  	struct inode inode = fake_inode(i915);
> > -	struct file filp = {};
> > +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
> >  	struct drm_file *file;
> >  	int err;
>  

	filp = kzalloc(sizeof(*filp), GFP_KERNEL);
	if (unlikely(!filp)) {
		err = -ENOMEM;
		goto err;
	}

And appropriate onion teardown in case drm_open fails, so that we don't
leak memory.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Joonas Lahtinen]

On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

<SNIP>

> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
> >  	struct inode inode = fake_inode(i915);
> > -	struct file filp = {};
> > +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
> >  	struct drm_file *file;
> >  	int err;
>  

	filp = kzalloc(sizeof(*filp), GFP_KERNEL);
	if (unlikely(!filp)) {
		err = -ENOMEM;
		goto err;
	}

And appropriate onion teardown in case drm_open fails, so that we don't
leak memory.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:

>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> index 113dec05c7dc..18514065c93d 100644
>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>  {
>> >     struct inode inode = fake_inode(i915);
>> > -   struct file filp = {};
>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>> >     struct drm_file *file;
>> >     int err;
>>
>
>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>         if (unlikely(!filp)) {
>                 err = -ENOMEM;
>                 goto err;
>         }
>
> And appropriate onion teardown in case drm_open fails, so that we don't
> leak memory.

Oops, of course you are right, sorry about that.

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

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:

>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> index 113dec05c7dc..18514065c93d 100644
>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>  {
>> >     struct inode inode = fake_inode(i915);
>> > -   struct file filp = {};
>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>> >     struct drm_file *file;
>> >     int err;
>>
>
>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>         if (unlikely(!filp)) {
>                 err = -ENOMEM;
>                 goto err;
>         }
>
> And appropriate onion teardown in case drm_open fails, so that we don't
> leak memory.

Oops, of course you are right, sorry about that.

    Arnd

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 1:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
>> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
>
>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> index 113dec05c7dc..18514065c93d 100644
>>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>>  {
>>> >     struct inode inode = fake_inode(i915);
>>> > -   struct file filp = {};
>>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>>> >     struct drm_file *file;
>>> >     int err;
>>>
>>
>>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>>         if (unlikely(!filp)) {
>>                 err = -ENOMEM;
>>                 goto err;
>>         }
>>
>> And appropriate onion teardown in case drm_open fails, so that we don't
>> leak memory.
>
> Oops, of course you are right, sorry about that.

Actually it's even worse, as I had originally planned to also allocate the inode
dynamically, but for some reasons didn't do that in the patch I sent.

This version of the patch as I sent it is rather bogus, so please ignore it
(but not the other two patches I sent). If David's solution to this problem
can make it into 4.12, there is no problem at all. Another alternative
would be to use anon_inode_getfile(), again with proper error handling
and cleanup. This is a bit slower but gives us valid file and inode structures.

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

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Arnd Bergmann]

On Mon, Mar 20, 2017 at 1:02 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, Mar 20, 2017 at 1:00 PM, Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com> wrote:
>> On ma, 2017-03-20 at 10:40 +0100, Arnd Bergmann wrote:
>
>>> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> index 113dec05c7dc..18514065c93d 100644
>>> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
>>> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>>>  struct drm_file *mock_file(struct drm_i915_private *i915)
>>>  {
>>> >     struct inode inode = fake_inode(i915);
>>> > -   struct file filp = {};
>>> > +   struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>>> >     struct drm_file *file;
>>> >     int err;
>>>
>>
>>         filp = kzalloc(sizeof(*filp), GFP_KERNEL);
>>         if (unlikely(!filp)) {
>>                 err = -ENOMEM;
>>                 goto err;
>>         }
>>
>> And appropriate onion teardown in case drm_open fails, so that we don't
>> leak memory.
>
> Oops, of course you are right, sorry about that.

Actually it's even worse, as I had originally planned to also allocate the inode
dynamically, but for some reasons didn't do that in the patch I sent.

This version of the patch as I sent it is rather bogus, so please ignore it
(but not the other two patches I sent). If David's solution to this problem
can make it into 4.12, there is no problem at all. Another alternative
would be to use anon_inode_getfile(), again with proper error handling
and cleanup. This is a bit slower but gives us valid file and inode structures.

     Arnd

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Chris Wilson]

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);
>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;

What I don't like about this approach is that we leak the unwanted
inode/file. We only want the drm_file here and any access to above that
inside i915.ko is fubar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915: allocate mock file pointer dynamically

[Chris Wilson]

On Mon, Mar 20, 2017 at 10:40:27AM +0100, Arnd Bergmann wrote:
> A struct file is a bit too large to put on the kernel stack in general
> and triggers a warning for low settings of CONFIG_FRAME_WARN:
> 
> 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=]
> 
> It's also slightly dangerous to leave a reference to a stack object
> in the drm_file structure after leaving the stack frame.
> This changes the code to just allocate the object dynamically
> and release it when we are done with it.
> 
> Fixes: 66d9cb5d805a ("drm/i915: Mock the GEM device for self-testing")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/selftests/mock_drm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/selftests/mock_drm.c b/drivers/gpu/drm/i915/selftests/mock_drm.c
> index 113dec05c7dc..18514065c93d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_drm.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_drm.c
> @@ -32,15 +32,15 @@ static inline struct inode fake_inode(struct drm_i915_private *i915)
>  struct drm_file *mock_file(struct drm_i915_private *i915)
>  {
>  	struct inode inode = fake_inode(i915);
> -	struct file filp = {};
> +	struct file *filp = kzalloc(sizeof(struct file), GFP_KERNEL);
>  	struct drm_file *file;
>  	int err;
>  
> -	err = drm_open(&inode, &filp);
> +	err = drm_open(&inode, filp);
>  	if (unlikely(err))
>  		return ERR_PTR(err);
>  
> -	file = filp.private_data;
> +	file = filp->private_data;

What I don't like about this approach is that we leak the unwanted
inode/file. We only want the drm_file here and any access to above that
inside i915.ko is fubar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre