On 6/9/2018 9:04 AM, Andrew Rybchenko wrote:

On 06/09/2018 01:41 AM, Ferruh Yigit wrote:

quoted

Cc: Shahaf Shuler <redacted>

Signed-off-by: Ferruh Yigit <redacted>
---

<...>

quoted

diff --git a/app/test-eventdev/test_perf_common.c b/app/test-eventdev/test_perf_common.c
index d00f91802..9fe042ffe 100644
--- a/app/test-eventdev/test_perf_common.c
+++ b/app/test-eventdev/test_perf_common.c

@@ -680,13 +680,7 @@ perf_ethdev_setup(struct evt_test *test, struct evt_options *opt)
 			.mq_mode = ETH_MQ_RX_RSS,
 			.max_rx_pkt_len = ETHER_MAX_LEN,
 			.split_hdr_size = 0,
-			.header_split   = 0,
-			.hw_ip_checksum = 0,
-			.hw_vlan_filter = 0,
-			.hw_vlan_strip  = 0,
-			.hw_vlan_extend = 0,
 			.jumbo_frame    = 0,
-			.hw_strip_crc   = 1,

Hi Andrew,

I have 2 questions here:
 1. Why is jumbo_frame kept? There is DEV_RX_OFFLOAD_JUMBO_FRAME.

Because there are still some usage of this flag in PMDs, they need to be
clarified before removing the flag. I am for removing the flag in final version,
but for this RFC I am not able to find enough time to work on PMDs for it.

 2. Why is hw_strip_crc=1 discarded? Yes, I remember plans to make it
     default behaviour and introduce flag to keep CRC, but right now the
     patch looks like mixture of two changes which is not good.

Yes it is wrong, app should replace "".hw_strip_crc=1 with KEEP_CRC offload.
Since both are RFC, kind of hard to maintain, but I think good to create a
dependency from this patch to KEEP_CRC one.

There are more cases in the patch where hw_strip_crc=1 is simply discarded.

<...>

quoted

diff --git a/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
index 1b6499f85..ee8ae5b9f 100644
--- a/drivers/net/sfc/sfc_ethdev.c
+++ b/drivers/net/sfc/sfc_ethdev.c

