| Message ID | 1524582401-14696-1-git-send-email-hemant.agrawal@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] net/dpaa: fix the ethdev offload checks | expand |
On 4/24/2018 4:06 PM, Hemant Agrawal wrote: > From: Sunil Kumar Kori <sunil.kori@nxp.com> > > Fixes: 16e2c27f4fc7 ("net/dpaa: support new ethdev offload APIs") > > Signed-off-by: Sunil Kumar Kori <sunil.kori@nxp.com> > --- > drivers/net/dpaa/dpaa_ethdev.c | 89 +++++++++++++++++++++++++++--------------- > 1 file changed, 57 insertions(+), 32 deletions(-) > > diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c > index b2740b4..32d36f2 100644 > --- a/drivers/net/dpaa/dpaa_ethdev.c > +++ b/drivers/net/dpaa/dpaa_ethdev.c > @@ -45,6 +45,33 @@ > #include <fsl_bman.h> > #include <fsl_fman.h> > > +/* Supported Rx offloads */ > +static uint64_t dev_rx_offloads_sup = > + DEV_RX_OFFLOAD_JUMBO_FRAME; > + > +/* Rx offloads which cannot be disabled */ > +static uint64_t dev_rx_offloads_nodis = > + DEV_RX_OFFLOAD_IPV4_CKSUM | > + DEV_RX_OFFLOAD_UDP_CKSUM | > + DEV_RX_OFFLOAD_TCP_CKSUM | > + DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM | > + DEV_RX_OFFLOAD_CRC_STRIP | > + DEV_RX_OFFLOAD_SCATTER; > + > +/* Supported Tx offloads */ > +static uint64_t dev_tx_offloads_sup; > + > +/* Tx offloads which cannot be disabled */ > +static uint64_t dev_tx_offloads_nodis = > + DEV_TX_OFFLOAD_IPV4_CKSUM | > + DEV_TX_OFFLOAD_UDP_CKSUM | > + DEV_TX_OFFLOAD_TCP_CKSUM | > + DEV_TX_OFFLOAD_SCTP_CKSUM | > + DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM | > + DEV_TX_OFFLOAD_MULTI_SEGS | > + DEV_TX_OFFLOAD_MT_LOCKFREE | > + DEV_TX_OFFLOAD_MBUF_FAST_FREE; > + > /* Keep track of whether QMAN and BMAN have been globally initialized */ > static int is_global_init; > /* At present we only allow up to 4 push mode queues - as each of this queue > @@ -143,35 +170,41 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev) > { > struct dpaa_if *dpaa_intf = dev->data->dev_private; > struct rte_eth_conf *eth_conf = &dev->data->dev_conf; > - struct rte_eth_dev_info dev_info; > uint64_t rx_offloads = eth_conf->rxmode.offloads; > uint64_t tx_offloads = eth_conf->txmode.offloads; > > PMD_INIT_FUNC_TRACE(); > > - dpaa_eth_dev_info(dev, &dev_info); > - if (((~(dev_info.rx_offload_capa) & rx_offloads) != 0)) { > - DPAA_PMD_ERR("Some Rx offloads are not supported " > - "requested 0x%" PRIx64 " supported 0x%" PRIx64, > - rx_offloads, dev_info.rx_offload_capa); > - return -ENOTSUP; > + /* Rx offloads validation */ > + if (dev_rx_offloads_nodis & ~rx_offloads) { > + DPAA_PMD_WARN( > + "Rx offloads non configurable - requested 0x%" PRIx64 > + " ignored 0x%" PRIx64, > + rx_offloads, dev_rx_offloads_nodis); > } > - > - if (((~(dev_info.tx_offload_capa) & tx_offloads) != 0)) { > - DPAA_PMD_ERR("Some Tx offloads are not supported " > - "requested 0x%" PRIx64 " supported 0x%" PRIx64, > - tx_offloads, dev_info.tx_offload_capa); > + if (~(dev_rx_offloads_sup | dev_rx_offloads_nodis) & rx_offloads) { > + DPAA_PMD_ERR( > + "Rx offloads non supported - requested 0x%" PRIx64 > + " supported 0x%" PRIx64, > + rx_offloads, > + dev_rx_offloads_sup | dev_rx_offloads_nodis); > return -ENOTSUP; > } Hi Hemant, Overall this looks good to me, thanks. Only I would like to ask if you prefer to replace nodis and not_supported checks. Because with current order, if an offlaod requested that both has not supported offload and not enable all nodis offloads, this will print both logs and return error. Since it will return error, do you really need "non configurable" log? If you replace checks, if any not supported offload requested it will only print log for it and return error without checking/caring nodis offloads. It is up to you, please let me know if you want to go with existing set. Thanks, ferruh
HI Ferruh, > Hi Hemant, > > Overall this looks good to me, thanks. > > Only I would like to ask if you prefer to replace nodis and not_supported checks. > > Because with current order, if an offlaod requested that both has not supported > offload and not enable all nodis offloads, this will print both logs and return > error. Since it will return error, do you really need "non configurable" log? > > If you replace checks, if any not supported offload requested it will only print log > for it and return error without checking/caring nodis offloads. > > It is up to you, please let me know if you want to go with existing set. [Hemant] Thanks for the review. In v4, I have reversed the order of check Regards, Hemant > > Thanks, > ferruh
diff --git a/drivers/net/dpaa/dpaa_ethdev.c b/drivers/net/dpaa/dpaa_ethdev.c index b2740b4..32d36f2 100644 --- a/drivers/net/dpaa/dpaa_ethdev.c +++ b/drivers/net/dpaa/dpaa_ethdev.c @@ -45,6 +45,33 @@ #include <fsl_bman.h> #include <fsl_fman.h> +/* Supported Rx offloads */ +static uint64_t dev_rx_offloads_sup = + DEV_RX_OFFLOAD_JUMBO_FRAME; + +/* Rx offloads which cannot be disabled */ +static uint64_t dev_rx_offloads_nodis = + DEV_RX_OFFLOAD_IPV4_CKSUM | + DEV_RX_OFFLOAD_UDP_CKSUM | + DEV_RX_OFFLOAD_TCP_CKSUM | + DEV_RX_OFFLOAD_OUTER_IPV4_CKSUM | + DEV_RX_OFFLOAD_CRC_STRIP | + DEV_RX_OFFLOAD_SCATTER; + +/* Supported Tx offloads */ +static uint64_t dev_tx_offloads_sup; + +/* Tx offloads which cannot be disabled */ +static uint64_t dev_tx_offloads_nodis = + DEV_TX_OFFLOAD_IPV4_CKSUM | + DEV_TX_OFFLOAD_UDP_CKSUM | + DEV_TX_OFFLOAD_TCP_CKSUM | + DEV_TX_OFFLOAD_SCTP_CKSUM | + DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM | + DEV_TX_OFFLOAD_MULTI_SEGS | + DEV_TX_OFFLOAD_MT_LOCKFREE | + DEV_TX_OFFLOAD_MBUF_FAST_FREE; + /* Keep track of whether QMAN and BMAN have been globally initialized */ static int is_global_init; /* At present we only allow up to 4 push mode queues - as each of this queue @@ -143,35 +170,41 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev) { struct dpaa_if *dpaa_intf = dev->data->dev_private; struct rte_eth_conf *eth_conf = &dev->data->dev_conf; - struct rte_eth_dev_info dev_info; uint64_t rx_offloads = eth_conf->rxmode.offloads; uint64_t tx_offloads = eth_conf->txmode.offloads; PMD_INIT_FUNC_TRACE(); - dpaa_eth_dev_info(dev, &dev_info); - if (((~(dev_info.rx_offload_capa) & rx_offloads) != 0)) { - DPAA_PMD_ERR("Some Rx offloads are not supported " - "requested 0x%" PRIx64 " supported 0x%" PRIx64, - rx_offloads, dev_info.rx_offload_capa); - return -ENOTSUP; + /* Rx offloads validation */ + if (dev_rx_offloads_nodis & ~rx_offloads) { + DPAA_PMD_WARN( + "Rx offloads non configurable - requested 0x%" PRIx64 + " ignored 0x%" PRIx64, + rx_offloads, dev_rx_offloads_nodis); } - - if (((~(dev_info.tx_offload_capa) & tx_offloads) != 0)) { - DPAA_PMD_ERR("Some Tx offloads are not supported " - "requested 0x%" PRIx64 " supported 0x%" PRIx64, - tx_offloads, dev_info.tx_offload_capa); + if (~(dev_rx_offloads_sup | dev_rx_offloads_nodis) & rx_offloads) { + DPAA_PMD_ERR( + "Rx offloads non supported - requested 0x%" PRIx64 + " supported 0x%" PRIx64, + rx_offloads, + dev_rx_offloads_sup | dev_rx_offloads_nodis); return -ENOTSUP; } - if (((rx_offloads & DEV_RX_OFFLOAD_IPV4_CKSUM) == 0) || - ((rx_offloads & DEV_RX_OFFLOAD_UDP_CKSUM) == 0) || - ((rx_offloads & DEV_RX_OFFLOAD_TCP_CKSUM) == 0) || - ((tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) == 0) || - ((tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) == 0) || - ((tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) == 0)) { - DPAA_PMD_ERR(" Cksum offloading is enabled by default " - " Cannot be disabled. So ignoring this configuration "); + /* Tx offloads validation */ + if (dev_tx_offloads_nodis & ~tx_offloads) { + DPAA_PMD_WARN( + "Tx offloads non configurable - requested 0x%" PRIx64 + " ignored 0x%" PRIx64, + tx_offloads, dev_tx_offloads_nodis); + } + if (~(dev_tx_offloads_sup | dev_tx_offloads_nodis) & tx_offloads) { + DPAA_PMD_ERR( + "Tx offloads non supported - requested 0x%" PRIx64 + " supported 0x%" PRIx64, + tx_offloads, + dev_tx_offloads_sup | dev_tx_offloads_nodis); + return -ENOTSUP; } if (rx_offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) { @@ -290,18 +323,10 @@ static void dpaa_eth_dev_info(struct rte_eth_dev *dev, dev_info->flow_type_rss_offloads = DPAA_RSS_OFFLOAD_ALL; dev_info->speed_capa = (ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G); - dev_info->rx_offload_capa = - (DEV_RX_OFFLOAD_IPV4_CKSUM | - DEV_RX_OFFLOAD_UDP_CKSUM | - DEV_RX_OFFLOAD_TCP_CKSUM) | - DEV_RX_OFFLOAD_JUMBO_FRAME | - DEV_RX_OFFLOAD_SCATTER; - dev_info->tx_offload_capa = - (DEV_TX_OFFLOAD_IPV4_CKSUM | - DEV_TX_OFFLOAD_UDP_CKSUM | - DEV_TX_OFFLOAD_TCP_CKSUM) | - DEV_TX_OFFLOAD_MBUF_FAST_FREE | - DEV_TX_OFFLOAD_MULTI_SEGS; + dev_info->rx_offload_capa = dev_rx_offloads_sup | + dev_rx_offloads_nodis; + dev_info->tx_offload_capa = dev_tx_offloads_sup | + dev_tx_offloads_nodis; } static int dpaa_eth_link_update(struct rte_eth_dev *dev,