[PATCH] m68k: Fix asm register constraints for atomic ops

[PATCH] m68k: Fix asm register constraints for atomic ops

[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

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Andreas Schwab]

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."

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Arnd Bergmann]

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>

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Finn Thain]

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.

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: Fix asm register constraints for atomic ops

[Arnd Bergmann]

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