Le 23/07/2021 à 22:31, Geoff Levand a écrit :
Fixes several DMA mapping problems with the PS3's gelic network driver:

  * Change from checking the return value of dma_map_single to using the
    dma_mapping_error routine.
  * Use the correct buffer length when mapping the RX skb.
  * Improved error checking and debug logging.

Fixes runtime errors like these, and also other randomly occurring errors:

   IP-Config: Complete:
   DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
   WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc

Signed-off-by: Geoff Levand <ge...@infradead.org>


CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#55: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:351:
+               descr->link.cpu_addr = dma_map_single(dev, descr,
+                       descr->link.size, DMA_BIDIRECTIONAL);

WARNING:BRACES: braces {} are not necessary for single statement blocks
#62: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:358:
+                               if (descr->link.cpu_addr) {
+                                       gelic_unmap_link(dev, descr);
+                               }

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#157: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:440:
+       cpu_addr = dma_map_single(dev, descr->skb->data,
+               descr->hw_regs.payload.size, DMA_FROM_DEVICE);

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#262: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:612:
+                       dev_info_ratelimited(dev,
+                               "%s:%d: forcing end of tx descriptor with status 
%x\n",

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#323: FILE: drivers/net/ethernet/toshiba/ps3_gelic_net.c:846:
+       cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
+               DMA_TO_DEVICE);


NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit cf6041cd6b17 ("net/ps3_gelic: Fix DMA mapping problems") has style 
problems, please review.

NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH EMAIL_SUBJECT FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS


---
  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 183 +++++++++++--------
  1 file changed, 108 insertions(+), 75 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c 
