| Message ID | 4254119.qJUdUQCUqs@wasted.cogentembedded.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html