On 6/19/20 6:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengch...@huawei.com>
>
> Currently, there is a potential problem that calling the API function
> rte_eth_dev_set_vlan_offload to start a vlan hardware offloads which the
> driver does not support. if the PMD driver does not support the relative
> hardware offloads and does not check for it, the hardware setting will
> not change, but the relative offload in dev->data->dev_conf.rxmode.offloads
> will be turned on.
I'm sorry, but I don't understand what 'relative' means in the
context.
Also, please, use 'VLAN' instead of 'vlan' in the description
and log message below.
> It is supposed to check the hardware capabilities to decide whether the
> relative callback needs to be called just like the behavior in the API
> function named rte_eth_dev_configure.
I think the direction of the fix is right, but it definitely
duplicates checks which are done in some PMDs. I think that it
would be good to do the cleanup in follow up patches.
> Fixes: 81f9db8ecc2c ("ethdev: add vlan offload support")
Most likely it is incorrect, since generic Rx offloads were
introduced later.
> Cc: sta...@dpdk.org
>
> Signed-off-by: Chengchang Tang <tangchengch...@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.hu...@huawei.com>
> ---
> lib/librte_ethdev/rte_ethdev.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 122fd2a..5176a0e 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -3261,12 +3261,14 @@ rte_eth_dev_set_vlan_ether_type(uint16_t port_id,
> int
> rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask)
> {
> + struct rte_eth_dev_info dev_info;
> struct rte_eth_dev *dev;
> int ret = 0;
> int mask = 0;
> int cur, org = 0;
> uint64_t orig_offloads;
> uint64_t dev_offloads;
> + uint64_t new_offloads;
>
> RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> dev = &rte_eth_devices[port_id];
> @@ -3320,6 +3322,25 @@ rte_eth_dev_set_vlan_offload(uint16_t port_id, int
> offload_mask)
> if (mask == 0)
> return ret;
>
> + ret = rte_eth_dev_info_get(port_id, &dev_info);
> + if (ret != 0)
> + return ret;
> +
> + /*
> + * New added Rx vlan offloading which are not enabled in
> + * rte_eth_dev_configure() must be within its device capabilities
> + */
> + if ((dev_offloads & dev_info.rx_offload_capa) != dev_offloads) {
> + new_offloads = dev_offloads & ~orig_offloads;
> + RTE_ETHDEV_LOG(ERR,
> + "Ethdev port_id=%u requested new added vlan offloads "
> + "0x%" PRIx64 " must be within Rx offloads capabilities "
> + "0x%"PRIx64 " in %s()\n",
> + port_id, new_offloads, dev_info.rx_offload_capa,
> + __func__);
> + return -EINVAL;
> + }
> +
> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vlan_offload_set, -ENOTSUP);
> dev->data->dev_conf.rxmode.offloads = dev_offloads;
> ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
>