b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index 42f4de9ad5fe..11ddeacb1159 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -336,22 +336,31 @@ static int gelic_card_init_chain(struct gelic_card *card,
        struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
        int descr_count)
  {
-       int i;
-       struct gelic_descr *descr;
+       struct gelic_descr *descr = start_descr;
        struct device *dev = ctodev(card);
+       unsigned int index;
- descr = start_descr;
-       memset(descr, 0, sizeof(*descr) *descr_count);
+       memset(start_descr, 0, descr_count * sizeof(*start_descr));
- for (i = 0; i < descr_count; i++, descr++) {
-               descr->link.size = sizeof(struct gelic_hw_regs);
+       for (index = 0, descr = start_descr; index < descr_count;
+               index++, descr++) {
                gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
-               descr->link.cpu_addr =
-                       dma_map_single(dev, descr, descr->link.size,
-                               DMA_BIDIRECTIONAL);
- if (!descr->link.cpu_addr)
-                       goto iommu_error;
+               descr->link.size = sizeof(struct gelic_hw_regs);
+               descr->link.cpu_addr = dma_map_single(dev, descr,
+                       descr->link.size, DMA_BIDIRECTIONAL);
+
+               if (unlikely(dma_mapping_error(dev, descr->link.cpu_addr))) {
+                       dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
+                               __LINE__);
+
+                       for (index--, descr--; index > 0; index--, descr--) {
+                               if (descr->link.cpu_addr) {
+                                       gelic_unmap_link(dev, descr);
+                               }
+                       }
+                       return -ENOMEM;
+               }
descr->next = descr + 1;
                descr->prev = descr - 1;
@@ -360,8 +369,9 @@ static int gelic_card_init_chain(struct gelic_card *card,
        (descr - 1)->next = start_descr;
        start_descr->prev = (descr - 1);
- descr = start_descr;
-       for (i = 0; i < descr_count; i++, descr++) {
+       /* chain bus addr of hw descriptor */
+       for (index = 0, descr = start_descr; index < descr_count;
+               index++, descr++) {
                descr->hw_regs.next_descr_addr =
                        cpu_to_be32(descr->next->link.cpu_addr);
        }
@@ -373,12 +383,6 @@ static int gelic_card_init_chain(struct gelic_card *card,
        (descr - 1)->hw_regs.next_descr_addr = 0;
return 0;
-
-iommu_error:
-       for (i--, descr--; 0 <= i; i--, descr--)
-               if (descr->link.cpu_addr)
-                       gelic_unmap_link(dev, descr);
-       return -ENOMEM;
  }
/**
@@ -395,49 +399,63 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
        struct gelic_descr *descr)
  {
        struct device *dev = ctodev(card);
-       int offset;
-       unsigned int bufsize;
+       struct aligned_buff {
+               unsigned int total_bytes;
+               unsigned int offset;
+       };
+       struct aligned_buff a_buf;
+       dma_addr_t cpu_addr;
if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
                dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
        }
- /* we need to round up the buffer size to a multiple of 128 */
-       bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
+       a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
+               + GELIC_NET_RXBUF_ALIGN;
+
+       descr->skb = dev_alloc_skb(a_buf.total_bytes);
- /* and we need to have it 128 byte aligned, therefore we allocate a
-        * bit more */
-       descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
        if (!descr->skb) {
-               descr->hw_regs.payload.dev_addr = 0; /* tell DMAC don't touch 
memory */
+               descr->hw_regs.payload.dev_addr = 0;
+               descr->hw_regs.payload.size = 0;
                return -ENOMEM;
        }
-       descr->hw_regs.payload.size = cpu_to_be32(bufsize);
+
+       a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
+               - descr->skb->data;
+
+       if (a_buf.offset) {
+               dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
+                       a_buf.offset);
+               skb_reserve(descr->skb, a_buf.offset);
+       }
+
        descr->hw_regs.dmac_cmd_status = 0;
        descr->hw_regs.result_size = 0;
        descr->hw_regs.valid_size = 0;
        descr->hw_regs.data_error = 0;
- offset = ((unsigned long)descr->skb->data) &
-               (GELIC_NET_RXBUF_ALIGN - 1);
-       if (offset)
-               skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
-       /* io-mmu-map the skb */
-       descr->hw_regs.payload.dev_addr = cpu_to_be32(dma_map_single(dev,
-                                                    descr->skb->data,
-                                                    GELIC_NET_MAX_MTU,
-                                                    DMA_FROM_DEVICE));
-       if (!descr->hw_regs.payload.dev_addr) {
+       descr->hw_regs.payload.size = a_buf.total_bytes - a_buf.offset;
+       cpu_addr = dma_map_single(dev, descr->skb->data,
+               descr->hw_regs.payload.size, DMA_FROM_DEVICE);
+       descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
+
+       if (unlikely(dma_mapping_error(dev, cpu_addr))) {
+               dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
+
+               descr->hw_regs.payload.dev_addr = 0;
+               descr->hw_regs.payload.size = 0;
+
                dev_kfree_skb_any(descr->skb);
                descr->skb = NULL;
-               dev_info(dev,
-                        "%s:Could not iommu-map rx buffer\n", __func__);
+
                gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
+
                return -ENOMEM;
-       } else {
-               gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
-               return 0;
        }
+
+       gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
+       return 0;
  }
/**
@@ -454,13 +472,18 @@ static void gelic_card_release_rx_chain(struct gelic_card 
*card)
                if (descr->skb) {
                        dma_unmap_single(dev,
                                be32_to_cpu(descr->hw_regs.payload.dev_addr),
-                               descr->skb->len, DMA_FROM_DEVICE);
-                       descr->hw_regs.payload.dev_addr = 0;
+                               descr->hw_regs.payload.size, DMA_FROM_DEVICE);
+
                        dev_kfree_skb_any(descr->skb);
                        descr->skb = NULL;
+
                        gelic_descr_set_status(descr,
                                GELIC_DESCR_DMA_NOT_IN_USE);
                }
+
+               descr->hw_regs.payload.dev_addr = 0;
+               descr->hw_regs.payload.size = 0;
+
                descr = descr->next;
        } while (descr != card->rx_chain.head);
  }
@@ -526,17 +549,19 @@ static void gelic_descr_release_tx(struct gelic_card 
*card,
                GELIC_DESCR_TX_TAIL));
dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
-               skb->len, DMA_TO_DEVICE);
-       dev_kfree_skb_any(skb);
+               descr->hw_regs.payload.size, DMA_TO_DEVICE);
descr->hw_regs.payload.dev_addr = 0;
        descr->hw_regs.payload.size = 0;
+
+       dev_kfree_skb_any(skb);
+       descr->skb = NULL;
+
        descr->hw_regs.next_descr_addr = 0;
        descr->hw_regs.result_size = 0;
        descr->hw_regs.valid_size = 0;
        descr->hw_regs.data_status = 0;
        descr->hw_regs.data_error = 0;
-       descr->skb = NULL;
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
  }
@@ -565,31 +590,34 @@ static void gelic_card_wake_queues(struct gelic_card 
*card)
  static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
  {
        struct gelic_descr_chain *tx_chain;
-       enum gelic_descr_dma_status status;
        struct device *dev = ctodev(card);
-       struct net_device *netdev;
-       int release = 0;
+       int release;
+
+       for (release = 0, tx_chain = &card->tx_chain;
+               tx_chain->head != tx_chain->tail && tx_chain->tail;
+               tx_chain->tail = tx_chain->tail->next) {
+               enum gelic_descr_dma_status status;
+               struct gelic_descr *descr;
+               struct net_device *netdev;
+
+               descr = tx_chain->tail;
+               status = gelic_descr_get_status(descr);
+               netdev = descr->skb->dev;
- for (tx_chain = &card->tx_chain;
-            tx_chain->head != tx_chain->tail && tx_chain->tail;
-            tx_chain->tail = tx_chain->tail->next) {
-               status = gelic_descr_get_status(tx_chain->tail);
-               netdev = tx_chain->tail->skb->dev;
                switch (status) {
                case GELIC_DESCR_DMA_RESPONSE_ERROR:
                case GELIC_DESCR_DMA_PROTECTION_ERROR:
                case GELIC_DESCR_DMA_FORCE_END:
-                        dev_info_ratelimited(dev,
-                                        "%s:%d: forcing end of tx descriptor with 
status %x\n",
-                                        __func__, __LINE__, status);
+                       dev_info_ratelimited(dev,
+                               "%s:%d: forcing end of tx descriptor with status 
%x\n",
+                               __func__, __LINE__, status);
                        netdev->stats.tx_dropped++;
                        break;
case GELIC_DESCR_DMA_COMPLETE:
-                       if (tx_chain->tail->skb) {
+                       if (descr->skb) {
                                netdev->stats.tx_packets++;
-                               netdev->stats.tx_bytes +=
-                                       tx_chain->tail->skb->len;
+                               netdev->stats.tx_bytes += descr->skb->len;
                        }
                        break;
@@ -599,7 +627,7 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
                        }
                }
- gelic_descr_release_tx(card, tx_chain->tail);
+               gelic_descr_release_tx(card, descr);
                release++;
        }
  out:
@@ -703,19 +731,19 @@ int gelic_net_stop(struct net_device *netdev)
   *
   * returns the address of the next descriptor, or NULL if not available.
   */
-static struct gelic_descr *
-gelic_card_get_next_tx_descr(struct gelic_card *card)
+static struct gelic_descr *gelic_card_get_next_tx_descr(struct gelic_card 
*card)
  {
        if (!card->tx_chain.head)
                return NULL;
+
        /*  see if the next descriptor is free */
        if (card->tx_chain.tail != card->tx_chain.head->next &&
-               gelic_descr_get_status(card->tx_chain.head) ==
-                       GELIC_DESCR_DMA_NOT_IN_USE)
+               (gelic_descr_get_status(card->tx_chain.head) ==
+                       GELIC_DESCR_DMA_NOT_IN_USE)) {
                return card->tx_chain.head;
-       else
-               return NULL;
+       }
+ return NULL;
  }
/**
@@ -809,18 +837,23 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
                if (!skb_tmp) {
                        return -ENOMEM;
                }
+
                skb = skb_tmp;
        }
- cpu_addr = dma_map_single(dev, skb->data, skb->len, DMA_TO_DEVICE);
+       descr->hw_regs.payload.size = skb->len;
+       cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
+               DMA_TO_DEVICE);
+       descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
- if (!cpu_addr) {
+       if (unlikely(dma_mapping_error(dev, cpu_addr))) {
                dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
+
+               descr->hw_regs.payload.dev_addr = 0;
+               descr->hw_regs.payload.size = 0;
                return -ENOMEM;
        }
- descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
-       descr->hw_regs.payload.size = cpu_to_be32(skb->len);
        descr->skb = skb;
        descr->hw_regs.data_status = 0;
        descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
@@ -948,9 +981,9 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
data_status = be32_to_cpu(descr->hw_regs.data_status);
        data_error = be32_to_cpu(descr->hw_regs.data_error);
-       /* unmap skb buffer */
+
        dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
-                        GELIC_NET_MAX_MTU, DMA_FROM_DEVICE);
+                        descr->hw_regs.payload.size, DMA_FROM_DEVICE);
skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size) ?
                be32_to_cpu(descr->hw_regs.valid_size) :

Reply via email to