From: Bruce Richardson <bruce.richardson@intel.com>
To: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Cc: <dev@dpdk.org>, <beilei.xing@intel.com>, <stable@dpdk.org>
Subject: Re: [PATCH] net/i40e: fix read register return status check
Date: Tue, 19 Nov 2024 11:52:38 +0000 [thread overview]
Message-ID: <Zzx8BtjXw-ARYia5@bricha3-mobl1.ger.corp.intel.com> (raw)
In-Reply-To: <20241115191425.970929-1-vladimir.medvedkin@intel.com>
On Fri, Nov 15, 2024 at 07:14:25PM +0000, Vladimir Medvedkin wrote:
> 'i40e_get_outer_vlan()' does not check 'i40e_aq_debug_read_register()'
> return value. This patch fixes this issue.
I think a little more detail on the scope of the changes could be good
here. It fixes the issue by allowing the function to return error, or zero
on success rather than the value read. This change then causes some rework
in the calling code to handle the error code and to change the form of the
function call.
>
> Coverity issue: 445518
>
> Fixes: 86eb05d6350b ("net/i40e: add flow validate function")
> Cc: beilei.xing@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
Fix below looks good, some minor comments.
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
> drivers/net/i40e/i40e_flow.c | 78 ++++++++++++++++++++++++++++++------
> 1 file changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> index c6857727e8..5015eda461 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -1263,27 +1263,31 @@ i40e_flow_parse_attr(const struct rte_flow_attr *attr,
> return 0;
> }
>
> -static uint16_t
> -i40e_get_outer_vlan(struct rte_eth_dev *dev)
> +static int
> +i40e_get_outer_vlan(struct rte_eth_dev *dev, uint16_t *tpid)
> {
> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> int qinq = dev->data->dev_conf.rxmode.offloads &
> RTE_ETH_RX_OFFLOAD_VLAN_EXTEND;
> uint64_t reg_r = 0;
> uint16_t reg_id;
> - uint16_t tpid;
> + int ret;
>
> if (qinq)
> reg_id = 2;
> else
> reg_id = 3;
>
> - i40e_aq_debug_read_register(hw, I40E_GL_SWT_L2TAGCTRL(reg_id),
> + ret = i40e_aq_debug_read_register(hw, I40E_GL_SWT_L2TAGCTRL(reg_id),
> ®_r, NULL);
> + if (ret != I40E_SUCCESS) {
> + PMD_DRV_LOG(ERR, "Fail to debug read from c[%d]", reg_id);
This error message maybe could do with a little clarification. The "c"
prefix doesn't make sense to me.
> + return -EIO;
> + }
>
> - tpid = (reg_r >> I40E_GL_SWT_L2TAGCTRL_ETHERTYPE_SHIFT) & 0xFFFF;
> + *tpid = (reg_r >> I40E_GL_SWT_L2TAGCTRL_ETHERTYPE_SHIFT) & 0xFFFF;
>
> - return tpid;
> + return 0;
> }
>
> /* 1. Last in item should be NULL as range is not supported.
> @@ -1303,6 +1307,8 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev *dev,
> const struct rte_flow_item_eth *eth_spec;
> const struct rte_flow_item_eth *eth_mask;
> enum rte_flow_item_type item_type;
> + int ret;
> + uint16_t tpid;
>
> for (; item->type != RTE_FLOW_ITEM_TYPE_END; item++) {
> if (item->last) {
> @@ -1361,8 +1367,24 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev *dev,
>
> if (filter->ether_type == RTE_ETHER_TYPE_IPV4 ||
> filter->ether_type == RTE_ETHER_TYPE_IPV6 ||
> - filter->ether_type == RTE_ETHER_TYPE_LLDP ||
> - filter->ether_type == i40e_get_outer_vlan(dev)) {
> + filter->ether_type == RTE_ETHER_TYPE_LLDP) {
> + rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Unsupported ether_type in"
> + " control packet filter.");
Best not to split error messages across lines.
> + return -rte_errno;
> + }
> +
> + ret = i40e_get_outer_vlan(dev, &tpid);
> + if (ret != 0) {
> + rte_flow_error_set(error, EIO,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Can not get the Ethertype identifying the L2 tag");
> + return -rte_errno;
> + }
> + if (filter->ether_type == tpid) {
> rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> @@ -1370,6 +1392,7 @@ i40e_flow_parse_ethertype_pattern(struct rte_eth_dev *dev,
> " control packet filter.");
> return -rte_errno;
> }
> +
> break;
> default:
> break;
> @@ -1641,6 +1664,7 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
> bool outer_ip = true;
> uint8_t field_idx;
> int ret;
> + uint16_t tpid;
>
> memset(off_arr, 0, sizeof(off_arr));
> memset(len_arr, 0, sizeof(len_arr));
> @@ -1709,14 +1733,29 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
> ether_type = rte_be_to_cpu_16(eth_spec->hdr.ether_type);
>
> if (ether_type == RTE_ETHER_TYPE_IPV4 ||
> - ether_type == RTE_ETHER_TYPE_IPV6 ||
> - ether_type == i40e_get_outer_vlan(dev)) {
> + ether_type == RTE_ETHER_TYPE_IPV6) {
> rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> "Unsupported ether_type.");
> return -rte_errno;
> }
> + ret = i40e_get_outer_vlan(dev, &tpid);
> + if (ret != 0) {
> + rte_flow_error_set(error, EIO,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Can not get the Ethertype identifying the L2 tag");
> + return -rte_errno;
> + }
> + if (ether_type == tpid) {
> + rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Unsupported ether_type.");
> + return -rte_errno;
> + }
> +
> input_set |= I40E_INSET_LAST_ETHER_TYPE;
> filter->input.flow.l2_flow.ether_type =
> eth_spec->hdr.ether_type;
> @@ -1763,14 +1802,29 @@ i40e_flow_parse_fdir_pattern(struct rte_eth_dev *dev,
> rte_be_to_cpu_16(vlan_spec->hdr.eth_proto);
>
> if (ether_type == RTE_ETHER_TYPE_IPV4 ||
> - ether_type == RTE_ETHER_TYPE_IPV6 ||
> - ether_type == i40e_get_outer_vlan(dev)) {
> + ether_type == RTE_ETHER_TYPE_IPV6) {
> rte_flow_error_set(error, EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> item,
> "Unsupported inner_type.");
> return -rte_errno;
> }
> + ret = i40e_get_outer_vlan(dev, &tpid);
> + if (ret != 0) {
> + rte_flow_error_set(error, EIO,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Can not get the Ethertype identifying the L2 tag");
> + return -rte_errno;
> + }
> + if (ether_type == tpid) {
> + rte_flow_error_set(error, EINVAL,
> + RTE_FLOW_ERROR_TYPE_ITEM,
> + item,
> + "Unsupported ether_type.");
> + return -rte_errno;
> + }
> +
> input_set |= I40E_INSET_LAST_ETHER_TYPE;
> filter->input.flow.l2_flow.ether_type =
> vlan_spec->hdr.eth_proto;
> --
> 2.43.0
>
next prev parent reply other threads:[~2024-11-19 11:52 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-15 19:14 Vladimir Medvedkin
2024-11-19 11:52 ` Bruce Richardson [this message]
2024-11-19 12:02 ` Bruce Richardson
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=Zzx8BtjXw-ARYia5@bricha3-mobl1.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=stable@dpdk.org \
--cc=vladimir.medvedkin@intel.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).