Skip to contentSkip to conversation

IB/hfi1: Eliminate allocation while atomic

bug report Dan Carpenter 3 participants 4 messages Oct 2, 2019 – Oct 5, 2019 InfiniBand

lore
Share & tools
lore
#0 lore raw

Hello Don Hiatt,

The patch f8195f3b14a0: "IB/hfi1: Eliminate allocation while atomic" from Oct 9, 2017, leads to the following static checker warning:

drivers/infiniband/hw/hfi1/verbs.c:824 build_verbs_tx_desc()
error: doing dma on the stack (trail_buf)

drivers/infiniband/hw/hfi1/verbs.c
147 /* Length of buffer to create verbs txreq cache name */ 148 #define TXREQ_NAME_LEN 24
149
150 /* 16B trailing buffer */

   151  static const u8 trail_buf[MAX_16B_PADDING];
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^

This used to be actually allocated on the stack but now it's here. I believe this is still a problem. It's not a problem for most people at runtime, but it's technically a bug. And I believe that soon we will add a check in dma_map_single() which will generate a warning.

   152  
   153  static uint wss_threshold = 80;

[ snip ]

801 } else {
802 ret = sdma_txinit_ahg(

   803                          &tx->txreq,
   804                          ahg_info->tx_flags,
   805                          length,
   806                          ahg_info->ahgidx,
   807                          ahg_info->ahgcount,
   808                          ahg_info->ahgdesc,
   809                          hdrbytes,
   810                          verbs_sdma_complete);
   811                  if (ret)
   812                          goto bail_txadd;
   813          }
   814          /* add the ulp payload - if any. tx->ss can be NULL for acks */
   815          if (tx->ss) {
   816                  ret = build_verbs_ulp_payload(sde, length, tx);
   817                  if (ret)
   818                          goto bail_txadd;
   819          }
   820  
   821          /* add icrc, lt byte, and padding to flit */
   822          if (extra_bytes)
   823                  ret = sdma_txadd_kvaddr(sde->dd, &tx->txreq,
   824                                          (void *)trail_buf, extra_bytes);
                                                        ^^^^^^^^^

This has to be DMAable memory.

825
826 bail_txadd:
827 return ret;
828 }

regards,
dan carpenter

#1 lore raw
Quoted 53 lines · Dan Carpenter

Hello Don Hiatt,

The patch f8195f3b14a0: "IB/hfi1: Eliminate allocation while atomic" from Oct 9, 2017, leads to the following static checker warning:

drivers/infiniband/hw/hfi1/verbs.c:824 build_verbs_tx_desc()
error: doing dma on the stack (trail_buf)

drivers/infiniband/hw/hfi1/verbs.c

    147  /* Length of buffer to create verbs txreq cache name */
    148  #define TXREQ_NAME_LEN 24
    149
    150  /* 16B trailing buffer */
    151  static const u8 trail_buf[MAX_16B_PADDING];
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^

This used to be actually allocated on the stack but now it's here. I believe this is still a problem. It's not a problem for most people at runtime, but it's technically a bug. And I believe that soon we will add a check in dma_map_single() which will generate a warning.

    152
    153  static uint wss_threshold = 80;

[ snip ]

    801          } else {
    802                  ret = sdma_txinit_ahg(
    803                          &tx->txreq,
    804                          ahg_info->tx_flags,
    805                          length,
    806                          ahg_info->ahgidx,
    807                          ahg_info->ahgcount,
    808                          ahg_info->ahgdesc,
    809                          hdrbytes,
    810                          verbs_sdma_complete);
    811                  if (ret)
    812                          goto bail_txadd;
    813          }
    814          /* add the ulp payload - if any. tx->ss can be NULL for acks */
    815          if (tx->ss) {
    816                  ret = build_verbs_ulp_payload(sde, length, tx);
    817                  if (ret)
    818                          goto bail_txadd;
    819          }
    820
    821          /* add icrc, lt byte, and padding to flit */
    822          if (extra_bytes)
    823                  ret = sdma_txadd_kvaddr(sde->dd, &tx->txreq,
    824                                          (void *)trail_buf, extra_bytes);
                                                         ^^^^^^^^^

This has to be DMAable memory.

Quoted 9 lines
    825
    826  bail_txadd:
    827          return ret;
    828  }

regards,
dan carpenter

Thanks Dan, we actually got a separate out-of-band email about this. We are working up a fix right now.

-Denny