DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Sardar, Shamsher singh" <Shamshersingh.Sardar@amd.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe
Date: Wed, 6 May 2020 10:48:35 +0100	[thread overview]
Message-ID: <6a9867fc-ec8c-6339-99dd-4fa59a03faaa@intel.com> (raw)
In-Reply-To: <DM6PR12MB2890C35242ADA8EDBFB0D8B284A40@DM6PR12MB2890.namprd12.prod.outlook.com>

On 5/6/2020 6:17 AM, Sardar, Shamsher singh wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Ferruh,
> Thanks for knowledge sharing.
> Yes etlt - 0x09 is nothing but indicate " ■ 4’b1001: The packet is type packet with Single CVLAN tag."
> And you are right it should be as below and will do changes on same:
> 
> if (vlan) {
>         mbuf->ol_flags |= PKT_RX_VLAN;
>         mbuf->vlan_tci = xxx
>         if (vlan_stripped) {
>                 mbuf->ol_flags |= PKT_RX_VLAN_STRIPPED;
>         }
> }
> 
> But rest of the items will do as incremental development.
> 1. Like in axgbe_dev_configure() need to check for default setting when system comes up.

OK

> 2. There are lot of changes need to be add for QinQ support.

I just would like to be sure you are not confusing QinQ support with
'DEV_RX_OFFLOAD_VLAN_EXTEND', because 'DEV_RX_OFFLOAD_VLAN_EXTEND' is not very
clearly defined.
In the 'axgbe_vlan_extend_enable()', it mentions from 'qinq', so it is confusing.
If you are clear on what 'DEV_RX_OFFLOAD_VLAN_EXTEND' is, that is OK. If not I
sugggest dropping the 'ETH_VLAN_EXTEND_MASK' part.

> Will add all as incremental development work.
> Currently working to make other vendor's SFP to work and in parallel above changes will add for upstream.
> Hope this should be fine.

OK

> 
> Can you please put some more light on below, what type issues may occur?
> " 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."

I was refering to the changes in 'axgbe_dev_tx_queue_setup()',
a) Why those changes are done at all?
b) Why they are done in this patch? Should it be a seperate fix?

> 
> Thanks & regards
> S Shamsher Singh
> (+91)7259016141
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, May 1, 2020 5:04 PM
> To: Sardar, Shamsher singh <Shamshersingh.Sardar@amd.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3] net/axgbe: enabling VLAN support in axgbe
> 
> [CAUTION: External Email]
> 
> 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)) {
>> +                             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.
> 


      reply	other threads:[~2020-05-06  9:48 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
2020-05-06  5:17   ` Sardar, Shamsher singh
2020-05-06  9:48     ` Ferruh Yigit [this message]

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=6a9867fc-ec8c-6339-99dd-4fa59a03faaa@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=Shamshersingh.Sardar@amd.com \
    --cc=dev@dpdk.org \
    /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).