[PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This reverts commit fa7d81bb3c269a2ee38b6e4d569d9eb8be1a78ad.

As Peter explained:
  [...] lockless_dereference() is _stronger_ than READ_ONCE(), not weaker.

  [...]

  Also, clue is in the name: 'dereference', you don't actually dereference
  the pointer here, only load it.

My next patch breaks compile on this, because it assumes you want to
deference and thus also need the struct type visible (which it isn't
here), so revert it.

Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ce54e985d91b..0a06f9120b5a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
 
 	/* Sometimes user space wants everything disabled, so don't steal the
 	 * display if there's a master. */
-	if (lockless_dereference(dev->master))
+	if (READ_ONCE(dev->master))
 		return false;
 
 	drm_for_each_crtc(crtc, dev) {
-- 
2.8.1

[PATCH 2/2] locking/barriers: suppress sparse warnings in lockless_dereference()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

After Peter's commit (see below) we get a lot of sparse warnings
(one for every rcu_dereference, and more) since the expression
here is assigning to the wrong address space.

Instead of validating that 'p' is a pointer this way, instead make
it fail compilation when it's not by using sizeof(*(p)). This will
not cause any sparse warnings (tested, likely since the address
space is irrelevant for sizeof), and will fail compilation when
'p' isn't a pointer type.

Fixes: 331b6d8c7afc ("locking/barriers: Validate lockless_dereference() is used on a pointer type")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/compiler.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 1bb954842725..436aa4e42221 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -527,13 +527,13 @@ static __always_inline void __write_once_size(volatile void *p, void *res, int s
  * object's lifetime is managed by something other than RCU.  That
  * "something other" might be reference counting or simple immortality.
  *
- * The seemingly unused void * variable is to validate @p is indeed a pointer
- * type. All pointer types silently cast to void *.
+ * The seemingly unused size_t variable is to validate @p is indeed a pointer
+ * type by making sure it can be dereferenced.
  */
 #define lockless_dereference(p) \
 ({ \
 	typeof(p) _________p1 = READ_ONCE(p); \
-	__maybe_unused const void * const _________p2 = _________p1; \
+	size_t __maybe_unused __size_of_ptr = sizeof(*(p)); \
 	smp_read_barrier_depends(); /* Dependency order vs. p above. */ \
 	(_________p1); \
 })
-- 
2.8.1

Re: [PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Daniel Vetter]

On Thu, Aug 11, 2016 at 11:50:21AM +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> This reverts commit fa7d81bb3c269a2ee38b6e4d569d9eb8be1a78ad.
> 
> As Peter explained:
>   [...] lockless_dereference() is _stronger_ than READ_ONCE(), not weaker.
> 
>   [...]
> 
>   Also, clue is in the name: 'dereference', you don't actually dereference
>   the pointer here, only load it.
> 
> My next patch breaks compile on this, because it assumes you want to
> deference and thus also need the struct type visible (which it isn't
> here), so revert it.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

And ack-by: me for merging through whatever tree this makes sense for.
-Daniel

> ---
>  drivers/gpu/drm/drm_fb_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ce54e985d91b..0a06f9120b5a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
>  
>  	/* Sometimes user space wants everything disabled, so don't steal the
>  	 * display if there's a master. */
> -	if (lockless_dereference(dev->master))
> +	if (READ_ONCE(dev->master))
>  		return false;
>  
>  	drm_for_each_crtc(crtc, dev) {
> -- 
> 2.8.1
> 
> _______________________________________________
> 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

✗ Ro.CI.BAT: failure for series starting with [1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Patchwork]

== Series Details ==

Series: series starting with [1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"
URL   : https://patchwork.freedesktop.org/series/10948/
State : failure

== Summary ==

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

Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> DMESG-WARN (fi-skl-i7-6700k)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-hsw-i7-4770r)
                fail       -> PASS       (ro-byt-n2820)
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-flip-vs-cursor-varying-size:
                fail       -> PASS       (fi-hsw-i7-4770k)
                fail       -> PASS       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i7-5557U)
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:244  pass:221  dwarn:0   dfail:0   fail:1   skip:22 
fi-kbl-qkkr      total:244  pass:185  dwarn:30  dfail:1   fail:1   skip:27 
fi-skl-i5-6260u  total:244  pass:224  dwarn:4   dfail:0   fail:2   skip:14 
fi-skl-i7-6700k  total:244  pass:208  dwarn:4   dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19 
ro-bdw-i7-5557U  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:199  dwarn:0   dfail:0   fail:1   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-ilk1-i5-650   total:235  pass:173  dwarn:0   dfail:0   fail:2   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1835/

3c6050a drm-intel-nightly: 2016y-08m-11d-10h-34m-38s UTC integration manifest
8ff8d08 locking/barriers: suppress sparse warnings in lockless_dereference()
fcc9432 Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

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

Re: [PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Paul E. McKenney]

On Thu, Aug 11, 2016 at 12:38:59PM +0200, Daniel Vetter wrote:
> On Thu, Aug 11, 2016 at 11:50:21AM +0200, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> > 
> > This reverts commit fa7d81bb3c269a2ee38b6e4d569d9eb8be1a78ad.
> > 
> > As Peter explained:
> >   [...] lockless_dereference() is _stronger_ than READ_ONCE(), not weaker.
> > 
> >   [...]
> > 
> >   Also, clue is in the name: 'dereference', you don't actually dereference
> >   the pointer here, only load it.
> > 
> > My next patch breaks compile on this, because it assumes you want to
> > deference and thus also need the struct type visible (which it isn't
> > here), so revert it.
> > 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> And ack-by: me for merging through whatever tree this makes sense for.
> -Daniel

Initial testing says that the change below must precede the change
to the definition of lockless_dereference(), so the two should go
together.

If my upcoming testing of the two changes together pans out, I will
give you a Tested-by -- I am guessing that you don't want to wait
until the next merge window for these changes.

							Thanx, Paul

> > ---
> >  drivers/gpu/drm/drm_fb_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> > index ce54e985d91b..0a06f9120b5a 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -464,7 +464,7 @@ static bool drm_fb_helper_is_bound(struct drm_fb_helper *fb_helper)
> >  
> >  	/* Sometimes user space wants everything disabled, so don't steal the
> >  	 * display if there's a master. */
> > -	if (lockless_dereference(dev->master))
> > +	if (READ_ONCE(dev->master))
> >  		return false;
> >  
> >  	drm_for_each_crtc(crtc, dev) {
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > 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
> 

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

Re: [PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Johannes Berg]

> Initial testing says that the change below must precede the change
> to the definition of lockless_dereference(), so the two should go
> together.

Indeed.

> If my upcoming testing of the two changes together pans out, I will
> give you a Tested-by -- I am guessing that you don't want to wait
> until the next merge window for these changes.

I don't mind hugely since I have a fix now, but this one actually
causes >1K warnings (including some "too many warnings!") for my build
(net/wireless and net/mac80211 only!) and drowns out the real ones...
I'm sure other parts of the tree are similarly affected.

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

Re: [PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Peter Zijlstra]

On Thu, Aug 11, 2016 at 11:26:47AM -0700, Paul E. McKenney wrote:
> If my upcoming testing of the two changes together pans out, I will
> give you a Tested-by -- I am guessing that you don't want to wait
> until the next merge window for these changes.

I was planning to stuff them in tip/locking/urgent, so they'd end up in
this release.

Re: [PATCH 1/2] Revert "drm/fb-helper: Reduce READ_ONCE(master) to lockless_dereference"

[Paul E. McKenney]

On Fri, Aug 12, 2016 at 08:25:43PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 11, 2016 at 11:26:47AM -0700, Paul E. McKenney wrote:
> > If my upcoming testing of the two changes together pans out, I will
> > give you a Tested-by -- I am guessing that you don't want to wait
> > until the next merge window for these changes.
> 
> I was planning to stuff them in tip/locking/urgent, so they'd end up in
> this release.

They seem to work fine for me, so for both:

Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

							Thanx, Paul

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