[PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

[PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

[Geert Uytterhoeven]

sja1105_unpack() takes a "const void *buf" as its first parameter, so
there is no need to cast away the "const" of the "buf" variable before
calling it.

Drop the cast, as it prevents the compiler performing some checks.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested only.

BTW, sja1105_packing() and packing() are really bad APIs, as the input
pointer parameters cannot be const due to the direction depending on
"op".  This means the compiler cannot do const checks.  Worse, callers
are required to cast away constness to prevent the compiler from
issueing warnings.  Please don't do this!
---
 drivers/net/dsa/sja1105/sja1105_static_config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/sja1105/sja1105_static_config.c b/drivers/net/dsa/sja1105/sja1105_static_config.c
index 139b7b4fbd0d5252..a8efb7fac3955307 100644
--- a/drivers/net/dsa/sja1105/sja1105_static_config.c
+++ b/drivers/net/dsa/sja1105/sja1105_static_config.c
@@ -85,7 +85,7 @@ u32 sja1105_crc32(const void *buf, size_t len)
 	/* seed */
 	crc = ~0;
 	for (i = 0; i < len; i += 4) {
-		sja1105_unpack((void *)buf + i, &word, 31, 0, 4);
+		sja1105_unpack(buf + i, &word, 31, 0, 4);
 		crc = crc32_le(crc, (u8 *)&word, 4);
 	}
 	return ~crc;
-- 
2.25.1

Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

[Vladimir Oltean]

On Tue, Feb 23, 2021 at 12:20:03PM +0100, Geert Uytterhoeven wrote:
> sja1105_unpack() takes a "const void *buf" as its first parameter, so
> there is no need to cast away the "const" of the "buf" variable before
> calling it.
> 
> Drop the cast, as it prevents the compiler performing some checks.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

By the way, your email went straight to my spam box, I just found the
patch by mistake on patchwork.

    Why is this message in spam?
    It is in violation of Google's recommended email sender guidelines.

> Compile-tested only.
> 
> BTW, sja1105_packing() and packing() are really bad APIs, as the input
> pointer parameters cannot be const due to the direction depending on
> "op".  This means the compiler cannot do const checks.  Worse, callers
> are required to cast away constness to prevent the compiler from
> issueing warnings.  Please don't do this!
> ---

What const checks can the compiler not do?
Also, if you know of an existing kernel API which can replace packing(),
I'm all ears.

Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

[Geert Uytterhoeven]

Hi Vladimir,

On Wed, Feb 24, 2021 at 11:44 PM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Tue, Feb 23, 2021 at 12:20:03PM +0100, Geert Uytterhoeven wrote:
> > sja1105_unpack() takes a "const void *buf" as its first parameter, so
> > there is no need to cast away the "const" of the "buf" variable before
> > calling it.
> >
> > Drop the cast, as it prevents the compiler performing some checks.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Thanks!

> By the way, your email went straight to my spam box, I just found the
> patch by mistake on patchwork.
>
>     Why is this message in spam?
>     It is in violation of Google's recommended email sender guidelines.

Yeah, sometimes Gmail can be annoying.  I recommend adding a filter
to never send emails with "PATCH" in the subject to spam.

> > Compile-tested only.
> >
> > BTW, sja1105_packing() and packing() are really bad APIs, as the input
> > pointer parameters cannot be const due to the direction depending on
> > "op".  This means the compiler cannot do const checks.  Worse, callers
> > are required to cast away constness to prevent the compiler from
> > issueing warnings.  Please don't do this!
> > ---
>
> What const checks can the compiler not do?

If you have a const and a non-const buffer, and accidentally call
packing() with the two buffer pointers exchanged (this is a common
mistake), you won't get a compiler warning.
So having separate pack() and unpack() functions would be safer.
You can rename packing() to __packing() to make it clear this function
is not to be called directly without deep consideration, and have
pack() and unpack() as wrappers just calling __packing().
Of course that means callers that do need a separate "op" parameter
still need to call __packing(), but they can provide their own safer
wrappers, too.

> Also, if you know of an existing kernel API which can replace packing(),
> I'm all ears.

No idea.

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] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Tue, 23 Feb 2021 12:20:03 +0100 you wrote:
> sja1105_unpack() takes a "const void *buf" as its first parameter, so
> there is no need to cast away the "const" of the "buf" variable before
> calling it.
> 
> Drop the cast, as it prevents the compiler performing some checks.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> [...]

Here is the summary with links:
  - net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()
    https://git.kernel.org/netdev/net/c/fcd4ba3bcba7

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html