[PATCH 0/3] sh_eth: fix & clean up sh_eth_soft_swap()

[PATCH 0/3] sh_eth: fix & clean up sh_eth_soft_swap()

[Sergei Shtylyov]

Hello!

Here's a set of 3 patches against DaveM's 'net-next.git' repo. First one fixes an
old buffer endiannes issue (luckily, the ARM SoCs are smart enough to not actually
care) plus couple clean ups around sh_eth_soft_swap()...

[1/1] sh_eth: make sh_eth_soft_swap() work on ARM
[2/3] sh_eth: uninline sh_eth_soft_swap()
[3/3] sh_eth: use DIV_ROUND_UP() in sh_eth_soft_swap()

MBR, Sergei

[PATCH 1/3] sh_eth: make sh_eth_soft_swap() work on ARM

[Sergei Shtylyov]

Browsing  thru the driver disassembly, I noticed that ARM gcc generated
no  code  whatsoever for sh_eth_soft_swap() while building a little-endian
kernel -- apparently __LITTLE_ENDIAN__ was not being #define'd, however
it got implicitly #define'd when building with the SH gcc (I could only
find the explicit #define __LITTLE_ENDIAN that was #include'd when building
a little-endian kernel).  Luckily, the Ether controller  only doing big-
endian DMA is encountered on the early SH771x SoCs only and all ARM SoCs
implement EDMR.DE and thus set 'sh_eth_cpu_data::hw_swap'. But anyway, we
need to fix the #ifdef inside sh_eth_soft_swap() to something that would
work on all architectures... 

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

---
 drivers/net/ethernet/renesas/sh_eth.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
@@ -562,7 +562,7 @@ struct sh_eth_private {
 
 static inline void sh_eth_soft_swap(char *src, int len)
 {
-#ifdef __LITTLE_ENDIAN__
+#ifdef __LITTLE_ENDIAN
 	u32 *p = (u32 *)src;
 	u32 *maxp;
 	maxp = p + ((len + sizeof(u32) - 1) / sizeof(u32));

Re: [PATCH 1/3] sh_eth: make sh_eth_soft_swap() work on ARM

[Geert Uytterhoeven]

On Sat, Jun 2, 2018 at 9:37 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Browsing  thru the driver disassembly, I noticed that ARM gcc generated
> no  code  whatsoever for sh_eth_soft_swap() while building a little-endian
> kernel -- apparently __LITTLE_ENDIAN__ was not being #define'd, however
> it got implicitly #define'd when building with the SH gcc (I could only
> find the explicit #define __LITTLE_ENDIAN that was #include'd when building
> a little-endian kernel).  Luckily, the Ether controller  only doing big-
> endian DMA is encountered on the early SH771x SoCs only and all ARM SoCs
> implement EDMR.DE and thus set 'sh_eth_cpu_data::hw_swap'. But anyway, we
> need to fix the #ifdef inside sh_eth_soft_swap() to something that would
> work on all architectures...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 1/3] sh_eth: make sh_eth_soft_swap() work on ARM

[Geert Uytterhoeven]

On Mon, Jun 4, 2018 at 10:48 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sat, Jun 2, 2018 at 9:37 PM, Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com> wrote:
>> Browsing  thru the driver disassembly, I noticed that ARM gcc generated
>> no  code  whatsoever for sh_eth_soft_swap() while building a little-endian
>> kernel -- apparently __LITTLE_ENDIAN__ was not being #define'd, however
>> it got implicitly #define'd when building with the SH gcc (I could only
>> find the explicit #define __LITTLE_ENDIAN that was #include'd when building
>> a little-endian kernel).  Luckily, the Ether controller  only doing big-
>> endian DMA is encountered on the early SH771x SoCs only and all ARM SoCs
>> implement EDMR.DE and thus set 'sh_eth_cpu_data::hw_swap'. But anyway, we
>> need to fix the #ifdef inside sh_eth_soft_swap() to something that would
>> work on all architectures...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Forgot to say: nice catch ;-)

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

[PATCH 2/3] sh_eth: uninline sh_eth_soft_swap()

[Sergei Shtylyov]

sh_eth_tsu_soft_swap() is called twice by the driver, remove *inline* and
move  that function  from the header to the driver itself to let gcc decide
whether to expand it inline or not...

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

---
 drivers/net/ethernet/renesas/sh_eth.c |   11 +++++++++++
 drivers/net/ethernet/renesas/sh_eth.h |   12 ------------
 2 files changed, 11 insertions(+), 12 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
