linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] m68k: Fix asm register constraints for atomic ops
@ 2021-07-25 10:46 Geert Uytterhoeven
  2021-07-25 14:24 ` Andreas Schwab
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-07-25 10:46 UTC (permalink / raw)
  To: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng
  Cc: Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, linux-kernel, Geert Uytterhoeven

Depending on register assignment by the compiler:

    {standard input}:3084: Error: operands mismatch -- statement `andl %a1,%d1' ignored
    {standard input}:3145: Error: operands mismatch -- statement `orl %a1,%d1' ignored
    {standard input}:3195: Error: operands mismatch -- statement `eorl %a1,%d1' ignored

Indeed, the first operand must not be an address register.  Fix this by
adjusting the register constraint from "g" (general purpose register) to
"d" (data register).

Fixes: e39d88ea3ce4a471 ("locking/atomic, arch/m68k: Implement atomic_fetch_{add,sub,and,or,xor}()")
Fixes: d839bae4269aea46 ("locking,arch,m68k: Fold atomic_ops")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Reported-by: Alexander Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Personally, I have never seen this failure in an 68020+ build, but I can
reproduce it on Coldfire with the config provided by lkp (with bogus
CONFIG_RMW_INSNS=y).

Technically, the issue was present before, but I doubt adding pre-v3.18
Fixes tags would make any difference for stable...
---
 arch/m68k/include/asm/atomic.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/m68k/include/asm/atomic.h b/arch/m68k/include/asm/atomic.h
index 8637bf8a2f652009..2d5a6e556754290b 100644
--- a/arch/m68k/include/asm/atomic.h
+++ b/arch/m68k/include/asm/atomic.h
@@ -48,7 +48,7 @@ static inline int arch_atomic_##op##_return(int i, atomic_t *v)		\
 			"	casl %2,%1,%0\n"			\
 			"	jne 1b"					\
 			: "+m" (*v), "=&d" (t), "=&d" (tmp)		\
-			: "g" (i), "2" (arch_atomic_read(v)));		\
+			: "d" (i), "2" (arch_atomic_read(v)));		\
 	return t;							\
 }
 
@@ -63,7 +63,7 @@ static inline int arch_atomic_fetch_##op(int i, atomic_t *v)		\
 			"	casl %2,%1,%0\n"			\
 			"	jne 1b"					\
 			: "+m" (*v), "=&d" (t), "=&d" (tmp)		\
-			: "g" (i), "2" (arch_atomic_read(v)));		\
+			: "d" (i), "2" (arch_atomic_read(v)));		\
 	return tmp;							\
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-25 10:46 [PATCH] m68k: Fix asm register constraints for atomic ops Geert Uytterhoeven
@ 2021-07-25 14:24 ` Andreas Schwab
  2021-07-26  7:29   ` Geert Uytterhoeven
  2021-07-25 15:26 ` Arnd Bergmann
  2021-07-25 23:44 ` Finn Thain
  2 siblings, 1 reply; 7+ messages in thread
From: Andreas Schwab @ 2021-07-25 14:24 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng,
	Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, linux-kernel

On Jul 25 2021, Geert Uytterhoeven wrote:

> Depending on register assignment by the compiler:
>
>     {standard input}:3084: Error: operands mismatch -- statement `andl %a1,%d1' ignored
>     {standard input}:3145: Error: operands mismatch -- statement `orl %a1,%d1' ignored
>     {standard input}:3195: Error: operands mismatch -- statement `eorl %a1,%d1' ignored
>
> Indeed, the first operand must not be an address register.  Fix this by
> adjusting the register constraint from "g" (general purpose register) to
> "d" (data register).

You should also allow immediate ("i").

There is the ASM_DI macro for that, but since CONFIG_RMW_INSNS is never
defined for CONFIG_COLDFIRE, it probably doesn't matter.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-25 10:46 [PATCH] m68k: Fix asm register constraints for atomic ops Geert Uytterhoeven
  2021-07-25 14:24 ` Andreas Schwab
@ 2021-07-25 15:26 ` Arnd Bergmann
  2021-07-25 23:44 ` Finn Thain
  2 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2021-07-25 15:26 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng,
	Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, Linux Kernel Mailing List

