rte_bus_probe() does not exit on error

["McDaniel, Timothy" <timothy.mcdaniel@intel.com>]

Hello DPDK community,

I am following up on a question/comment that I submitted on April 18, for which
I have not received any responses. See the original comment below for context.

Are there objections to modifying the behavior of rte_bus_probe() so that it propagates
any errors detected while processing the command line arguments? It currently ignores
errors and continues on, always returning success instead of any error that was returned
by the probe function.

Thank You,
Tim McDaniel

> -----Original Message-----
> From: dev-request@dpdk.org <dev-request@dpdk.org>
> Sent: Monday, April 18, 2022 8:20 AM
> To: dev@dpdk.org
> Subject: dev Digest, Vol 400, Issue 6
> 
> Send dev mailing list submissions to
> 	dev@dpdk.org
> 
> To subscribe or unsubscribe via the World Wide Web, visit
> 	https://mails.dpdk.org/listinfo/dev
> or, via email, send a message with subject or body 'help' to
> 	dev-request@dpdk.org
> 
> You can reach the person managing the list at
> 	dev-owner@dpdk.org
> 
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of dev digest..."
> 
> 
> Today's Topics:
> 
>    1. [PATCH 1/2] common/mlx5: extend crypto capabilities (Raja Zidane)
>    2. RE: [PATCH] kni: update kernel API to receive packets
>       (Gagandeep Singh)
>    3. rte_bus_probe() does not exit on error (McDaniel, Timothy)
> 
> 
> ----------------------------------------------------------------------
> 
> Message: 1
> Date: Mon, 18 Apr 2022 14:10:04 +0300
> From: Raja Zidane <rzidane@nvidia.com>
> To: <dev@dpdk.org>
> Cc: <matan@nvidia.com>
> Subject: [PATCH 1/2] common/mlx5: extend crypto capabilities
> Message-ID: <20220418111005.2291-2-rzidane@nvidia.com>
> Content-Type: text/plain
> 
> Crypto capabilities struct contains info about crypto import method
> (wrapped/plaintext DEK) for each of the supported algorithms.
> 
> Query crypto capabilities struct and save import methods.
> 
> Signed-off-by: Raja Zidane <rzidane@nvidia.com>
> ---
>  drivers/common/mlx5/mlx5_devx_cmds.c | 13 +++++++++++--
>  drivers/common/mlx5/mlx5_devx_cmds.h |  1 +
>  drivers/common/mlx5/mlx5_prm.h       | 29 ++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.c
> b/drivers/common/mlx5/mlx5_devx_cmds.c
> index d02ac2a678..c6bdbc12bb 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.c
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.c
> @@ -964,12 +964,21 @@ mlx5_devx_cmd_query_hca_attr(void *ctx,
>  		MLX5_GET(cmd_hca_cap, hcattr,
> umr_modify_entity_size_disabled);
>  	attr->wait_on_time = MLX5_GET(cmd_hca_cap, hcattr, wait_on_time);
>  	attr->crypto = MLX5_GET(cmd_hca_cap, hcattr, crypto);
> -	if (attr->crypto)
> -		attr->aes_xts = MLX5_GET(cmd_hca_cap, hcattr, aes_xts);
>  	attr->ct_offload = !!(MLX5_GET64(cmd_hca_cap, hcattr,
>  					 general_obj_types) &
> 
> MLX5_GENERAL_OBJ_TYPES_CAP_CONN_TRACK_OFFLOAD);
>  	attr->rq_delay_drop = MLX5_GET(cmd_hca_cap, hcattr, rq_delay_drop);
> +	if (attr->crypto) {
> +		attr->aes_xts = MLX5_GET(cmd_hca_cap, hcattr, aes_xts);
> +		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
> +				MLX5_GET_HCA_CAP_OP_MOD_CRYPTO |
> +				MLX5_HCA_CAP_OPMOD_GET_CUR);
> +		if (!hcattr)
> +			return -1;
> +		attr->crypto_wrapped_import_method =
> !!(MLX5_GET(crypto_caps,
> +						hcattr,
> wrapped_import_method)
> +						& 1 << 2);
> +	}
>  	if (hca_cap_2_sup) {
>  		hcattr = mlx5_devx_get_hca_cap(ctx, in, out, &rc,
> 
> 	MLX5_GET_HCA_CAP_OP_MOD_GENERAL_DEVICE_2 |
> diff --git a/drivers/common/mlx5/mlx5_devx_cmds.h
> b/drivers/common/mlx5/mlx5_devx_cmds.h
> index 1bac18c59d..3747ef9e33 100644
> --- a/drivers/common/mlx5/mlx5_devx_cmds.h
> +++ b/drivers/common/mlx5/mlx5_devx_cmds.h
> @@ -254,6 +254,7 @@ struct mlx5_hca_attr {
>  	uint32_t umr_indirect_mkey_disabled:1;
>  	uint32_t log_min_stride_wqe_sz:5;
>  	uint32_t esw_mgr_vport_id_valid:1; /* E-Switch Mgr vport ID is valid. */
> +	uint32_t crypto_wrapped_import_method:1;
>  	uint16_t esw_mgr_vport_id; /* E-Switch Mgr vport ID . */
>  	uint16_t max_wqe_sz_sq;
>  };
> diff --git a/drivers/common/mlx5/mlx5_prm.h
> b/drivers/common/mlx5/mlx5_prm.h
> index 44b18225f6..d04c9e32c9 100644
> --- a/drivers/common/mlx5/mlx5_prm.h
> +++ b/drivers/common/mlx5/mlx5_prm.h
> @@ -1293,6 +1293,7 @@ enum {
>  	MLX5_GET_HCA_CAP_OP_MOD_NIC_FLOW_TABLE = 0x7 << 1,
>  	MLX5_SET_HCA_CAP_OP_MOD_ESW = 0x9 << 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_VDPA_EMULATION = 0x13 << 1,
> +	MLX5_GET_HCA_CAP_OP_MOD_CRYPTO = 0x1A << 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_PARSE_GRAPH_NODE_CAP = 0x1C <<
> 1,
>  	MLX5_GET_HCA_CAP_OP_MOD_GENERAL_DEVICE_2 = 0x20 << 1,
>  };
> @@ -3794,6 +3795,34 @@ struct mlx5_ifc_crypto_operational_register_bits {
>  	u8 reserved_at_280[0x180];
>  };
> 
> +struct mlx5_ifc_crypto_caps_bits {
> +	u8 wrapped_crypto_operational[0x1];
> +	u8 wrapped_crypto_going_to_commissioning[0x1];
> +	u8 sw_wrapped_dek[0x1];
> +	u8 synchronize_dek[0x1];
> +	u8 int_kek_manual[0x1];
> +	u8 int_kek_auto[0x1];
> +	u8 reserved_at_6[0x12];
> +	u8 wrapped_import_method[0x8];
> +	u8 reserved_at_20[0x3];
> +	u8 log_dek_max_alloc[0x5];
> +	u8 reserved_at_28[0x3];
> +	u8 log_max_num_deks[0x5];
> +	u8 reserved_at_30[0x3];
> +	u8 log_max_num_import_keks[0x5];
> +	u8 reserved_at_38[0x3];
> +	u8 log_max_num_creds[0x5];
> +	u8 failed_selftests[0x10];
> +	u8 num_nv_import_keks[0x8];
> +	u8 num_nv_credentials[0x8];
> +	u8 reserved_at_60[0x3];
> +	u8 log_dek_granularity[0x5];
> +	u8 reserved_at_68[0x3];
> +	u8 log_max_num_int_kek[0x5];
> +	u8 reserved_at_70[0x10];
> +	u8 reserved_at_80[0x780];
> +};
> +
>  struct mlx5_ifc_crypto_commissioning_register_bits {
>  	u8 token[0x1]; /* TODO: add size after PRM update */
>  };
> --
> 2.21.0
> 
> 
> 
> ------------------------------
> 
> Message: 2
> Date: Mon, 18 Apr 2022 11:33:01 +0000
> From: Gagandeep Singh <G.Singh@nxp.com>
> To: Stephen Hemminger <stephen@networkplumber.org>, Ferruh Yigit
> 	<ferruh.yigit@xilinx.com>
> Cc: Harold Huang <baymaxhuang@gmail.com>, "dev@dpdk.org"
> 	<dev@dpdk.org>
> Subject: RE: [PATCH] kni: update kernel API to receive packets
> Message-ID:
> 	<AS8PR04MB8198A3AD1035DBE4BC9169D7E1F39@AS8PR04MB8198.e
> urprd04.prod.outlook.com>
> 
> Content-Type: text/plain; charset="us-ascii"
> 
> 
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Friday, April 15, 2022 8:30 PM
> > To: Ferruh Yigit <ferruh.yigit@xilinx.com>
> > Cc: Gagandeep Singh <G.Singh@nxp.com>; Harold Huang
> > <baymaxhuang@gmail.com>; dev@dpdk.org
> > Subject: Re: [PATCH] kni: update kernel API to receive packets
> >
> > On Fri, 15 Apr 2022 13:30:33 +0100
> > Ferruh Yigit <ferruh.yigit@xilinx.com> wrote:
> >
> > > >> But this change would cause KNI kernel module does not work in the
> > > >> old kernel without this patch. I suggested using netif_rx_ni to keep
> > compatibility.
> > > >
> > > > netif_rx() API exists from very older versions of kernel before
> > > > v2.6. There will be no compilation issues. Only difference was,
> > > > netif_rx_ni() can be used in noninterrupt contexts to improve performance.
> > >
> > > May not be compilation issue, but with old kernels won't the behavior
> > > be different when 'netif_rx_ni()' switched to 'netif_rx()
> >
> > Probably best handled by #ifdef on kernel version but will be a mess for
> > backports to distro kernels.
> >
> > Looks like:
> >
> > 	Older	   -> New
> > 	netif_rx_ni   netif_rx
> > 	neitf_rx      __netif_rx
> 
> If it is ok for everyone, I can add #ifdef for kernel version >= 5.17 to use API
> netif_rx, to
> avoid any functional/performance impact with old kernels.
> 
> 
> ------------------------------
> 
> Message: 3
> Date: Mon, 18 Apr 2022 13:20:10 +0000
> From: "McDaniel, Timothy" <timothy.mcdaniel@intel.com>
> To: "dev@dpdk.org" <dev@dpdk.org>
> Subject: rte_bus_probe() does not exit on error
> Message-ID:
> 	<SN6PR11MB3103A5A4CE6D4C5846FDAB999EF39@SN6PR11MB3103.n
> amprd11.prod.outlook.com>
> 
> Content-Type: text/plain; charset="us-ascii"
> 
> Hello DPDK community,
> 
> We are looking into an issue where we pass an invalid command line argument
> to a vdev, and we print out an error message but don't exit.  This is an issue with
> all of the command line options that we handle in dlb2_parse_params().  In a
> nutshell, it looks like when we encounter an error in parsing, the error code is
> propagated to event_dlb2_vdev_probe() (event_dlb2_pci_probe() for PF), which
> is called by EAL during device probe in rte_bus_probe().  The problem is that
> rte_bus_probe() calls the .probe function for each device but doesn't exit on
> error:
> 
> if (vbus) {
>                 ret = vbus->probe();
>                 if (ret)
>                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
> }
> return 0;
> 
> We want to exit if the command line arguments can't be parsed, so we have a
> couple of options:
> 
> 
>   1.  In the DLB PMD, if we get an error while parsing parameters, exit right
> away.
>   2.  Change the behavior of rte_bus_probe() so that it propagates the error,
> which causes the program to exit due to EAL error.
> 
> if (vbus) {
>                 ret = vbus->probe();
>                 if (ret) {
>                         RTE_LOG(ERR, EAL, "Bus (%s) probe failed.\n", vbus->name);
>                         return ret;
>                 }
> }
> return 0;
> 
> Any preference, or other ideas?  Option 1 is simple and contained in our code,
> but does call rte_exit() from library code.  I'm not sure if that's really an issue
> because we do want to exit if the DLB-specific options are malformed.  Option 2
> is simple and seems like a better option, but requires changes outside of DLB, so
> may be more difficult to upstream?  (There may be reasons why the error is
> ignored for some devices).  Let me know what you think.
> 
> Thanks you for your comments,
> Tim McDaniel
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL:
> <http://mails.dpdk.org/archives/dev/attachments/20220418/aa32380f/attachm
> ent.htm>
> 
> End of dev Digest, Vol 400, Issue 6
> ***********************************