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 <[email protected]>
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Geert Uytterhoeven <[email protected]>
---
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
Hi Günter,
On Fri, May 20, 2022 at 4:52 PM Guenter Roeck <[email protected]> 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 <[email protected]>
> > Reported-by: Guenter Roeck <[email protected]>
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> Reviewed-by: Guenter Roeck <[email protected]>
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 -- [email protected]
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
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 <[email protected]>
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> 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
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 <[email protected]>
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 <[email protected]>
>> Reported-by: Guenter Roeck <[email protected]>
>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>
> Reviewed-by: Guenter Roeck <[email protected]>
>
>> ---
>> 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
On 5/20/22 08:07, Geert Uytterhoeven wrote:
> Hi Günter,
>
> On Fri, May 20, 2022 at 4:52 PM Guenter Roeck <[email protected]> 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 <[email protected]>
>>> Reported-by: Guenter Roeck <[email protected]>
>>> Signed-off-by: Geert Uytterhoeven <[email protected]>
>>
>> Reviewed-by: Guenter Roeck <[email protected]>
>
> 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)