[PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

In commit 6a4e812 (net/mlx4_en: Avoid calling bswap in tx fast path), we store
doorbell_qpn in big endian to avoid bswap(). Then we try to write it directly
by iowrite32() instead of iowrite32be().

This works fine on little endian platform, while has some problem on big
endian platform. Here is the definition in general:

	#define iowrite32(v, addr)	writel((v), (addr))
	#define writel(b,addr)		__raw_writel(__cpu_to_le32(b),addr)

On little endian platform, the value is not swapped before write. While on big
endian platform, the value is swapped. This is not expected to happen.

This patch does the swap on big endian platform before it is written.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
CC: Eric Dumazet <edumazet@google.com>
CC: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index 454d9fe..76f028b 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -954,7 +954,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
 		tx_desc->ctrl.owner_opcode = op_own;
 		if (send_doorbell) {
 			wmb();
-			iowrite32(ring->doorbell_qpn,
+			iowrite32(__cpu_to_le32(ring->doorbell_qpn),
 				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
 		} else {
 			ring->xmit_more++;
-- 
1.7.9.5

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

BTW, I have one confusion in commit 6a4e812,

        if (ring->bf_enabled && desc_size <= MAX_BF && !bounce &&
            !vlan_tx_tag_present(skb) && send_doorbell) {
-               tx_desc->ctrl.bf_qpn |= cpu_to_be32(ring->doorbell_qpn);
+               tx_desc->ctrl.bf_qpn = ring->doorbell_qpn |
+                                      cpu_to_be32(real_size);

I am curious about why it adds an | with the real_size?

I see this code pass is invoked in vlan network. I am not farmiliar with
the vlan test. I guess it needs some router with vlan configuration. If you
have some suggestion or link on how to test it, I am happy to have a try.

On Sun, Nov 30, 2014 at 10:43:51AM +0800, Wei Yang wrote:
>In commit 6a4e812 (net/mlx4_en: Avoid calling bswap in tx fast path), we store
>doorbell_qpn in big endian to avoid bswap(). Then we try to write it directly
>by iowrite32() instead of iowrite32be().
>
>This works fine on little endian platform, while has some problem on big
>endian platform. Here is the definition in general:
>
>	#define iowrite32(v, addr)	writel((v), (addr))
>	#define writel(b,addr)		__raw_writel(__cpu_to_le32(b),addr)
>
>On little endian platform, the value is not swapped before write. While on big
>endian platform, the value is swapped. This is not expected to happen.
>
>This patch does the swap on big endian platform before it is written.
>
>Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>CC: Eric Dumazet <edumazet@google.com>
>CC: Amir Vadai <amirv@mellanox.com>
>---
> drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>index 454d9fe..76f028b 100644
>--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>@@ -954,7 +954,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
> 		tx_desc->ctrl.owner_opcode = op_own;
> 		if (send_doorbell) {
> 			wmb();
>-			iowrite32(ring->doorbell_qpn,
>+			iowrite32(__cpu_to_le32(ring->doorbell_qpn),
> 				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
> 		} else {
> 			ring->xmit_more++;
>-- 
>1.7.9.5

-- 
Richard Yang
Help you, Help me

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Eric Dumazet]

On Sun, 2014-11-30 at 10:43 +0800, Wei Yang wrote:
> In commit 6a4e812 (net/mlx4_en: Avoid calling bswap in tx fast path), we store
> doorbell_qpn in big endian to avoid bswap(). Then we try to write it directly
> by iowrite32() instead of iowrite32be().
> 
> This works fine on little endian platform, while has some problem on big
> endian platform. Here is the definition in general:
> 
> 	#define iowrite32(v, addr)	writel((v), (addr))
> 	#define writel(b,addr)		__raw_writel(__cpu_to_le32(b),addr)

Oh well...

> 
> On little endian platform, the value is not swapped before write. While on big
> endian platform, the value is swapped. This is not expected to happen.
> 
> This patch does the swap on big endian platform before it is written.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Amir Vadai <amirv@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index 454d9fe..76f028b 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -954,7 +954,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>  		tx_desc->ctrl.owner_opcode = op_own;
>  		if (send_doorbell) {
>  			wmb();
> -			iowrite32(ring->doorbell_qpn,
> +			iowrite32(__cpu_to_le32(ring->doorbell_qpn),
>  				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
>  		} else {
>  			ring->xmit_more++;

Well, whole idea of the patch was to perform the swap (if any) out of
the fast path.

ring->doorbell_qpn is a cache of whatever value.

Have you tried to convert it to a __le32 instead of __be32 ?

Thanks

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

On Sun, Nov 30, 2014 at 11:27:48AM -0800, Eric Dumazet wrote:
>On Sun, 2014-11-30 at 10:43 +0800, Wei Yang wrote:
>> In commit 6a4e812 (net/mlx4_en: Avoid calling bswap in tx fast path), we store
>> doorbell_qpn in big endian to avoid bswap(). Then we try to write it directly
>> by iowrite32() instead of iowrite32be().
>> 
>> This works fine on little endian platform, while has some problem on big
>> endian platform. Here is the definition in general:
>> 
>> 	#define iowrite32(v, addr)	writel((v), (addr))
>> 	#define writel(b,addr)		__raw_writel(__cpu_to_le32(b),addr)
>
>Oh well...
>
>> 
>> On little endian platform, the value is not swapped before write. While on big
>> endian platform, the value is swapped. This is not expected to happen.
>> 
>> This patch does the swap on big endian platform before it is written.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> CC: Eric Dumazet <edumazet@google.com>
>> CC: Amir Vadai <amirv@mellanox.com>
>> ---
>>  drivers/net/ethernet/mellanox/mlx4/en_tx.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index 454d9fe..76f028b 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -954,7 +954,7 @@ netdev_tx_t mlx4_en_xmit(struct sk_buff *skb, struct net_device *dev)
>>  		tx_desc->ctrl.owner_opcode = op_own;
>>  		if (send_doorbell) {
>>  			wmb();
>> -			iowrite32(ring->doorbell_qpn,
>> +			iowrite32(__cpu_to_le32(ring->doorbell_qpn),
>>  				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
>>  		} else {
>>  			ring->xmit_more++;
>
>Well, whole idea of the patch was to perform the swap (if any) out of
>the fast path.

Yes, I know the it.

>
>ring->doorbell_qpn is a cache of whatever value.
>
>Have you tried to convert it to a __le32 instead of __be32 ?
>

You mean change the type of doorbell_qpn or the function __cpu_to_be32()?
I don't get your point.

>Thanks
>

-- 
Richard Yang
Help you, Help me

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[David Miller]

From: Wei Yang <weiyang@linux.vnet.ibm.com>
Date: Sun, 30 Nov 2014 10:43:51 +0800

> In commit 6a4e812 (net/mlx4_en: Avoid calling bswap in tx fast path), we store
> doorbell_qpn in big endian to avoid bswap(). Then we try to write it directly
> by iowrite32() instead of iowrite32be().
> 
> This works fine on little endian platform, while has some problem on big
> endian platform. Here is the definition in general:
> 
> 	#define iowrite32(v, addr)	writel((v), (addr))
> 	#define writel(b,addr)		__raw_writel(__cpu_to_le32(b),addr)
> 
> On little endian platform, the value is not swapped before write. While on big
> endian platform, the value is swapped. This is not expected to happen.
> 
> This patch does the swap on big endian platform before it is written.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>

Guys, let's figure out what we are doing with this patch.

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Eric Dumazet]

On Fri, 2014-12-05 at 21:31 -0800, David Miller wrote:

> Guys, let's figure out what we are doing with this patch.
> --

Oh well, patch is fine, please apply it, thanks !

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

On Fri, Dec 05, 2014 at 09:46:41PM -0800, Eric Dumazet wrote:
>On Fri, 2014-12-05 at 21:31 -0800, David Miller wrote:
>
>> Guys, let's figure out what we are doing with this patch.
>> --
>
>Oh well, patch is fine, please apply it, thanks !

Thanks :-)

>
>

-- 
Richard Yang
Help you, Help me

RE: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[David Laight]

From: Eric Dumazet
> On Fri, 2014-12-05 at 21:31 -0800, David Miller wrote:
> 
> > Guys, let's figure out what we are doing with this patch.
> > --
> 
> Oh well, patch is fine, please apply it, thanks !

I'm not to sure that the patch doesn't generate a software byteswap
followed by a byteswapping write on ppc - clearly not ideal.
It might even generate back to back software byteswaps.

If the write to the doorbell register includes a byteswap on BE (ppc)
then there is no real value in keeping the value as BE.

OTOH ppc ought to have ways of doing IO writes without the byteswap
(and byteswapping accesses to non-io memory for that matter).

What happens on a BE system with BE peripherals is another matter.

	David

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

On Mon, Dec 08, 2014 at 10:00:19AM +0000, David Laight wrote:
>From: Eric Dumazet
>> On Fri, 2014-12-05 at 21:31 -0800, David Miller wrote:
>> 
>> > Guys, let's figure out what we are doing with this patch.
>> > --
>> 
>> Oh well, patch is fine, please apply it, thanks !
>
>I'm not to sure that the patch doesn't generate a software byteswap
>followed by a byteswapping write on ppc - clearly not ideal.
>It might even generate back to back software byteswaps.
>
>If the write to the doorbell register includes a byteswap on BE (ppc)
>then there is no real value in keeping the value as BE.
>
>OTOH ppc ought to have ways of doing IO writes without the byteswap
>(and byteswapping accesses to non-io memory for that matter).
>
>What happens on a BE system with BE peripherals is another matter.

David

Thanks for your comment.

How about use __raw_writel() to replace the iowrite32()? Looks this is better,
if so, I will make up another version for this.

>
>	David
>

-- 
Richard Yang
Help you, Help me

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

On Mon, Dec 08, 2014 at 10:42:37PM +0800, Wei Yang wrote:
>On Mon, Dec 08, 2014 at 10:00:19AM +0000, David Laight wrote:
>>From: Eric Dumazet
>>> On Fri, 2014-12-05 at 21:31 -0800, David Miller wrote:
>>> 
>>> > Guys, let's figure out what we are doing with this patch.
>>> > --
>>> 
>>> Oh well, patch is fine, please apply it, thanks !
>>
>>I'm not to sure that the patch doesn't generate a software byteswap
>>followed by a byteswapping write on ppc - clearly not ideal.
>>It might even generate back to back software byteswaps.
>>
>>If the write to the doorbell register includes a byteswap on BE (ppc)
>>then there is no real value in keeping the value as BE.
>>
>>OTOH ppc ought to have ways of doing IO writes without the byteswap
>>(and byteswapping accesses to non-io memory for that matter).
>>
>>What happens on a BE system with BE peripherals is another matter.
>
>David
>
>Thanks for your comment.
>
>How about use __raw_writel() to replace the iowrite32()? Looks this is better,
>if so, I will make up another version for this.
>

Hi, David

If you prefer this way, I would like to send a new version for this.
Is it ok for you?

>>
>>	David
>>
>
>-- 
>Richard Yang
>Help you, Help me

-- 
Richard Yang
Help you, Help me

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[David Miller]

From: Wei Yang <weiyang@linux.vnet.ibm.com>
Date: Sat, 13 Dec 2014 11:13:38 +0800

> On Mon, Dec 08, 2014 at 10:42:37PM +0800, Wei Yang wrote:
> If you prefer this way, I would like to send a new version for this.
> Is it ok for you?

I'm not so sure.  There are implications when using the __raw_*()
routines.

In particular, using __raw_{read,write}l() also means that the usual
necessary I/O memory barriers are not being performed.

There are therefore no ordering guarantees between __raw_*() and other
I/O or memory accesses whatsoever.

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Wei Yang]

On Sat, Dec 13, 2014 at 11:43:20PM -0500, David Miller wrote:
>From: Wei Yang <weiyang@linux.vnet.ibm.com>
>Date: Sat, 13 Dec 2014 11:13:38 +0800
>
>> On Mon, Dec 08, 2014 at 10:42:37PM +0800, Wei Yang wrote:
>> If you prefer this way, I would like to send a new version for this.
>> Is it ok for you?
>
>I'm not so sure.  There are implications when using the __raw_*()
>routines.
>
>In particular, using __raw_{read,write}l() also means that the usual
>necessary I/O memory barriers are not being performed.
>
>There are therefore no ordering guarantees between __raw_*() and other
>I/O or memory accesses whatsoever.

Thanks David.

Actually, the last mail is asking David Laight. I am trying to understanding
his comment and Amir told me he was suggesting to use __raw_*() version.

Hmm... this is really a problem found in the v3.18-rc1 and the root cause is
the endianess. I am ok to use any method to fix this problem, even revert it.
Could the maintainer from Mellanox gives me a word?

-- 
Richard Yang
Help you, Help me

Re: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[Amir Vadai]

On 12/15/2014 3:32 AM, Wei Yang wrote:
[...]

Hi David's,

I need to do a native endianness write to the NIC register (the write is
on the fast path and the register value could be calculated once).

iowrite32be() is calling cpu_to_be32() on the value.
iowrite32() is calling cpu_to_le32().

I thought about using raw_writel() but as David Miller said, it lacks
necessary io barriers on some archs.

Does the only solution to this, is to add some sort of
iowrite32be_native(__be32 val,...) function to all the archs?

Thanks,
Amir

RE: [PATCH net] net/mlx4_en: correct the endianness of doorbell_qpn on big endian platform

[David Laight]

From: Wei Yang
> On Sat, Dec 13, 2014 at 11:43:20PM -0500, David Miller wrote:
> >From: Wei Yang <weiyang@linux.vnet.ibm.com>
> >Date: Sat, 13 Dec 2014 11:13:38 +0800
> >
> >> On Mon, Dec 08, 2014 at 10:42:37PM +0800, Wei Yang wrote:
> >> If you prefer this way, I would like to send a new version for this.
> >> Is it ok for you?
> >
> >I'm not so sure.  There are implications when using the __raw_*()
> >routines.
> >
> >In particular, using __raw_{read,write}l() also means that the usual
> >necessary I/O memory barriers are not being performed.
> >
> >There are therefore no ordering guarantees between __raw_*() and other
> >I/O or memory accesses whatsoever.
> 
> Thanks David.
> 
> Actually, the last mail is asking David Laight. I am trying to understanding
> his comment and Amir told me he was suggesting to use __raw_*() version.

I don't know (off hand) which of the access functions contain byteswaps and
memory barriers, from trying to get some drivers running on ppc I know the
whole thing is a complete mess (beats me why all the on-chip devices of
opposite endianness to the cpu iteself).

In this case you seem to have an earlier change to store the value that
doesn't need a byteswap on write (for efficiency), then are using an
access function that does a byteswapping write to memory (efficient, but
the wrong value), and are fixing it by adding another byteswap (inefficient)
prior to the byteswapping write.

So you should either be using a non-byteswapping write, of just save
the byteswapped value so that the written value is correct.

	David

> Hmm... this is really a problem found in the v3.18-rc1 and the root cause is
> the endianess. I am ok to use any method to fix this problem, even revert it.
> Could the maintainer from Mellanox gives me a word?
> 
> --
> Richard Yang
> Help you, Help me