m68k: atari: Make Atari ROM port I/O write macros return void

[PATCH] m68k: atari: Make Atari ROM port I/O write macros return void

Posted by Geert Uytterhoeven 3 years, 11 months ago

The macros implementing Atari ROM port I/O writes do not cast away their
output, unlike similar implementations for other I/O buses.
When they are combined using conditional expressions in the definitions of
outb() and friends, this triggers sparse warnings like:

    drivers/net/appletalk/cops.c:382:17: error: incompatible types in conditional expression (different base types):
    drivers/net/appletalk/cops.c:382:17:    unsigned char
    drivers/net/appletalk/cops.c:382:17:    void

Fix this by adding casts to "void".

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Survived m68k/allmodconfig.
To be queued in the m68k tree for v5.19.

Removing the casts instead causes issues with functions propagating void
return values (return expression in void function), which BTW sparse
complains about, too.
---
 arch/m68k/include/asm/raw_io.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/m68k/include/asm/raw_io.h b/arch/m68k/include/asm/raw_io.h
index 80eb2396d01ebf61..3ba40bc1dfaa9471 100644
--- a/arch/m68k/include/asm/raw_io.h
+++ b/arch/m68k/include/asm/raw_io.h
@@ -80,14 +80,14 @@
 	({ u16 __v = le16_to_cpu(*(__force volatile u16 *) (addr)); __v; })
 
 #define rom_out_8(addr, b)	\
