Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()
@ 2021-02-23 11:20 Geert Uytterhoeven
  2021-02-24 22:43 ` Vladimir Oltean
  2021-02-25 17:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 4+ messages in thread
From: Geert Uytterhoeven @ 2021-02-23 11:20 UTC (permalink / raw)
  To: Vladimir Oltean, Andrew Lunn, Vivien Didelot, Florian Fainelli,
	David S . Miller, Jakub Kicinski
  Cc: netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()
  2021-02-23 11:20 [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32() Geert Uytterhoeven
@ 2021-02-24 22:43 ` Vladimir Oltean
  2021-02-25  7:28   ` Geert Uytterhoeven
  2021-02-25 17:50 ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2021-02-24 22:43 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
	Jakub Kicinski, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()
  2021-02-24 22:43 ` Vladimir Oltean
@ 2021-02-25  7:28   ` Geert Uytterhoeven
  0 siblings, 0 replies; 4+ messages in thread
From: Geert Uytterhoeven @ 2021-02-25  7:28 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S . Miller,
	Jakub Kicinski, netdev, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32()
  2021-02-23 11:20 [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32() Geert Uytterhoeven
  2021-02-24 22:43 ` Vladimir Oltean
@ 2021-02-25 17:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-02-25 17:50 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: olteanv, andrew, vivien.didelot, f.fainelli, davem, kuba, netdev,
	linux-kernel

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



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 11:20 [PATCH] net: dsa: sja1105: Remove unneeded cast in sja1105_crc32() Geert Uytterhoeven
2021-02-24 22:43 ` Vladimir Oltean
2021-02-25  7:28   ` Geert Uytterhoeven
2021-02-25 17:50 ` patchwork-bot+netdevbpf

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git