@@ -1089,7 +1089,6 @@ sfc_tx_queue_info_get(struct rte_eth_dev *dev, uint16_t tx_queue_id,
 
 	memset(qinfo, 0, sizeof(*qinfo));
 
-	qinfo->conf.txq_flags = txq_info->txq->flags;
 	qinfo->conf.offloads = txq_info->txq->offloads;
 	qinfo->conf.tx_free_thresh = txq_info->txq->free_thresh;
 	qinfo->conf.tx_deferred_start = txq_info->deferred_start;

diff --git a/drivers/net/sfc/sfc_rx.c b/drivers/net/sfc/sfc_rx.c
index cc76a5b15..58a0df583 100644
--- a/drivers/net/sfc/sfc_rx.c
+++ b/drivers/net/sfc/sfc_rx.c

@@ -1446,7 +1446,6 @@ sfc_rx_check_mode(struct sfc_adapter *sa, struct rte_eth_rxmode *rxmode)
 	if (~rxmode->offloads & DEV_RX_OFFLOAD_CRC_STRIP) {
 		sfc_warn(sa, "FCS stripping cannot be disabled - always on");
 		rxmode->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
-		rxmode->hw_strip_crc = 1;
 	}
 
 	return rc;

diff --git a/drivers/net/sfc/sfc_tx.c b/drivers/net/sfc/sfc_tx.c
index 1bcc2c697..6d42a1a65 100644
--- a/drivers/net/sfc/sfc_tx.c
+++ b/drivers/net/sfc/sfc_tx.c

@@ -171,7 +171,6 @@ sfc_tx_qinit(struct sfc_adapter *sa, unsigned int sw_index,
 	txq->free_thresh =
 		(tx_conf->tx_free_thresh) ? tx_conf->tx_free_thresh :
 		SFC_TX_DEFAULT_FREE_THRESH;
-	txq->flags = tx_conf->txq_flags;
 	txq->offloads = offloads;
 
 	rc = sfc_dma_alloc(sa, "txq", sw_index, EFX_TXQ_SIZE(txq_info->entries),

@@ -182,7 +181,6 @@ sfc_tx_qinit(struct sfc_adapter *sa, unsigned int sw_index,
 	memset(&info, 0, sizeof(info));
 	info.max_fill_level = txq_max_fill_level;
 	info.free_thresh = txq->free_thresh;
-	info.flags = tx_conf->txq_flags;
 	info.offloads = offloads;
 	info.txq_entries = txq_info->entries;
 	info.dma_desc_size_max = encp->enc_tx_dma_desc_size_max;

@@ -431,18 +429,10 @@ sfc_tx_qstart(struct sfc_adapter *sa, unsigned int sw_index)
 	if (rc != 0)
 		goto fail_ev_qstart;
 
-	/*
-	 * The absence of ETH_TXQ_FLAGS_IGNORE is associated with a legacy
-	 * application which expects that IPv4 checksum offload is enabled
-	 * all the time as there is no legacy flag to turn off the offload.
-	 */
-	if ((txq->offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) ||
-	    (~txq->flags & ETH_TXQ_FLAGS_IGNORE))
+	if (txq->offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
 		flags |= EFX_TXQ_CKSUM_IPV4;
 
-	if ((txq->offloads & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM) ||
-	    ((~txq->flags & ETH_TXQ_FLAGS_IGNORE) &&
-	     (offloads_supported & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)))
+	if (txq->offloads & DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM)
 		flags |= EFX_TXQ_CKSUM_INNER_IPV4;
 
 	if ((txq->offloads & DEV_TX_OFFLOAD_TCP_CKSUM) ||

@@ -453,16 +443,7 @@ sfc_tx_qstart(struct sfc_adapter *sa, unsigned int sw_index)
 			flags |= EFX_TXQ_CKSUM_INNER_TCPUDP;
 	}
 
-	/*
-	 * The absence of ETH_TXQ_FLAGS_IGNORE is associated with a legacy
-	 * application. In turn, the absence of ETH_TXQ_FLAGS_NOXSUMTCP is
-	 * associated specifically with a legacy application which expects
-	 * both TCP checksum offload and TSO to be enabled because the legacy
-	 * API does not provide a dedicated mechanism to control TSO.
-	 */
-	if ((txq->offloads & DEV_TX_OFFLOAD_TCP_TSO) ||
-	    ((~txq->flags & ETH_TXQ_FLAGS_IGNORE) &&
-	     (~txq->flags & ETH_TXQ_FLAGS_NOXSUMTCP)))
+	if (txq->offloads & DEV_TX_OFFLOAD_TCP_TSO)
 		flags |= EFX_TXQ_FATSOV2;
 
 	rc = efx_tx_qcreate(sa->nic, sw_index, 0, &txq->mem,

net/sfc changes looks good.
Plus 'struct sfc_txq -> flags' (drivers/net/sfc/sfc_tx.h) and
'struct sfc_dp_tx_qcreate_info -> flags' (drivers/net/sfc/sfc_dp_tx.h) should be
removed since there are not used now.

If finally rxmode.jumbo_frame is removed, it should removed from
net/sfc as well (but compiler will help to find it in any case).

After applying the patch:
$ git grep ETH_TXQ_FLAGS
drivers/net/fm10k/fm10k.h:#define FM10K_SIMPLE_TX_FLAG
((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
drivers/net/fm10k/fm10k.h:                              ETH_TXQ_FLAGS_NOOFFLOADS)

Thanks, will remove this too.

In general I think that we should do it ASAP. Also it will guarantee
that new PMDs do not use corresponding structure members etc.

+1, +1