[PATCH] sh_eth: move inline functions to the header file

[PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

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 <sergei.shtylyov@cogentembedded.com>

---
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,

[PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

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 <sergei.shtylyov@cogentembedded.com>

---
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,

Re: [PATCH] sh_eth: move inline functions to the header file

[Geert Uytterhoeven]

Hi Sergei,

On Sun, Apr 5, 2015 at 11:13 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> 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.

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

That change is fine for me.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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] sh_eth: move inline functions to the header file

[Geert Uytterhoeven]

Hi Sergei,

On Sun, Apr 5, 2015 at 11:13 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> 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.

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

That change is fine for me.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

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] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 04:21 PM, Geert Uytterhoeven wrote:

>> 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.

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

    No.

> 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.

[...]

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

> Gr{oetje,eeting}s,
>                          Geert

WBR, Sergei

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 04:21 PM, Geert Uytterhoeven wrote:

>> 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.

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

    No.

> 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.

[...]

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

> Gr{oetje,eeting}s,
>                          Geert

WBR, Sergei

Re: [PATCH] sh_eth: move inline functions to the header file

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 06 Apr 2015 00:13:46 +0300

> 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 <sergei.shtylyov@cogentembedded.com>

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

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

Re: [PATCH] sh_eth: move inline functions to the header file

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 06 Apr 2015 00:13:46 +0300

> 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 <sergei.shtylyov@cogentembedded.com>

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

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

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:22 PM, David Miller wrote:

>> 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 <sergei.shtylyov@cogentembedded.com>

> 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

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:22 PM, David Miller wrote:

>> 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 <sergei.shtylyov@cogentembedded.com>

> 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

Re: [PATCH] sh_eth: move inline functions to the header file

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Tue, 07 Apr 2015 22:26:23 +0300

> Hello.
> 
> On 04/07/2015 10:22 PM, David Miller wrote:
> 
>>> 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 <sergei.shtylyov@cogentembedded.com>
> 
>> 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.

Re: [PATCH] sh_eth: move inline functions to the header file

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Tue, 07 Apr 2015 22:26:23 +0300

> Hello.
> 
> On 04/07/2015 10:22 PM, David Miller wrote:
> 
>>> 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 <sergei.shtylyov@cogentembedded.com>
> 
>> 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.

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:33 PM, David Miller wrote:

>>>> 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 <sergei.shtylyov@cogentembedded.com>
>>
>>> 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

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:33 PM, David Miller wrote:

>>>> 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 <sergei.shtylyov@cogentembedded.com>
>>
>>> 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

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:33 PM, David Miller wrote:

>>>> 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 <sergei.shtylyov@cogentembedded.com>
>>
>>> 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

Re: [PATCH] sh_eth: move inline functions to the header file

[Sergei Shtylyov]

Hello.

On 04/07/2015 10:33 PM, David Miller wrote:

>>>> 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 <sergei.shtylyov@cogentembedded.com>
>>
>>> 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