[PATCH net] net/mlx4_en: Doorbell is byteswapped in Little Endian archs

[PATCH net] net/mlx4_en: Doorbell is byteswapped in Little Endian archs

[Amir Vadai]

iowrite32() will byteswap it's argument on big endian archs.
iowrite32be() will byteswap on little endian archs.
Since we don't want to do this unnecessary byteswap on the fast path,
doorbell is stored in the NIC's native endianness. Using the right
iowrite() according to the arch endianness.

CC: Wei Yang <weiyang@linux.vnet.ibm.com>
CC: David Laight <david.laight@aculab.com>
Fixes: 6a4e812 ("net/mlx4_en: Avoid calling bswap in tx fast path")
Signed-off-by: Amir Vadai <amirv@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
index a308d41..6477cc7 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
@@ -962,7 +962,16 @@ 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,
+		/* Since there is no iowrite*_native() that writes the value
+		 * as is, without byteswapping - using the one the doesn't do
+		 * byteswapping in the relevant arch endianness.
+		 */
+#if defined(__LITTLE_ENDIAN)
+			iowrite32(
+#else
+			iowrite32be(
+#endif
+				  ring->doorbell_qpn,
 				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
 		} else {
 			ring->xmit_more++;
-- 
1.9.3

Re: [PATCH net] net/mlx4_en: Doorbell is byteswapped in Little Endian archs

[Sergei Shtylyov]

Hello.

On 12/21/2014 9:18 PM, Amir Vadai wrote:

> iowrite32() will byteswap it's argument on big endian archs.
> iowrite32be() will byteswap on little endian archs.
> Since we don't want to do this unnecessary byteswap on the fast path,
> doorbell is stored in the NIC's native endianness. Using the right
> iowrite() according to the arch endianness.

> CC: Wei Yang <weiyang@linux.vnet.ibm.com>
> CC: David Laight <david.laight@aculab.com>
> Fixes: 6a4e812 ("net/mlx4_en: Avoid calling bswap in tx fast path")
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/en_tx.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index a308d41..6477cc7 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -962,7 +962,16 @@ 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,
> +		/* Since there is no iowrite*_native() that writes the value
> +		 * as is, without byteswapping - using the one the doesn't do
> +		 * byteswapping in the relevant arch endianness.
> +		 */

    Why the comment is not aligned with the code?

> +#if defined(__LITTLE_ENDIAN)
> +			iowrite32(
> +#else
> +			iowrite32be(
> +#endif

    Ugh...

> +				  ring->doorbell_qpn,
>   				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
[...]

WBR, Sergei

Re: [PATCH net] net/mlx4_en: Doorbell is byteswapped in Little Endian archs

[Amir Vadai]

On 12/21/2014 9:53 PM, Sergei Shtylyov wrote:
> Hello.
> 
Hi,

> On 12/21/2014 9:18 PM, Amir Vadai wrote:
> 
>> iowrite32() will byteswap it's argument on big endian archs.
>> iowrite32be() will byteswap on little endian archs.
>> Since we don't want to do this unnecessary byteswap on the fast path,
>> doorbell is stored in the NIC's native endianness. Using the right
>> iowrite() according to the arch endianness.
> 
>> CC: Wei Yang <weiyang@linux.vnet.ibm.com>
>> CC: David Laight <david.laight@aculab.com>
>> Fixes: 6a4e812 ("net/mlx4_en: Avoid calling bswap in tx fast path")
>> Signed-off-by: Amir Vadai <amirv@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx4/en_tx.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> index a308d41..6477cc7 100644
>> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
>> @@ -962,7 +962,16 @@ 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,
>> +		/* Since there is no iowrite*_native() that writes the value
>> +		 * as is, without byteswapping - using the one the doesn't do
>> +		 * byteswapping in the relevant arch endianness.
>> +		 */
> 
>     Why the comment is not aligned with the code?
By mistake. Sending V1.

> 
>> +#if defined(__LITTLE_ENDIAN)
>> +			iowrite32(
>> +#else
>> +			iowrite32be(
>> +#endif
> 
>     Ugh...
Yes I think so too, but there is no iowrite*_native(). I plan to send a
patch to add it, but meanwhile the driver is completely broken in little
endian archs - so it must be fixed now. And just reverting to the old
behavior of bswap in the fast path looks like a bad alternative too.

> 
>> +				  ring->doorbell_qpn,
>>   				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
> [...]
> 
> WBR, Sergei
> 
Amir

RE: [PATCH net] net/mlx4_en: Doorbell is byteswapped in Little Endian archs

[David Laight]

> iowrite32() will byteswap it's argument on big endian archs.
> iowrite32be() will byteswap on little endian archs.
> Since we don't want to do this unnecessary byteswap on the fast path,
> doorbell is stored in the NIC's native endianness. Using the right
> iowrite() according to the arch endianness.

Someone seems to have forgotten that the byteswap within iowrite32()
on ppc (and more recent sparc) is almost certainly 'free'.
So the value ought to be stored in the byte order required by iowrite32().

	David

> Fixes: 6a4e812 ("net/mlx4_en: Avoid calling bswap in tx fast path")
> Signed-off-by: Amir Vadai <amirv@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/en_tx.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_tx.c b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> index a308d41..6477cc7 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/en_tx.c
> @@ -962,7 +962,16 @@ 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,
> +		/* Since there is no iowrite*_native() that writes the value
> +		 * as is, without byteswapping - using the one the doesn't do
> +		 * byteswapping in the relevant arch endianness.
> +		 */
> +#if defined(__LITTLE_ENDIAN)
> +			iowrite32(
> +#else
> +			iowrite32be(
> +#endif
> +				  ring->doorbell_qpn,
>  				  ring->bf.uar->map + MLX4_SEND_DOORBELL);
>  		} else {
>  			ring->xmit_more++;
> --
> 1.9.3