sh_eth: move inline functions to the header file

PATCH In review Sergei Shtylyov 3 participants 8 messages Apr 5, 2015 – Dec 12, 2015

Subsystems (3)

Networking Network drivers SuperH

lore

Share & tools

lore

Sergei Shtylyov

author

2015-04-05 21:13 UTC #0 lore raw

PATCH 1/1 sh_eth: move inline functions to the header file

The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.

While at it, make these functions return 'u32' instead of '__u32'.

Signed-off-by Sergei Shtylyov <[email protected]>

---
The patch is against the David Miller's 'net-next.git' repo.
I was going to let gcc figure out whether inlining was needed but it turned out that with *inline* keywords removed gcc generated even more code...

 drivers/net/ethernet/renesas/sh_eth.c |   24 ------------------------
 drivers/net/ethernet/renesas/sh_eth.h |   23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 24 deletions(-)

Index: net-next/drivers/net/ethernet/renesas/sh_eth.c =================================--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c +++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -941,30 +941,6 @@ static void sh_eth_set_receive_align(str
skb_reserve(skb, SH_ETH_RX_ALIGN - reserve);
}

-
-/* CPU <-> EDMAC endian convert */
-static inline __u32 cpu_to_edmac(struct sh_eth_private *mdp, u32 x)
-{
- switch (mdp->edmac_endian) {
- case EDMAC_LITTLE_ENDIAN:
- return cpu_to_le32(x);
- case EDMAC_BIG_ENDIAN:
- return cpu_to_be32(x);
- }
- return x;
-}
-
-static inline __u32 edmac_to_cpu(struct sh_eth_private *mdp, u32 x)
-{
- switch (mdp->edmac_endian) {
- case EDMAC_LITTLE_ENDIAN:
- return le32_to_cpu(x);
- case EDMAC_BIG_ENDIAN:
- return be32_to_cpu(x);
- }
- return x;
-}
-
/* Program the hardware MAC address from dev->dev_addr. */
static void update_mac_address(struct net_device *ndev)
{
Index: net-next/drivers/net/ethernet/renesas/sh_eth.h =================================--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.h +++ net-next/drivers/net/ethernet/renesas/sh_eth.h
@@ -546,6 +546,29 @@ static inline void sh_eth_soft_swap(char
#endif
}

+/* CPU <-> EDMAC endian convert */
+static inline u32 cpu_to_edmac(struct sh_eth_private *mdp, u32 x) +{
+ switch (mdp->edmac_endian) {
+ case EDMAC_LITTLE_ENDIAN:
+ return cpu_to_le32(x);
+ case EDMAC_BIG_ENDIAN:
+ return cpu_to_be32(x);
+ }
+ return x;
+}
+
+static inline u32 edmac_to_cpu(struct sh_eth_private *mdp, u32 x) +{
+ switch (mdp->edmac_endian) {
+ case EDMAC_LITTLE_ENDIAN:
+ return le32_to_cpu(x);
+ case EDMAC_BIG_ENDIAN:
+ return be32_to_cpu(x);
+ }
+ return x;
+}
+
#define SH_ETH_OFFSET_INVALID ((u16) ~0)

static inline void sh_eth_write(struct net_device *ndev, u32 data,

Geert Uytterhoeven

2015-04-07 13:21 UTC #1 lore raw

Hi Sergei,

Quoted 2 lines · Sergei Shtylyov

The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.

Why do they belong in the header file? Are they (planned to be) used by another source file (EtherAVB?)?

Currently no other source file besides sh_eth.c includes sh_eth.h, so IMHO sh_eth.h could just be absorbed by sh_eth.c.

Quoted 1 line

While at it, make these functions return 'u32' instead of '__u32'.

That change is fine for me.

Quoted 1 line

Signed-off-by Sergei Shtylyov <[email protected]>

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

Sergei Shtylyov

author

2015-04-07 17:21 UTC #2 lore raw

Hello.

Quoted 4 lines · Geert Uytterhoeven

Quoted 2 lines
The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
Why do they belong in the header file?

Because they're explicitly *inline*. The functions in the .c file
shouldn't have the *inline* keyword, DaveM wants us to rely on gcc's judgment.

Quoted 2 lines

Are they (planned to be) used by
another source file (EtherAVB?)?

No.

Quoted 2 lines

Currently no other source file besides sh_eth.c includes sh_eth.h, so IMHO sh_eth.h could just be absorbed by sh_eth.c.

I agree, in principle.

[...]

Quoted 4 lines

Quoted 1 line
Signed-off-by Sergei Shtylyov <[email protected]>
Gr{oetje,eeting}s,
Geert

WBR, Sergei

David Miller

maintainer

2015-04-07 19:22 UTC #3 lore raw

In reply to Sergei Shtylyov

From: Sergei Shtylyov <[email protected]>
Date: Mon, 06 Apr 2015 00:13:46 +0300

Quoted 6 lines

The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
While at it, make these functions return 'u32' instead of '__u32'.
Signed-off-by Sergei Shtylyov <[email protected]>

Then... ummm... remove the inline keyword?

If it's not used anywhere else, that's the thing to do.

Sergei Shtylyov

author

2015-04-07 19:26 UTC #4 lore raw

Hello.

Quoted 10 lines · David Miller

Quoted 6 lines
The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
While at it, make these functions return 'u32' instead of '__u32'.
Signed-off-by Sergei Shtylyov <[email protected]>
Then... ummm... remove the inline keyword?
If it's not used anywhere else, that's the thing to do.

Results in more code, as I noted in the patch posting that you skipped
when replying. At least with gcc 4.7.

WBR, Sergei

David Miller

maintainer

2015-04-07 19:33 UTC #5 lore raw

From: Sergei Shtylyov <[email protected]>
Date: Tue, 07 Apr 2015 22:26:23 +0300

Quoted 9 lines

Hello.
Quoted 4 lines · David Miller
Quoted 4 lines
The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
While at it, make these functions return 'u32' instead of '__u32'.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Sergei Shtylyov <[email protected]>
Then... ummm... remove the inline keyword?
If it's not used anywhere else, that's the thing to do.
Results in more code, as I noted in the patch posting that you skipped when replying. At least with gcc 4.7.

That's a compiler bug.

Sergei Shtylyov

author

2015-10-30 23:09 UTC #6 lore raw

Hello.

Quoted 7 lines · David Miller

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
While at it, make these functions return 'u32' instead of '__u32'.
Signed-off-by Sergei Shtylyov <[email protected]>

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Then... ummm... remove the inline keyword?
If it's not used anywhere else, that's the thing to do.
Results in more code, as I noted in the patch posting that you skipped when replying. At least with gcc 4.7.
That's a compiler bug.

Even worse, these functions support the BE mode which never was used and
never worked correctly. I'll just try to remove them altogether.

MBR, Sergei

Sergei Shtylyov

author

2015-12-12 22:24 UTC #7 lore raw

In reply to David Miller

Hello.

Quoted 7 lines · David Miller

Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
The explicitly inline functions belong to the header files, so move cpu_to_edmac() and edmac_to_cpu() into the driver header.
While at it, make these functions return 'u32' instead of '__u32'.
Signed-off-by Sergei Shtylyov <[email protected]>

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Then... ummm... remove the inline keyword?
If it's not used anywhere else, that's the thing to do.
Results in more code, as I noted in the patch posting that you skipped when replying. At least with gcc 4.7.
That's a compiler bug.

Just tried x86 gcc 5.1.1, same result:

$ size drivers/net/ethernet/renesas/sh_eth.o{~,}
    text	   data	    bss	    dec	    hex	filename
   23245	   1188	      0	  24433	   5f71	drivers/net/ethernet/renesas/sh_eth.o~
   23259	   1188	      0	  24447	   5f7f	drivers/net/ethernet/renesas/sh_eth.o

i.e. +14 bytes of code. :-O

MBR, Sergei