On Sun, Jul 25, 2021 at 12:46 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Depending on register assignment by the compiler:
>
>     {standard input}:3084: Error: operands mismatch -- statement `andl %a1,%d1' ignored
>     {standard input}:3145: Error: operands mismatch -- statement `orl %a1,%d1' ignored
>     {standard input}:3195: Error: operands mismatch -- statement `eorl %a1,%d1' ignored
>
> Indeed, the first operand must not be an address register.  Fix this by
> adjusting the register constraint from "g" (general purpose register) to
> "d" (data register).
>
> Fixes: e39d88ea3ce4a471 ("locking/atomic, arch/m68k: Implement atomic_fetch_{add,sub,and,or,xor}()")
> Fixes: d839bae4269aea46 ("locking,arch,m68k: Fold atomic_ops")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Reported-by: Alexander Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Personally, I have never seen this failure in an 68020+ build, but I can
> reproduce it on Coldfire with the config provided by lkp (with bogus
> CONFIG_RMW_INSNS=y).

This fixed the problem for me in the 68020 build with all compiler versions.

Tested-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-25 10:46 [PATCH] m68k: Fix asm register constraints for atomic ops Geert Uytterhoeven
  2021-07-25 14:24 ` Andreas Schwab
  2021-07-25 15:26 ` Arnd Bergmann
@ 2021-07-25 23:44 ` Finn Thain
  2021-07-26  7:34   ` Geert Uytterhoeven
  2 siblings, 1 reply; 7+ messages in thread
From: Finn Thain @ 2021-07-25 23:44 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng,
	Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, linux-kernel

On Sun, 25 Jul 2021, Geert Uytterhoeven wrote:

> Fixes: d839bae4269aea46 ("locking,arch,m68k: Fold atomic_ops")
> ...
> Technically, the issue was present before, but I doubt adding pre-v3.18
> Fixes tags would make any difference for stable...

There is a better way to constrain backporting, that is Cc: 
stable@vger.kernel.org # 3.12+

The reason I mention it is that Fixes tags could be seen as a way to 
identify commits that introduce bugs, e.g. for the purposes of training 
AIs, or attributing blame, or measuring quality etc. I think it would be 
unfair to point the finger at Peter's commit.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-25 14:24 ` Andreas Schwab
@ 2021-07-26  7:29   ` Geert Uytterhoeven
  0 siblings, 0 replies; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-07-26  7:29 UTC (permalink / raw)
  To: Andreas Schwab
  Cc: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng,
	Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, Linux Kernel Mailing List

Hi Andreas,

On Sun, Jul 25, 2021 at 4:24 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Jul 25 2021, Geert Uytterhoeven wrote:
> > Depending on register assignment by the compiler:
> >
> >     {standard input}:3084: Error: operands mismatch -- statement `andl %a1,%d1' ignored
> >     {standard input}:3145: Error: operands mismatch -- statement `orl %a1,%d1' ignored
> >     {standard input}:3195: Error: operands mismatch -- statement `eorl %a1,%d1' ignored
> >
> > Indeed, the first operand must not be an address register.  Fix this by
> > adjusting the register constraint from "g" (general purpose register) to
> > "d" (data register).
>
> You should also allow immediate ("i").

Good point.

> There is the ASM_DI macro for that, but since CONFIG_RMW_INSNS is never
> defined for CONFIG_COLDFIRE, it probably doesn't matter.

As the second operand is a register, not memory, there is no need to
use ASM_DI, and "di" should be fine, right?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-25 23:44 ` Finn Thain
@ 2021-07-26  7:34   ` Geert Uytterhoeven
  2021-07-26  7:39     ` Arnd Bergmann
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2021-07-26  7:34 UTC (permalink / raw)
  To: Finn Thain
  Cc: Greg Ungerer, Will Deacon, Peter Zijlstra, Boqun Feng,
	Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, Linux Kernel Mailing List

Hi Finn,

On Mon, Jul 26, 2021 at 1:45 AM Finn Thain <fthain@linux-m68k.org> wrote:
> On Sun, 25 Jul 2021, Geert Uytterhoeven wrote:
> > Fixes: d839bae4269aea46 ("locking,arch,m68k: Fold atomic_ops")
> > ...
> > Technically, the issue was present before, but I doubt adding pre-v3.18
> > Fixes tags would make any difference for stable...
>
> There is a better way to constrain backporting, that is Cc:
> stable@vger.kernel.org # 3.12+

I don't want to constrain backporting.

> The reason I mention it is that Fixes tags could be seen as a way to
> identify commits that introduce bugs, e.g. for the purposes of training

OK, had a look through the full log....
There are no other commits introducing the bug (renaming and
merging files without changing functions doesn't count), except for the
initial import into git. So I'll add that one, too.

> AIs, or attributing blame, or measuring quality etc. I think it would be
> unfair to point the finger at Peter's commit.

The first Fixes commit definitely introduced a new buggy user.
The second Fixes commit is debatable, as it copied the bug for a new
function from two functions that were removed in the process.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] m68k: Fix asm register constraints for atomic ops
  2021-07-26  7:34   ` Geert Uytterhoeven
@ 2021-07-26  7:39     ` Arnd Bergmann
  0 siblings, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2021-07-26  7:39 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Finn Thain, Greg Ungerer, Will Deacon, Peter Zijlstra,
	Boqun Feng, Brendan Jackman, kernel test robot, Arnd Bergmann,
	Alexander Viro, linux-m68k, Linux Kernel Mailing List

On Mon, Jul 26, 2021 at 9:34 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Finn,
>
> On Mon, Jul 26, 2021 at 1:45 AM Finn Thain <fthain@linux-m68k.org> wrote:
> > On Sun, 25 Jul 2021, Geert Uytterhoeven wrote:
> > > Fixes: d839bae4269aea46 ("locking,arch,m68k: Fold atomic_ops")
> > > ...
> > > Technically, the issue was present before, but I doubt adding pre-v3.18
> > > Fixes tags would make any difference for stable...
> >
> > There is a better way to constrain backporting, that is Cc:
> > stable@vger.kernel.org # 3.12+
>
> I don't want to constrain backporting.

I would recommend adding the plain

Cc: stable@vger.kernel.org

line to the footer to make it unambiguous that you want it backported then,
plus moving the explanation above the --- line. You can never be too explicit
with those.

      Arnd

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-07-26  7:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-25 10:46 [PATCH] m68k: Fix asm register constraints for atomic ops Geert Uytterhoeven
2021-07-25 14:24 ` Andreas Schwab
2021-07-26  7:29   ` Geert Uytterhoeven
2021-07-25 15:26 ` Arnd Bergmann
2021-07-25 23:44 ` Finn Thain
2021-07-26  7:34   ` Geert Uytterhoeven
2021-07-26  7:39     ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).