-	({u8 __maybe_unused __w, __v = (b);  u32 _addr = ((u32) (addr)); \
+	(void)({u8 __maybe_unused __w, __v = (b);  u32 _addr = ((u32) (addr)); \
 	__w = ((*(__force volatile u8 *)  ((_addr | 0x10000) + (__v<<1)))); })
 #define rom_out_be16(addr, w)	\
-	({u16 __maybe_unused __w, __v = (w); u32 _addr = ((u32) (addr)); \
+	(void)({u16 __maybe_unused __w, __v = (w); u32 _addr = ((u32) (addr)); \
 	__w = ((*(__force volatile u16 *) ((_addr & 0xFFFF0000UL) + ((__v & 0xFF)<<1)))); \
 	__w = ((*(__force volatile u16 *) ((_addr | 0x10000) + ((__v >> 8)<<1)))); })
 #define rom_out_le16(addr, w)	\
-	({u16 __maybe_unused __w, __v = (w); u32 _addr = ((u32) (addr)); \
+	(void)({u16 __maybe_unused __w, __v = (w); u32 _addr = ((u32) (addr)); \
 	__w = ((*(__force volatile u16 *) ((_addr & 0xFFFF0000UL) + ((__v >> 8)<<1)))); \
 	__w = ((*(__force volatile u16 *) ((_addr | 0x10000) + ((__v & 0xFF)<<1)))); })
 
-- 
2.25.1

Re: [PATCH] m68k: atari: Make Atari ROM port I/O write macros return void

Posted by Guenter Roeck 3 years, 11 months ago

On 5/20/22 07:32, Geert Uytterhoeven wrote:
> The macros implementing Atari ROM port I/O writes do not cast away their
> output, unlike similar implementations for other I/O buses.
> When they are combined using conditional expressions in the definitions of
> outb() and friends, this triggers sparse warnings like:
> 
>      drivers/net/appletalk/cops.c:382:17: error: incompatible types in conditional expression (different base types):
>      drivers/net/appletalk/cops.c:382:17:    unsigned char
>      drivers/net/appletalk/cops.c:382:17:    void
> 
> Fix this by adding casts to "void".
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
> Survived m68k/allmodconfig.
> To be queued in the m68k tree for v5.19.
> 
> Removing the casts instead causes issues with functions propagating void
> return values (return expression in void function), which BTW sparse
> complains about, too.

We live and learn. I didn't even know that this was valid syntax.
I thought it might be easier to just fix that code, but coccinelle
reports that there are hundreds of places in the kernel where this
is done. Outch.

Guenter

Re: [PATCH] m68k: atari: Make Atari ROM port I/O write macros return void

Posted by Michael Schmitz 3 years, 11 months ago

Hi Guenter,

I wish I knew of a way to fix this mess for good... suggestions would be 
welcome.

Looks like you spotted the last remaining macros without a void cast 
here, thanks.

FWIW:
Reviewed-by: Michael Schmitz <schmitzmic@gmail.com>


Am 21.05.2022 um 02:52 schrieb Guenter Roeck:
> On 5/20/22 07:32, Geert Uytterhoeven wrote:
>> The macros implementing Atari ROM port I/O writes do not cast away their
>> output, unlike similar implementations for other I/O buses.
>> When they are combined using conditional expressions in the
>> definitions of
>> outb() and friends, this triggers sparse warnings like:
>>
>>      drivers/net/appletalk/cops.c:382:17: error: incompatible types in
>> conditional expression (different base types):
>>      drivers/net/appletalk/cops.c:382:17:    unsigned char
>>      drivers/net/appletalk/cops.c:382:17:    void
>>
>> Fix this by adding casts to "void".
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
>
>> ---
>> Survived m68k/allmodconfig.
>> To be queued in the m68k tree for v5.19.
>>
>> Removing the casts instead causes issues with functions propagating void
>> return values (return expression in void function), which BTW sparse
>> complains about, too.
>
> We live and learn. I didn't even know that this was valid syntax.
> I thought it might be easier to just fix that code, but coccinelle
> reports that there are hundreds of places in the kernel where this
> is done. Outch.
>
> Guenter

Re: [PATCH] m68k: atari: Make Atari ROM port I/O write macros return void

Posted by Geert Uytterhoeven 3 years, 11 months ago

Hi Günter,

On Fri, May 20, 2022 at 4:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 5/20/22 07:32, Geert Uytterhoeven wrote:
> > The macros implementing Atari ROM port I/O writes do not cast away their
> > output, unlike similar implementations for other I/O buses.
> > When they are combined using conditional expressions in the definitions of
> > outb() and friends, this triggers sparse warnings like:
> >
> >      drivers/net/appletalk/cops.c:382:17: error: incompatible types in conditional expression (different base types):
> >      drivers/net/appletalk/cops.c:382:17:    unsigned char
> >      drivers/net/appletalk/cops.c:382:17:    void
> >
> > Fix this by adding casts to "void".
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Thanks!

> > Removing the casts instead causes issues with functions propagating void
> > return values (return expression in void function), which BTW sparse
> > complains about, too.
>
> We live and learn. I didn't even know that this was valid syntax.

I knew about the syntax, but didn't realize immediately why it was
done that way.

Initially I thought it was some relic from the "always cast to void
to make it clear you do not care about the return value"-frenzy, which
are inside Linux visible mostly in the various "(void)acpi_<foo>(...);"
calls.  AFAIK these are checked by some external tools.
In Linux, we have __must_check to annotate the important cases.

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: atari: Make Atari ROM port I/O write macros return void

Posted by Guenter Roeck 3 years, 11 months ago

On 5/20/22 08:07, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Fri, May 20, 2022 at 4:52 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> On 5/20/22 07:32, Geert Uytterhoeven wrote:
>>> The macros implementing Atari ROM port I/O writes do not cast away their
>>> output, unlike similar implementations for other I/O buses.
>>> When they are combined using conditional expressions in the definitions of
>>> outb() and friends, this triggers sparse warnings like:
>>>
>>>       drivers/net/appletalk/cops.c:382:17: error: incompatible types in conditional expression (different base types):
>>>       drivers/net/appletalk/cops.c:382:17:    unsigned char
>>>       drivers/net/appletalk/cops.c:382:17:    void
>>>
>>> Fix this by adding casts to "void".
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks!
> 
>>> Removing the casts instead causes issues with functions propagating void
>>> return values (return expression in void function), which BTW sparse
>>> complains about, too.
>>
>> We live and learn. I didn't even know that this was valid syntax.
> 
> I knew about the syntax, but didn't realize immediately why it was
> done that way.
> 
> Initially I thought it was some relic from the "always cast to void
> to make it clear you do not care about the return value"-frenzy, which
> are inside Linux visible mostly in the various "(void)acpi_<foo>(...);"
> calls.  AFAIK these are checked by some external tools.
> In Linux, we have __must_check to annotate the important cases.
> 

I looked at the output of my coccinelle script. To me it looks like
(almost ?) all of the code with "return f(...)" in a function returning
void is just sloppy programming.

In case someone is interested, I attached the script I used below.

Guenter

---
virtual report

@d@
function f;
expression e;
position p;
@@

void f(...)
{
     <...
     return e@p;
     ...>
}

@script:python depends on report@
p << d.p;
f << d.f;
e << d.e;
@@

print "Return in void function %s() at %s:%s: %s" % (f, p[0].file, p[0].line, e)