| Message ID | 20170516005523.26124-9-jakub.kicinski@netronome.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, May 15, 2017 at 05:55:22PM -0700, Jakub Kicinski wrote: > We have a number of places where we calculate the descriptor > index based on a value which may have overflown. Create a > macro for masking with the ring size. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > --- > drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 +++ > drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 21 ++++++++++----------- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h > index 66319a1026bb..7b9518cbe965 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h > @@ -117,6 +117,9 @@ struct nfp_eth_table_port; > struct nfp_net; > struct nfp_net_r_vector; > > +/* Convenience macro for wrapping descriptor index on ring size */ > +#define D_IDX(ring, idx) ((idx) & ((ring)->cnt - 1)) Any reason not to make this a function? That notwithstanding: Reviewed-by: Simon Horman <simon.horman@netronome.com>
On Tue, 16 May 2017 09:08:08 +0200, Simon Horman wrote: > On Mon, May 15, 2017 at 05:55:22PM -0700, Jakub Kicinski wrote: > > We have a number of places where we calculate the descriptor > > index based on a value which may have overflown. Create a > > macro for masking with the ring size. > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > --- > > drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 +++ > > drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 21 ++++++++++----------- > > 2 files changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h > > index 66319a1026bb..7b9518cbe965 100644 > > --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h > > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h > > @@ -117,6 +117,9 @@ struct nfp_eth_table_port; > > struct nfp_net; > > struct nfp_net_r_vector; > > > > +/* Convenience macro for wrapping descriptor index on ring size */ > > +#define D_IDX(ring, idx) ((idx) & ((ring)->cnt - 1)) > > Any reason not to make this a function? This is to be able to use the same macro for both RX and TX rings. If you look below in the code I have a macro for setting dma addresses on descriptors also regardless of the type. It makes the code a tiny bit cleaner IMHO and should be acceptable for trivial macros. > That notwithstanding: > > Reviewed-by: Simon Horman <simon.horman@netronome.com> Thanks for the reviews!
On Tue, May 16, 2017 at 12:38:57AM -0700, Jakub Kicinski wrote: > On Tue, 16 May 2017 09:08:08 +0200, Simon Horman wrote: > > On Mon, May 15, 2017 at 05:55:22PM -0700, Jakub Kicinski wrote: > > > We have a number of places where we calculate the descriptor > > > index based on a value which may have overflown. Create a > > > macro for masking with the ring size. > > > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > > > --- > > > drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 +++ > > > drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 21 ++++++++++----------- > > > 2 files changed, 13 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h > > > index 66319a1026bb..7b9518cbe965 100644 > > > --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h > > > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h > > > @@ -117,6 +117,9 @@ struct nfp_eth_table_port; > > > struct nfp_net; > > > struct nfp_net_r_vector; > > > > > > +/* Convenience macro for wrapping descriptor index on ring size */ > > > +#define D_IDX(ring, idx) ((idx) & ((ring)->cnt - 1)) > > > > Any reason not to make this a function? > > This is to be able to use the same macro for both RX and TX rings. If > you look below in the code I have a macro for setting dma addresses on > descriptors also regardless of the type. > > It makes the code a tiny bit cleaner IMHO and should be acceptable for > trivial macros. Thanks for the clarification, that sounds reasonable to me. > > That notwithstanding: > > > > Reviewed-by: Simon Horman <simon.horman@netronome.com> > > Thanks for the reviews!
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h index 66319a1026bb..7b9518cbe965 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h @@ -117,6 +117,9 @@ struct nfp_eth_table_port; struct nfp_net; struct nfp_net_r_vector; +/* Convenience macro for wrapping descriptor index on ring size */ +#define D_IDX(ring, idx) ((idx) & ((ring)->cnt - 1)) + /* Convenience macro for writing dma address into RX/TX descriptors */ #define nfp_desc_set_dma_addr(desc, dma_addr) \ do { \ diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c index a4c6878f1df1..c64514f8ee65 100644 --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c @@ -809,7 +809,7 @@ static int nfp_net_tx(struct sk_buff *skb, struct net_device *netdev) if (dma_mapping_error(dp->dev, dma_addr)) goto err_free; - wr_idx = tx_ring->wr_p & (tx_ring->cnt - 1); + wr_idx = D_IDX(tx_ring, tx_ring->wr_p); /* Stash the soft descriptor of the head then initialize it */ txbuf = &tx_ring->txbufs[wr_idx]; @@ -852,7 +852,7 @@ static int nfp_net_tx(struct sk_buff *skb, struct net_device *netdev) if (dma_mapping_error(dp->dev, dma_addr)) goto err_unmap; - wr_idx = (wr_idx + 1) & (tx_ring->cnt - 1); + wr_idx = D_IDX(tx_ring, wr_idx + 1); tx_ring->txbufs[wr_idx].skb = skb; tx_ring->txbufs[wr_idx].dma_addr = dma_addr; tx_ring->txbufs[wr_idx].fidx = f; @@ -946,8 +946,7 @@ static void nfp_net_tx_complete(struct nfp_net_tx_ring *tx_ring) todo = qcp_rd_p + tx_ring->cnt - tx_ring->qcp_rd_p; while (todo--) { - idx = tx_ring->rd_p & (tx_ring->cnt - 1); - tx_ring->rd_p++; + idx = D_IDX(tx_ring, tx_ring->rd_p++); skb = tx_ring->txbufs[idx].skb; if (!skb) @@ -1023,11 +1022,11 @@ static bool nfp_net_xdp_complete(struct nfp_net_tx_ring *tx_ring) done_all = todo <= NFP_NET_XDP_MAX_COMPLETE; todo = min(todo, NFP_NET_XDP_MAX_COMPLETE); - tx_ring->qcp_rd_p = (tx_ring->qcp_rd_p + todo) & (tx_ring->cnt - 1); + tx_ring->qcp_rd_p = D_IDX(tx_ring, tx_ring->qcp_rd_p + todo); done_pkts = todo; while (todo--) { - idx = tx_ring->rd_p & (tx_ring->cnt - 1); + idx = D_IDX(tx_ring, tx_ring->rd_p); tx_ring->rd_p++; done_bytes += tx_ring->txbufs[idx].real_len; @@ -1063,7 +1062,7 @@ nfp_net_tx_ring_reset(struct nfp_net_dp *dp, struct nfp_net_tx_ring *tx_ring) struct sk_buff *skb; int idx, nr_frags; - idx = tx_ring->rd_p & (tx_ring->cnt - 1); + idx = D_IDX(tx_ring, tx_ring->rd_p); tx_buf = &tx_ring->txbufs[idx]; skb = tx_ring->txbufs[idx].skb; @@ -1216,7 +1215,7 @@ static void nfp_net_rx_give_one(const struct nfp_net_dp *dp, { unsigned int wr_idx; - wr_idx = rx_ring->wr_p & (rx_ring->cnt - 1); + wr_idx = D_IDX(rx_ring, rx_ring->wr_p); nfp_net_dma_sync_dev_rx(dp, dma_addr); @@ -1254,7 +1253,7 @@ static void nfp_net_rx_ring_reset(struct nfp_net_rx_ring *rx_ring) unsigned int wr_idx, last_idx; /* Move the empty entry to the end of the list */ - wr_idx = rx_ring->wr_p & (rx_ring->cnt - 1); + wr_idx = D_IDX(rx_ring, rx_ring->wr_p); last_idx = rx_ring->cnt - 1; rx_ring->rxbufs[wr_idx].dma_addr = rx_ring->rxbufs[last_idx].dma_addr; rx_ring->rxbufs[wr_idx].frag = rx_ring->rxbufs[last_idx].frag; @@ -1522,7 +1521,7 @@ nfp_net_tx_xdp_buf(struct nfp_net_dp *dp, struct nfp_net_rx_ring *rx_ring, } } - wr_idx = tx_ring->wr_p & (tx_ring->cnt - 1); + wr_idx = D_IDX(tx_ring, tx_ring->wr_p); /* Stash the soft descriptor of the head then initialize it */ txbuf = &tx_ring->txbufs[wr_idx]; @@ -1610,7 +1609,7 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget) dma_addr_t new_dma_addr; void *new_frag; - idx = rx_ring->rd_p & (rx_ring->cnt - 1); + idx = D_IDX(rx_ring, rx_ring->rd_p); rxd = &rx_ring->rxds[idx]; if (!(rxd->rxd.meta_len_dd & PCIE_DESC_RX_DD))
We have a number of places where we calculate the descriptor index based on a value which may have overflown. Create a macro for masking with the ring size. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- drivers/net/ethernet/netronome/nfp/nfp_net.h | 3 +++ drivers/net/ethernet/netronome/nfp/nfp_net_common.c | 21 ++++++++++----------- 2 files changed, 13 insertions(+), 11 deletions(-)