| Message ID | 20210223112003.2223332-1-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | net: dsa: sja1105: Remove unneeded cast in sja1105_crc32() | expand |
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
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
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;
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(-)