From: Ferruh Yigit <ferruh.yigit@intel.com>
To: ssardar@amd.com, dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe
Date: Fri, 1 May 2020 17:15:29 +0100 [thread overview]
Message-ID: <59a8b722-a638-d58e-1aad-dce535341c4b@intel.com> (raw)
In-Reply-To: <5528f7ad-2731-6a23-20ee-78bcbf3328b6@intel.com>
On 5/1/2020 12:33 PM, Ferruh Yigit wrote:
> On 4/30/2020 7:59 AM, ssardar@amd.com wrote:
>> From: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>
>>
>> adding below APIs for axgbe
>> - axgbe_enable_rx_vlan_stripping: to enable vlan header stipping
>> - axgbe_disable_rx_vlan_stripping: to disable vlan header stipping
>> - axgbe_enable_rx_vlan_filtering: to enable vlan filter mode
>> - axgbe_disable_rx_vlan_filtering: to disable vlan filter mode
>> - axgbe_update_vlan_hash_table: crc calculation and hash table update
>> based on vlan values post filter enable
>> - axgbe_vlan_filter_set: setting of active vlan out of max 4K values before
>> doing hash update of same
>> - axgbe_vlan_tpid_set: setting of default tpid values
>> - axgbe_vlan_offload_set: a top layer function to call stip/filter etc
>> based on mask values
>>
>> Signed-off-by: Sardar Shamsher Singh <Shamshersingh.Sardar@amd.com>
>
> <...>
>
>> +static int
>> +axgbe_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>> +{
>> + struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>> + struct axgbe_port *pdata = dev->data->dev_private;
>> +
>> + /* Indicate that VLAN Tx CTAGs come from context descriptors */
>> + AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, CSVL, 0);
>> + AXGMAC_IOWRITE_BITS(pdata, MAC_VLANIR, VLTI, 1);
>> +
>> + if (mask & ETH_VLAN_STRIP_MASK) {
>> + if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
>> + PMD_DRV_LOG(DEBUG, "Strip ON for device = %s\n",
>> + pdata->eth_dev->device->name);
>> + pdata->hw_if.enable_rx_vlan_stripping(pdata);
>> + } else {
>> + PMD_DRV_LOG(DEBUG, "Strip OFF for device = %s\n",
>> + pdata->eth_dev->device->name);
>> + pdata->hw_if.disable_rx_vlan_stripping(pdata);
>> + }
>> + }
>> + if (mask & ETH_VLAN_FILTER_MASK) {
>> + if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_FILTER) {
>> + PMD_DRV_LOG(DEBUG, "Filter ON for device = %s\n",
>> + pdata->eth_dev->device->name);
>> + pdata->hw_if.enable_rx_vlan_filtering(pdata);
>> + } else {
>> + PMD_DRV_LOG(DEBUG, "Filter OFF for device = %s\n",
>> + pdata->eth_dev->device->name);
>> + pdata->hw_if.disable_rx_vlan_filtering(pdata);
>> + }
>> + }
>> + if (mask & ETH_VLAN_EXTEND_MASK) {
>> + if (rxmode->offloads & DEV_RX_OFFLOAD_VLAN_EXTEND) {
>> + PMD_DRV_LOG(DEBUG, "enabling vlan extended mode\n");
>> + axgbe_vlan_extend_enable(pdata);
>> + /* Set global registers with default ethertype*/
>> + axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_OUTER,
>> + RTE_ETHER_TYPE_VLAN);
>> + axgbe_vlan_tpid_set(dev, ETH_VLAN_TYPE_INNER,
>> + RTE_ETHER_TYPE_VLAN);
>> + } else {
>> + PMD_DRV_LOG(DEBUG, "disabling vlan extended mode\n");
>> + axgbe_vlan_extend_disable(pdata);
>> + }
>> + }
>
>
> Is the intention here to enable disable QinQ stip, because enable/disable
> fucntions talks about qinq, if so 'ETH_QINQ_STRIP_MASK' should be used.
>
> <...>
>> @@ -275,6 +275,23 @@ axgbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>> /* Get the RSS hash */
>> if (AXGMAC_GET_BITS_LE(desc->write.desc3, RX_NORMAL_DESC3, RSV))
>> mbuf->hash.rss = rte_le_to_cpu_32(desc->write.desc1);
>> + etlt = AXGMAC_GET_BITS_LE(desc->write.desc3,
>> + RX_NORMAL_DESC3, ETLT);
>> + if (!err || !etlt) {
>> + if (etlt == 0x09 &&
>> + (rxq->pdata->eth_dev->data->dev_conf.rxmode.offloads &
>> + DEV_RX_OFFLOAD_VLAN_STRIP)) {
Also checking these offload flags only in the '.vlan_offload_set' dev_ops is not
enough, user may be set these flags but not called the '.vlan_offload_set' at
all, that is why configure() fucntion should do similar checks and configuration.
>> + mbuf->ol_flags |=
>> + PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED;
>> + mbuf->vlan_tci =
>> + AXGMAC_GET_BITS_LE(desc->write.desc0,
>> + RX_NORMAL_DESC0, OVT);
>
> I don't know HW capabilities, but if 'etlt == 0x09' means packet has VLAN tag,
> you can use it independent from strip, like below, up to you.
>
> if (vlan) {
> mbuf->ol_flags |= PKT_RX_VLAN;
> mbuf->vlan_tci = xxx
> if (vlan_stripped) {
> mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
> }
> }
>
> <...>
>
>> @@ -487,6 +520,7 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>> struct axgbe_tx_queue *txq;
>> unsigned int tsize;
>> const struct rte_memzone *tz;
>> + uint64_t offloads;
>>
>> tx_desc = nb_desc;
>> pdata = dev->data->dev_private;
>> @@ -506,7 +540,8 @@ int axgbe_dev_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
>> if (!txq)
>> return -ENOMEM;
>> txq->pdata = pdata;
>> -
>> + offloads = tx_conf->offloads |
>> + txq->pdata->eth_dev->data->dev_conf.txmode.offloads;
>
> As far as I can see PMD doesn't support queue specific offloads, so
> 'tx_conf->offloads' should be always 0.
>
> And since there is no queue specific offload and this 'offloads' information is
> only used to set burst function, which is again only port bases (this will keep
> overwrite same info per each queue), why not do this check in the
> 'axgbe_dev_configure()' instead of per queue?
>
> And I can see you may hit the problem becuase of VLAN offload but the issue
> seems generic, not directly related to VLAN support, and this can be separate
> fix I think.
>
next prev parent reply other threads:[~2020-05-01 16:15 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 6:59 ssardar
2020-05-01 11:33 ` Ferruh Yigit
2020-05-01 16:15 ` Ferruh Yigit [this message]
2020-05-06 5:17 ` Sardar, Shamsher singh
2020-05-06 9:48 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59a8b722-a638-d58e-1aad-dce535341c4b@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=ssardar@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).