Re: [PATCHv2] net: moxart: use correct accessors for DMA memory

2016-01-29 Thread David Miller
From: Arnd Bergmann 
Date: Thu, 28 Jan 2016 17:54:33 +0100

> The moxart ethernet driver confuses coherent DMA buffers with
> MMIO registers.
> 
> moxart_ether.c: In function 'moxart_mac_setup_desc_ring':
> moxart_ether.c:146:428: error: passing argument 1 of '__fswab32' makes 
> integer from pointer without a cast [-Werror=int-conversion]
> moxart_ether.c:74:39: warning: incorrect type in argument 3 (different 
> address spaces)
> moxart_ether.c:74:39:expected void *cpu_addr
> moxart_ether.c:74:39:got void [noderef] *tx_desc_base
> 
> This leaves the basic logic alone and uses normal pointers for
> the virtual address of the descriptor. As we cannot use readl/writel
> to access them, we also introduce our own moxart_desc_read
> moxart_desc_write helpers that perform the same endianess swap
> as the original code, but without the address space conversion.
> 
> The barriers are made explicit here where needed: Even in the worst-case
> scenario, we just have to use a rmb() after checking ownership so
> we don't read any input data before we are sure it is value, and we
> use wmb() before transferring ownership back to the device.
> 
> Signed-off-by: Arnd Bergmann 

Applied, thanks.


[PATCHv2] net: moxart: use correct accessors for DMA memory

2016-01-28 Thread Arnd Bergmann
The moxart ethernet driver confuses coherent DMA buffers with
MMIO registers.

moxart_ether.c: In function 'moxart_mac_setup_desc_ring':
moxart_ether.c:146:428: error: passing argument 1 of '__fswab32' makes integer 
from pointer without a cast [-Werror=int-conversion]
moxart_ether.c:74:39: warning: incorrect type in argument 3 (different address 
spaces)
moxart_ether.c:74:39:expected void *cpu_addr
moxart_ether.c:74:39:got void [noderef] *tx_desc_base

This leaves the basic logic alone and uses normal pointers for
the virtual address of the descriptor. As we cannot use readl/writel
to access them, we also introduce our own moxart_desc_read
moxart_desc_write helpers that perform the same endianess swap
as the original code, but without the address space conversion.

The barriers are made explicit here where needed: Even in the worst-case
scenario, we just have to use a rmb() after checking ownership so
we don't read any input data before we are sure it is value, and we
use wmb() before transferring ownership back to the device.

Signed-off-by: Arnd Bergmann 

diff --git a/drivers/net/ethernet/moxa/moxart_ether.c 
b/drivers/net/ethernet/moxa/moxart_ether.c
index a10c928bbd6b..00cfd95ca59d 100644
--- a/drivers/net/ethernet/moxa/moxart_ether.c
+++ b/drivers/net/ethernet/moxa/moxart_ether.c
@@ -28,6 +28,16 @@
 
 #include "moxart_ether.h"
 
+static inline void moxart_desc_write(u32 data, u32 *desc)
+{
+   *desc = cpu_to_le32(data);
+}
+
+static inline u32 moxart_desc_read(u32 *desc)
+{
+   return le32_to_cpu(*desc);
+}
+
 static inline void moxart_emac_write(struct net_device *ndev,
 unsigned int reg, unsigned long value)
 {
@@ -112,7 +122,7 @@ static void moxart_mac_enable(struct net_device *ndev)
 static void moxart_mac_setup_desc_ring(struct net_device *ndev)
 {
struct moxart_mac_priv_t *priv = netdev_priv(ndev);
-   void __iomem *desc;
+   void *desc;
int i;
 
for (i = 0; i < TX_DESC_NUM; i++) {
@@ -121,7 +131,7 @@ static void moxart_mac_setup_desc_ring(struct net_device 
*ndev)
 
priv->tx_buf[i] = priv->tx_buf_base + priv->tx_buf_size * i;
}
-   writel(TX_DESC1_END, desc + TX_REG_OFFSET_DESC1);
+   moxart_desc_write(TX_DESC1_END, desc + TX_REG_OFFSET_DESC1);
 
priv->tx_head = 0;
priv->tx_tail = 0;
@@ -129,8 +139,8 @@ static void moxart_mac_setup_desc_ring(struct net_device 
*ndev)
for (i = 0; i < RX_DESC_NUM; i++) {
desc = priv->rx_desc_base + i * RX_REG_DESC_SIZE;
memset(desc, 0, RX_REG_DESC_SIZE);
-   writel(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
-   writel(RX_BUF_SIZE & RX_DESC1_BUF_SIZE_MASK,
+   moxart_desc_write(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
+   moxart_desc_write(RX_BUF_SIZE & RX_DESC1_BUF_SIZE_MASK,
   desc + RX_REG_OFFSET_DESC1);
 
priv->rx_buf[i] = priv->rx_buf_base + priv->rx_buf_size * i;
@@ -141,12 +151,12 @@ static void moxart_mac_setup_desc_ring(struct net_device 
*ndev)
if (dma_mapping_error(&ndev->dev, priv->rx_mapping[i]))
netdev_err(ndev, "DMA mapping error\n");
 
-   writel(priv->rx_mapping[i],
+   moxart_desc_write(priv->rx_mapping[i],
   desc + RX_REG_OFFSET_DESC2 + RX_DESC2_ADDRESS_PHYS);
-   writel(priv->rx_buf[i],
+   moxart_desc_write((uintptr_t)priv->rx_buf[i],
   desc + RX_REG_OFFSET_DESC2 + RX_DESC2_ADDRESS_VIRT);
}
-   writel(RX_DESC1_END, desc + RX_REG_OFFSET_DESC1);
+   moxart_desc_write(RX_DESC1_END, desc + RX_REG_OFFSET_DESC1);
 
priv->rx_head = 0;
 
@@ -201,14 +211,15 @@ static int moxart_rx_poll(struct napi_struct *napi, int 
budget)
  napi);
struct net_device *ndev = priv->ndev;
struct sk_buff *skb;
-   void __iomem *desc;
+   void *desc;
unsigned int desc0, len;
int rx_head = priv->rx_head;
int rx = 0;
 
while (rx < budget) {
desc = priv->rx_desc_base + (RX_REG_DESC_SIZE * rx_head);
-   desc0 = readl(desc + RX_REG_OFFSET_DESC0);
+   desc0 = moxart_desc_read(desc + RX_REG_OFFSET_DESC0);
+   rmb(); /* ensure desc0 is up to date */
 
if (desc0 & RX_DESC0_DMA_OWN)
break;
@@ -250,7 +261,8 @@ static int moxart_rx_poll(struct napi_struct *napi, int 
budget)
priv->stats.multicast++;
 
 rx_next:
-   writel(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
+   wmb(); /* prevent setting ownership back too early */
+   moxart_desc_write(RX_DESC0_DMA_OWN, desc + RX_REG_OFFSET_DESC0);
 
rx_head = RX_NEXT(rx_head);
priv->rx_head = rx_head;
@@ -310,7 +322,7 @@ static irqreturn_