@@ -460,6 +460,17 @@ static u32 sh_eth_tsu_read(struct sh_eth
 	return ioread32(mdp->tsu_addr + offset);
 }
 
+static void sh_eth_soft_swap(char *src, int len)
+{
+#ifdef __LITTLE_ENDIAN
+	u32 *p = (u32 *)src;
+	u32 *maxp = p + ((len + sizeof(u32) - 1) / sizeof(u32));
+
+	for (; p < maxp; p++)
+		*p = swab32(*p);
+#endif
+}
+
 static void sh_eth_select_mii(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(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
@@ -560,18 +560,6 @@ struct sh_eth_private {
 	unsigned wol_enabled:1;
 };
 
-static inline void sh_eth_soft_swap(char *src, int len)
-{
-#ifdef __LITTLE_ENDIAN
-	u32 *p = (u32 *)src;
-	u32 *maxp;
-	maxp = p + ((len + sizeof(u32) - 1) / sizeof(u32));
-
-	for (; p < maxp; p++)
-		*p = swab32(*p);
-#endif
-}
-
 static inline void *sh_eth_tsu_get_offset(struct sh_eth_private *mdp,
 					  int enum_index)
 {

Re: [PATCH 2/3] sh_eth: uninline sh_eth_soft_swap()

[Geert Uytterhoeven]

On Sat, Jun 2, 2018 at 9:38 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> sh_eth_tsu_soft_swap() is called twice by the driver, remove *inline* and
> move  that function  from the header to the driver itself to let gcc decide
> whether to expand it inline or not...
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH 3/3] sh_eth: use DIV_ROUND_UP() in sh_eth_soft_swap()

[Sergei Shtylyov]

When initializing 'maxp' in sh_eth_soft_swap(), the buffer length needs
to be rounded  up -- that's just asking for DIV_ROUND_UP()!

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

---
 drivers/net/ethernet/renesas/sh_eth.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
@@ -464,7 +464,7 @@ static void sh_eth_soft_swap(char *src,
 {
 #ifdef __LITTLE_ENDIAN
 	u32 *p = (u32 *)src;
-	u32 *maxp = p + ((len + sizeof(u32) - 1) / sizeof(u32));
+	u32 *maxp = p + DIV_ROUND_UP(len, sizeof(u32));
 
 	for (; p < maxp; p++)
 		*p = swab32(*p);

Re: [PATCH 3/3] sh_eth: use DIV_ROUND_UP() in sh_eth_soft_swap()

[Geert Uytterhoeven]

On Sat, Jun 2, 2018 at 9:40 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> When initializing 'maxp' in sh_eth_soft_swap(), the buffer length needs
> to be rounded  up -- that's just asking for DIV_ROUND_UP()!
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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 0/3] sh_eth: fix & clean up sh_eth_soft_swap()

[Geert Uytterhoeven]

Hi Sergei,

On Sat, Jun 2, 2018 at 9:32 PM, Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Here's a set of 3 patches against DaveM's 'net-next.git' repo. First one fixes an
> old buffer endiannes issue (luckily, the ARM SoCs are smart enough to not actually
> care) plus couple clean ups around sh_eth_soft_swap()...
>
> [1/1] sh_eth: make sh_eth_soft_swap() work on ARM
> [2/3] sh_eth: uninline sh_eth_soft_swap()
> [3/3] sh_eth: use DIV_ROUND_UP() in sh_eth_soft_swap()

Does the swapping actually work?
In sh_eth_rx(), it's called before dma_unmap_single(), without calling
dma_sync_single_for_cpu() first.
Shouldn't it be called after the unmap instead?

In addition, why is it passed the dma_addr converted to virt, while the skb
address is available?

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 0/3] sh_eth: fix & clean up sh_eth_soft_swap()

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sat, 2 Jun 2018 22:32:48 +0300

> Here's a set of 3 patches against DaveM's 'net-next.git' repo. First one fixes an
> old buffer endiannes issue (luckily, the ARM SoCs are smart enough to not actually
> care) plus couple clean ups around sh_eth_soft_swap()...
> 
> [1/1] sh_eth: make sh_eth_soft_swap() work on ARM
> [2/3] sh_eth: uninline sh_eth_soft_swap()
> [3/3] sh_eth: use DIV_ROUND_UP() in sh_eth_soft_swap()

Series applied, thank you.