[PATCH -next] kernel: Replace ACCESS_ONCE with READ/WRITE_ONCE

[PATCH -next] kernel: Replace ACCESS_ONCE with READ/WRITE_ONCE

[Davidlohr Bueso]

We already have a number of calls converted, but still a decent
amount of ACCESS_ONCE callers in core-code, which is somewhat
troubling. Convert more users (even if some are scalar types,
we want to someday get rid of ACCESS_ONCE altogether I suppose.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 Documentation/atomic_ops.txt | 18 +++++++++---------
 include/linux/bitops.h       |  2 +-
 include/linux/llist.h        |  2 +-
 kernel/acct.c                |  4 ++--
 kernel/events/core.c         |  6 +++---
 kernel/events/ring_buffer.c  |  2 +-
 kernel/exit.c                |  2 +-
 kernel/kthread.c             |  2 +-
 kernel/task_work.c           |  6 +++---
 kernel/trace/ring_buffer.c   |  2 +-
 kernel/trace/trace.h         |  2 +-
 kernel/trace/trace_stack.c   |  2 +-
 kernel/user_namespace.c      |  2 +-
 kernel/watchdog.c            |  4 ++--
 kernel/workqueue.c           |  4 ++--
 15 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/Documentation/atomic_ops.txt b/Documentation/atomic_ops.txt
index c9d1cacb4395..9e880d221820 100644
--- a/Documentation/atomic_ops.txt
+++ b/Documentation/atomic_ops.txt
@@ -90,10 +90,10 @@ compiler optimizes the section accessing atomic_t variables.
 
 Properly aligned pointers, longs, ints, and chars (and unsigned
 equivalents) may be atomically loaded from and stored to in the same
-sense as described for atomic_read() and atomic_set().  The ACCESS_ONCE()
-macro should be used to prevent the compiler from using optimizations
-that might otherwise optimize accesses out of existence on the one hand,
-or that might create unsolicited accesses on the other.
+sense as described for atomic_read() and atomic_set().  The READ_ONCE()
+and WRITE_ONCE() macros should be used to prevent the compiler from using
+optimizations that might otherwise optimize accesses out of existence on
+the one hand, or that might create unsolicited accesses on the other.
 
 For example consider the following code:
 
@@ -112,7 +112,7 @@ the following:
 If you don't want the compiler to do this (and you probably don't), then
 you should use something like the following:
 
-	while (ACCESS_ONCE(a) < 0)
+	while (READ_ONCE(a) < 0)
 		do_something();
 
 Alternatively, you could place a barrier() call in the loop.
@@ -141,7 +141,7 @@ of registers: reloading from variable a could save a flush to the
 stack and later reload.  To prevent the compiler from attacking your
 code in this manner, write the following:
 
-	tmp_a = ACCESS_ONCE(a);
+	tmp_a = READ_ONCE(a);
 	do_something_with(tmp_a);
 	do_something_else_with(tmp_a);
 
@@ -166,14 +166,14 @@ that expected b to never have the value 42 if a was zero.  To prevent
 the compiler from doing this, write something like:
 
 	if (a)
-		ACCESS_ONCE(b) = 9;
+		WRITE_ONCE(b, 9);
 	else
-		ACCESS_ONCE(b) = 42;
+		WRITE_ONCE(b, 42);
 
 Don't even -think- about doing this without proper use of memory barriers,
 locks, or atomic operations if variable a can change at runtime!
 
-*** WARNING: ACCESS_ONCE() DOES NOT IMPLY A BARRIER! ***
+*** WARNING: NEITHER READ_ONCE() OR WRITE_ONCE() IMPLY A BARRIER! ***
 
 Now, we move onto the atomic operation interfaces typically implemented with
 the help of assembly code.
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index defeaac0745f..26da22da6f63 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -219,7 +219,7 @@ static inline unsigned long __ffs64(u64 word)
 	typeof(*ptr) old, new;					\
 								\
 	do {							\
-		old = ACCESS_ONCE(*ptr);			\
+		old = READ_ONCE(*ptr);				\
 		new = (old & ~mask) | bits;			\
 	} while (cmpxchg(ptr, old, new) != old);		\
 								\
diff --git a/include/linux/llist.h b/include/linux/llist.h
index fd4ca0b4fe0f..2381ace84815 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -157,7 +157,7 @@ static inline void init_llist_head(struct llist_head *list)
  */
 static inline bool llist_empty(const struct llist_head *head)
 {
-	return ACCESS_ONCE(head->first) == NULL;
+	return READ_ONCE(head->first) == NULL;
 }
 
 static inline struct llist_node *llist_next(struct llist_node *node)
diff --git a/kernel/acct.c b/kernel/acct.c
index 74963d192c5d..fca2d25c3708 100644
--- a/kernel/acct.c
+++ b/kernel/acct.c
@@ -144,7 +144,7 @@ static struct bsd_acct_struct *acct_get(struct pid_namespace *ns)
 again:
 	smp_rmb();
 	rcu_read_lock();
-	res = to_acct(ACCESS_ONCE(ns->bacct));
+	res = to_acct(READ_ONCE(ns->bacct));
 	if (!res) {
 		rcu_read_unlock();
 		return NULL;
@@ -156,7 +156,7 @@ again:
 	}
 	rcu_read_unlock();
 	mutex_lock(&res->lock);
-	if (res != to_acct(ACCESS_ONCE(ns->bacct))) {
+	if (res != to_acct(READ_ONCE(ns->bacct))) {
 		mutex_unlock(&res->lock);
 		acct_put(res);
 		goto again;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 8c3b35f2a269..ca9060cdd6a0 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1119,7 +1119,7 @@ perf_event_ctx_lock_nested(struct perf_event *event, int nesting)
 
 again:
 	rcu_read_lock();
-	ctx = ACCESS_ONCE(event->ctx);
+	ctx = READ_ONCE(event->ctx);
 	if (!atomic_inc_not_zero(&ctx->refcount)) {
 		rcu_read_unlock();
 		goto again;
@@ -4881,8 +4881,8 @@ static int perf_mmap(struct file *file, struct vm_area_struct *vma)
 		if (!rb)
 			goto aux_unlock;
 
-		aux_offset = ACCESS_ONCE(rb->user_page->aux_offset);
-		aux_size = ACCESS_ONCE(rb->user_page->aux_size);
+		aux_offset = READ_ONCE(rb->user_page->aux_offset);
+		aux_size = READ_ONCE(rb->user_page->aux_size);
 
 		if (aux_offset < perf_data_size(rb) + PAGE_SIZE)
 			goto aux_unlock;
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 60be55a64040..b39f4f809b62 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -346,7 +346,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	 * (B) <-> (C) ordering is still observed by the pmu driver.
 	 */
 	if (!rb->aux_overwrite) {
-		aux_tail = ACCESS_ONCE(rb->user_page->aux_tail);
+		aux_tail = READ_ONCE(rb->user_page->aux_tail);
 		handle->wakeup = local_read(&rb->aux_wakeup) + rb->aux_watermark;
 		if (aux_head - aux_tail < perf_aux_size(rb))
 			handle->size = CIRC_SPACE(aux_head, aux_tail, perf_aux_size(rb));
diff --git a/kernel/exit.c b/kernel/exit.c
index fd90195667e1..d0d57d938366 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -1294,7 +1294,7 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
 	 * Ensure that EXIT_ZOMBIE -> EXIT_DEAD/EXIT_TRACE transition
 	 * can't confuse the checks below.
 	 */
-	int exit_state = ACCESS_ONCE(p->exit_state);
+	int exit_state = READ_ONCE(p->exit_state);
 	int ret;
 
 	if (unlikely(exit_state == EXIT_DEAD))
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9ff173dca1ae..fefa65d102b9 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -63,7 +63,7 @@ static inline struct kthread *to_kthread(struct task_struct *k)
 
 static struct kthread *to_live_kthread(struct task_struct *k)
 {
-	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
+	struct completion *vfork = READ_ONCE(k->vfork_done);
 	if (likely(vfork))
 		return __to_kthread(vfork);
 	return NULL;
diff --git a/kernel/task_work.c b/kernel/task_work.c
index 53fa971d000d..b16168d6988b 100644
--- a/kernel/task_work.c
+++ b/kernel/task_work.c
@@ -29,7 +29,7 @@ task_work_add(struct task_struct *task, struct callback_head *work, bool notify)
 	struct callback_head *head;
 
 	do {
-		head = ACCESS_ONCE(task->task_works);
+		head = READ_ONCE(task->task_works);
 		if (unlikely(head == &work_exited))
 			return -ESRCH;
 		work->next = head;
@@ -64,7 +64,7 @@ task_work_cancel(struct task_struct *task, task_work_func_t func)
 	 * we raced with task_work_run(), *pprev == NULL/exited.
 	 */
 	raw_spin_lock_irqsave(&task->pi_lock, flags);
-	while ((work = ACCESS_ONCE(*pprev))) {
+	while ((work = READ_ONCE(*pprev))) {
 		smp_read_barrier_depends();
 		if (work->func != func)
 			pprev = &work->next;
@@ -95,7 +95,7 @@ void task_work_run(void)
 		 * work_exited unless the list is empty.
 		 */
 		do {
-			work = ACCESS_ONCE(task->task_works);
+			work = READ_ONCE(task->task_works);
 			head = !work && (task->flags & PF_EXITING) ?
 				&work_exited : NULL;
 		} while (cmpxchg(&task->task_works, work, head) != work);
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 95181e36891a..c629d0a392a9 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -2753,7 +2753,7 @@ rb_reserve_next_event(struct ring_buffer *buffer,
 	 * if it happened, we have to fail the write.
 	 */
 	barrier();
-	if (unlikely(ACCESS_ONCE(cpu_buffer->buffer) != buffer)) {
+	if (unlikely(READ_ONCE(cpu_buffer->buffer) != buffer)) {
 		local_dec(&cpu_buffer->committing);
 		local_dec(&cpu_buffer->commits);
 		return NULL;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 3fff4adfd431..7a1a0c512a89 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1154,7 +1154,7 @@ extern struct trace_event_file *find_event_file(struct trace_array *tr,
 
 static inline void *event_file_data(struct file *filp)
 {
-	return ACCESS_ONCE(file_inode(filp)->i_private);
+	return READ_ONCE(file_inode(filp)->i_private);
 }
 
 extern struct mutex event_mutex;
diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 2a1abbaca10e..008a28ad652a 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -76,7 +76,7 @@ check_stack(unsigned long ip, unsigned long *stack)
 {
 	unsigned long this_size, flags; unsigned long *p, *top, *start;
 	static int tracer_frame;
-	int frame_size = ACCESS_ONCE(tracer_frame);
+	int frame_size = READ_ONCE(tracer_frame);
 	int i, x;
 
 	this_size = ((unsigned long)stack) & (THREAD_SIZE-1);
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 9bafc211930c..2dc24bcc99f8 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -847,7 +847,7 @@ static bool new_idmap_permitted(const struct file *file,
 int proc_setgroups_show(struct seq_file *seq, void *v)
 {
 	struct user_namespace *ns = seq->private;
-	unsigned long userns_flags = ACCESS_ONCE(ns->flags);
+	unsigned long userns_flags = READ_ONCE(ns->flags);
 
 	seq_printf(seq, "%s\n",
 		   (userns_flags & USERNS_SETGROUPS_ALLOWED) ?
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9acb29f280ec..0dda0b005927 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -981,7 +981,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 		goto out;
 	}
 
-	old = ACCESS_ONCE(watchdog_thresh);
+	old = READ_ONCE(watchdog_thresh);
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 
 	if (err || !write)
@@ -990,7 +990,7 @@ int proc_watchdog_thresh(struct ctl_table *table, int write,
 	/*
 	 * Update the sample period. Restore on failure.
 	 */
-	new = ACCESS_ONCE(watchdog_thresh);
+	new = READ_ONCE(watchdog_thresh);
 	if (old == new)
 		goto out;
 
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2232ae3e3ad6..767aa8ae7492 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4550,7 +4550,7 @@ static void rebind_workers(struct worker_pool *pool)
 		 * concurrency management.  Note that when or whether
 		 * @worker clears REBOUND doesn't affect correctness.
 		 *
-		 * ACCESS_ONCE() is necessary because @worker->flags may be
+		 * WRITE_ONCE() is necessary because @worker->flags may be
 		 * tested without holding any lock in
 		 * wq_worker_waking_up().  Without it, NOT_RUNNING test may
 		 * fail incorrectly leading to premature concurrency
@@ -4559,7 +4559,7 @@ static void rebind_workers(struct worker_pool *pool)
 		WARN_ON_ONCE(!(worker_flags & WORKER_UNBOUND));
 		worker_flags |= WORKER_REBOUND;
 		worker_flags &= ~WORKER_UNBOUND;
-		ACCESS_ONCE(worker->flags) = worker_flags;
+		WRITE_ONCE(worker->flags, worker_flags);
 	}
 
 	spin_unlock_irq(&pool->lock);
-- 
2.8.1

[PATCH] checkpatch: Whine about ACCESS_ONCE

[Joe Perches]

Add a test for use of ACCESS_ONCE that could be written using
READ_ONCE or WRITE_ONCE.

--fix it too if desired.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3d9c34..5e5d2a4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5837,6 +5837,28 @@ sub process {
 			}
 		}
 
+# whine about ACCESS_ONCE
+		if ($^V && $^V ge 5.10.0 &&
+		    $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {
+			my $par = $1;
+			my $eq = $2;
+			my $fun = $3;
+			$par =~ s/^\(\s*(.*)\s*\)$/$1/;
+			if (defined($eq)) {
+				if (WARN("PREFER_WRITE_ONCE",
+					 "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;
+				}
+			} else {
+				if (WARN("PREFER_READ_ONCE",
+					 "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;
+				}
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Joe Perches]

On Sat, 2016-04-16 at 12:04 -0700, Joe Perches wrote:
> Add a test for use of ACCESS_ONCE that could be written using
> READ_ONCE or WRITE_ONCE.
> 
> --fix it too if desired.

And here's a simple coccinelle script that does a
rather better job:

$ cat access_once.cocci
@@
expression e1;
expression e2;
@@

-	ACCESS_ONCE(e1) = e2
+	WRITE_ONCE(e1, e2)

@@
expression e1;
@@

-	ACCESS_ONCE(e1)
+	READ_ONCE(e1)

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Julia Lawall]

On Sat, 16 Apr 2016, Joe Perches wrote:

> On Sat, 2016-04-16 at 12:04 -0700, Joe Perches wrote:
> > Add a test for use of ACCESS_ONCE that could be written using
> > READ_ONCE or WRITE_ONCE.
> > 
> > --fix it too if desired.
> 
> And here's a simple coccinelle script that does a
> rather better job:
> 
> $ cat access_once.cocci
> @@
> expression e1;
> expression e2;
> @@
> 
> -	ACCESS_ONCE(e1) = e2
> +	WRITE_ONCE(e1, e2)
> 
> @@
> expression e1;
> @@
> 
> -	ACCESS_ONCE(e1)
> +	READ_ONCE(e1)

Looks good to me.  Is this something to put in the kernel?

julia

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Joe Perches]

On Sun, 2016-04-17 at 07:43 +0200, Julia Lawall wrote:
> On Sat, 16 Apr 2016, Joe Perches wrote:
> > On Sat, 2016-04-16 at 12:04 -0700, Joe Perches wrote:
> > > Add a test for use of ACCESS_ONCE that could be written using
> > > READ_ONCE or WRITE_ONCE.
> > > 
> > > --fix it too if desired.
> > 
> > And here's a simple coccinelle script that does a
> > rather better job:
> > 
> > $ cat access_once.cocci
> > @@
> > expression e1;
> > expression e2;
> > @@
> > 
> > -     ACCESS_ONCE(e1) = e2
> > +     WRITE_ONCE(e1, e2)
> > 
> > @@
> > expression e1;
> > @@
> > 
> > -     ACCESS_ONCE(e1)
> > +     READ_ONCE(e1)
> 
> Looks good to me.  Is this something to put in the kernel?

Maybe.  There are more than 500 of these ACCESS_ONCE
uses that could be converted.

The kernel cocci scripts are typically more complicated
with virtual org/report/context/patch blocks.

So perhaps this needs to be fleshed out more.

Works for me as-is though.

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1052 bytes --]



On Sun, 17 Apr 2016, Joe Perches wrote:

> On Sun, 2016-04-17 at 07:43 +0200, Julia Lawall wrote:
> > On Sat, 16 Apr 2016, Joe Perches wrote:
> > > On Sat, 2016-04-16 at 12:04 -0700, Joe Perches wrote:
> > > > Add a test for use of ACCESS_ONCE that could be written using
> > > > READ_ONCE or WRITE_ONCE.
> > > > 
> > > > --fix it too if desired.
> > > 
> > > And here's a simple coccinelle script that does a
> > > rather better job:
> > > 
> > > $ cat access_once.cocci
> > > @@
> > > expression e1;
> > > expression e2;
> > > @@
> > > 
> > > -     ACCESS_ONCE(e1) = e2
> > > +     WRITE_ONCE(e1, e2)
> > > 
> > > @@
> > > expression e1;
> > > @@
> > > 
> > > -     ACCESS_ONCE(e1)
> > > +     READ_ONCE(e1)
> > 
> > Looks good to me.  Is this something to put in the kernel?
> 
> Maybe.  There are more than 500 of these ACCESS_ONCE
> uses that could be converted.
> 
> The kernel cocci scripts are typically more complicated
> with virtual org/report/context/patch blocks.
> 
> So perhaps this needs to be fleshed out more.

I can do that.

julia

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Julia Lawall]

A suitably versatile semantic patch is below.  Feel free to update 
at 
least the copyright line, and perhaps the initial explanation and the 
strings printed in report and org modes.

julia

/// Use READ_ONCE or WRITE_ONCE instead of ACCESS_ONCE.
///
// Confidence: High
// Copyright: (C) 2016 Joe Perches, afffiliation?, license?
// Options: --no-includes --include-headers

virtual patch
virtual context
virtual org
virtual report

@written depends on patch && !context && !org && !report@
expression e1;
expression e2;
@@

-       ACCESS_ONCE(e1) = e2
+       WRITE_ONCE(e1, e2)

@read depends on patch && !context && !org && !report@
expression e1;
@@

-       ACCESS_ONCE(e1)
+       READ_ONCE(e1)

// ----------------------------------------------------------------------------

@written_context depends on !patch && (context || org || report)@
expression e1, e2;
position j0;
@@

*        ACCESS_ONCE@j0(e1) = e2

@read_context depends on !patch && (context || org || report)@
expression e1;
position j0;
@@

*        ACCESS_ONCE@j0(e1)

// ----------------------------------------------------------------------------

@script:python written_org depends on org@
j0 << written_context.j0;
@@

msg = "Use WRITE_ONCE for an ACCESS_ONCE that is written."
coccilib.org.print_todo(j0[0], msg, color="ovl-face2")

@script:python read_org depends on org@
j0 << read_context.j0;
@@

msg = "Use READ_ONCE for an ACCESS_ONCE that is read."
coccilib.org.print_todo(j0[0], msg)

// ----------------------------------------------------------------------------

@script:python written_report depends on report@
j0 << written_context.j0;
@@

msg = "Use WRITE_ONCE for an ACCESS_ONCE that is written."
coccilib.report.print_report(j0[0], msg)

@script:python read_report depends on report@
j0 << read_context.j0;
@@

msg = "Use READ_ONCE for an ACCESS_ONCE that is read."
coccilib.report.print_report(j0[0], msg)

Re: [PATCH] checkpatch: Whine about ACCESS_ONCE

[Joe Perches]

On Sun, 2016-04-17 at 13:39 +0200, Julia Lawall wrote:
> A suitably versatile semantic patch is below.  Feel free to update 
> at least the copyright line, and perhaps the initial explanation
> and the strings printed in report and org modes.

Seems sensible enough, thanks Julia.

// Copyright: (C) 2016 Joe Perches, afffiliation?, license?

The structure is all your work so you can mark it as
you please, but for me 'no rights reserved'

[PATCH V2] checkpatch: Whine about ACCESS_ONCE

[Joe Perches]

Add a test for use of ACCESS_ONCE that could be written using
READ_ONCE or WRITE_ONCE.

--fix it too if desired.

The WRITE_ONCE fixes are less correct than the coccinelle script below
as checkpatch cannot have a completely correct "expression" mechanism
because checkpatch works on patches and not complete files.

$ cat access_once.cocci
@@
expression e1;
expression e2;
@@

-       ACCESS_ONCE(e1) = e2
+       WRITE_ONCE(e1, e2)

@@
expression e1;
@@

-       ACCESS_ONCE(e1)
+       READ_ONCE(e1)

Signed-off-by: Joe Perches <joe@perches.com>
---

V2: Evolution 3.18 is a _really_ bad email client for sending patches.
    Improve the commit log a little and include a proper coccinelle script.

 scripts/checkpatch.pl | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3d9c34..5e5d2a4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5837,6 +5837,28 @@ sub process {
 			}
 		}
 
+# whine about ACCESS_ONCE
+		if ($^V && $^V ge 5.10.0 &&
+		    $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {
+			my $par = $1;
+			my $eq = $2;
+			my $fun = $3;
+			$par =~ s/^\(\s*(.*)\s*\)$/$1/;
+			if (defined($eq)) {
+				if (WARN("PREFER_WRITE_ONCE",
+					 "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;
+				}
+			} else {
+				if (WARN("PREFER_READ_ONCE",
+					 "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;
+				}
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {
-- 
2.8.0.rc4.16.g56331f8

coccinelle: bool if (foo) return true; else return false;

[Joe Perches]

There's ~150 of these in the kernel.

Maybe there's use for this conversion to be added
to scripts/coccinelle/misc/boolreturn.cocci or in
a separate file.

$ cat booltruefalse.cocci
@@
identifier fn;
expression e;
typedef bool;
symbol true;
symbol false;
@@

bool fn ( ... )
{
<...
-	if (e) return true; else return false;
+	return e;
...>
}

@@
identifier fn;
expression e;
@@

bool fn ( ... )
{
<...
-	if (e) return false; else return true;
+	return !e;
...>
}

[Cocci] coccinelle: bool if (foo) return true; else return false;

[Joe Perches]

There's ~150 of these in the kernel.

Maybe there's use for this conversion to be added
to?scripts/coccinelle/misc/boolreturn.cocci or in
a separate file.

$ cat booltruefalse.cocci
@@
identifier fn;
expression e;
typedef bool;
symbol true;
symbol false;
@@

bool fn ( ... )
{
<...
-	if (e) return true; else return false;
+	return e;
...>
}

@@
identifier fn;
expression e;
@@

bool fn ( ... )
{
<...
-	if (e) return false; else return true;
+	return !e;
...>
}

Re: coccinelle: bool if (foo) return true; else return false;

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 643 bytes --]



On Tue, 19 Apr 2016, Joe Perches wrote:

> There's ~150 of these in the kernel.
> 
> Maybe there's use for this conversion to be added
> to scripts/coccinelle/misc/boolreturn.cocci or in
> a separate file.
> 
> $ cat booltruefalse.cocci
> @@
> identifier fn;
> expression e;
> typedef bool;
> symbol true;
> symbol false;
> @@
> 
> bool fn ( ... )
> {
> <...
> -	if (e) return true; else return false;
> +	return e;
> ...>
> }
> 
> @@
> identifier fn;
> expression e;
> @@
> 
> bool fn ( ... )
> {
> <...
> -	if (e) return false; else return true;
> +	return !e;
> ...>
> }

Thanks for the suggestion.  I will take care of it shortly.

julia

[Cocci] coccinelle: bool if (foo) return true; else return false;

[Julia Lawall]

On Tue, 19 Apr 2016, Joe Perches wrote:

> There's ~150 of these in the kernel.
> 
> Maybe there's use for this conversion to be added
> to?scripts/coccinelle/misc/boolreturn.cocci or in
> a separate file.
> 
> $ cat booltruefalse.cocci
> @@
> identifier fn;
> expression e;
> typedef bool;
> symbol true;
> symbol false;
> @@
> 
> bool fn ( ... )
> {
> <...
> -	if (e) return true; else return false;
> +	return e;
> ...>
> }
> 
> @@
> identifier fn;
> expression e;
> @@
> 
> bool fn ( ... )
> {
> <...
> -	if (e) return false; else return true;
> +	return !e;
> ...>
> }

Thanks for the suggestion.  I will take care of it shortly.

julia

Re: [Cocci] coccinelle: bool if (foo) return true; else return false;

[Michael Stefaniuc]

On 04/19/2016 09:15 PM, Julia Lawall wrote:
> 
> 
> On Tue, 19 Apr 2016, Joe Perches wrote:
> 
>> There's ~150 of these in the kernel.
>>
>> Maybe there's use for this conversion to be added
>> to scripts/coccinelle/misc/boolreturn.cocci or in
>> a separate file.
>>
>> $ cat booltruefalse.cocci
>> @@
>> identifier fn;
>> expression e;
>> typedef bool;
>> symbol true;
>> symbol false;
>> @@
>>
>> bool fn ( ... )
>> {
>> <...
>> -	if (e) return true; else return false;
>> +	return e;
Shouldn't that be:
    return !!e
?


>> ...>
>> }
>>
>> @@
>> identifier fn;
>> expression e;
>> @@
>>
>> bool fn ( ... )
>> {
>> <...
>> -	if (e) return false; else return true;
>> +	return !e;
>> ...>
>> }
> 
> Thanks for the suggestion.  I will take care of it shortly.


bye
	michael

[Cocci] coccinelle: bool if (foo) return true; else return false;

[Michael Stefaniuc]

On 04/19/2016 09:15 PM, Julia Lawall wrote:
> 
> 
> On Tue, 19 Apr 2016, Joe Perches wrote:
> 
>> There's ~150 of these in the kernel.
>>
>> Maybe there's use for this conversion to be added
>> to scripts/coccinelle/misc/boolreturn.cocci or in
>> a separate file.
>>
>> $ cat booltruefalse.cocci
>> @@
>> identifier fn;
>> expression e;
>> typedef bool;
>> symbol true;
>> symbol false;
>> @@
>>
>> bool fn ( ... )
>> {
>> <...
>> -	if (e) return true; else return false;
>> +	return e;
Shouldn't that be:
    return !!e
?


>> ...>
>> }
>>
>> @@
>> identifier fn;
>> expression e;
>> @@
>>
>> bool fn ( ... )
>> {
>> <...
>> -	if (e) return false; else return true;
>> +	return !e;
>> ...>
>> }
> 
> Thanks for the suggestion.  I will take care of it shortly.


bye
	michael

Re: [Cocci] coccinelle: bool if (foo) return true; else return false;

[Joe Perches]

On Wed, 2016-04-20 at 09:19 +0200, Michael Stefaniuc wrote:
> On 04/19/2016 09:15 PM, Julia Lawall wrote:
> > On Tue, 19 Apr 2016, Joe Perches wrote:
> > > There's ~150 of these in the kernel.
> > > 
> > > Maybe there's use for this conversion to be added
> > > to scripts/coccinelle/misc/boolreturn.cocci or in
> > > a separate file.
> > > 
> > > $ cat booltruefalse.cocci
> > > @@
> > > identifier fn;
> > > expression e;
> > > typedef bool;
> > > symbol true;
> > > symbol false;
> > > @@
> > > 
> > > bool fn ( ... )
> > > {
> > > <...
> > > -	if (e) return true; else return false;
> > > +	return e;
> Shouldn't that be:
>     return !!e
> ?

No, it's not necessary.
The compiler does that because the return type is bool

6.3.1.2 Boolean type

When any scalar value is converted to _Bool, the result is 0 if the value compares equal
to 0; otherwise, the result is 1.

[Cocci] coccinelle: bool if (foo) return true; else return false;

[Joe Perches]

On Wed, 2016-04-20 at 09:19 +0200, Michael Stefaniuc wrote:
> On 04/19/2016 09:15 PM, Julia Lawall wrote:
> > On Tue, 19 Apr 2016, Joe Perches wrote:
> > > There's ~150 of these in the kernel.
> > > 
> > > Maybe there's use for this conversion to be added
> > > to scripts/coccinelle/misc/boolreturn.cocci or in
> > > a separate file.
> > > 
> > > $ cat booltruefalse.cocci
> > > @@
> > > identifier fn;
> > > expression e;
> > > typedef bool;
> > > symbol true;
> > > symbol false;
> > > @@
> > > 
> > > bool fn ( ... )
> > > {
> > > <...
> > > -	if (e) return true; else return false;
> > > +	return e;
> Shouldn't that be:
> ????return !!e
> ?

No, it's not necessary.
The compiler does that because the return type is bool

6.3.1.2 Boolean type

When any scalar value is converted to _Bool, the result is 0 if the value compares equal
to 0; otherwise, the result is 1.