On Wed, 13 May 2020 10:17:23 +0200 Simon Horman wrote:
> From: Louis Peens <louis.pe...@netronome.com>
>
> For backwards compatibility it may be required for the firmware to
> disable certain features depending on the features supported by
> the host. Combine the host feature bits and firmware feature bits
> and write this back to the firmware.
>
> Signed-off-by: Louis Peens <louis.pe...@netronome.com>
> Signed-off-by: Simon Horman <simon.hor...@netronome.com>
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.c
> b/drivers/net/ethernet/netronome/nfp/flower/main.c
> index d8ad9346a26a..2830c1203c76 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/main.c
> +++ b/drivers/net/ethernet/netronome/nfp/flower/main.c
> @@ -665,6 +665,81 @@ static int nfp_flower_vnic_init(struct nfp_app *app,
> struct nfp_net *nn)
> return err;
> }
>
> +static void nfp_flower_wait_host_bit(struct nfp_app *app)
> +{
> + unsigned long err_at;
> + u64 feat;
> + int err;
> +
> + /* Wait for HOST_ACK flag bit to propagate */
> + feat = nfp_rtsym_read_le(app->pf->rtbl,
> + "_abi_flower_combined_feat_global",
> + &err);
> + err_at = jiffies + HZ / 100;
msecs_to_jiffies() ?
> + while (!err && !(feat & NFP_FL_FEATS_HOST_ACK)) {
> + usleep_range(1000, 2000);
> + feat = nfp_rtsym_read_le(app->pf->rtbl,
> + "_abi_flower_combined_feat_global",
> + &err);
> + if (time_is_before_eq_jiffies(err_at)) {
> + nfp_warn(app->cpp,
> + "HOST_ACK bit not propagated in FW.\n");
> + break;
> + }
Probably could be better off with a do {} while (), but okay :)
> + }
No warning here if reading fails (err is set) ?
> +}
> +
> +static int nfp_flower_sync_feature_bits(struct nfp_app *app)
> +{
> + struct nfp_flower_priv *app_priv = app->priv;
> + int err = 0;
No need to init err.
> + /* Tell the firmware of the host supported features. */
> + err = nfp_rtsym_write_le(app->pf->rtbl, "_abi_flower_host_mask",
> + app_priv->flower_ext_feats |
> + NFP_FL_FEATS_HOST_ACK);
> + if (!err) {
> + nfp_flower_wait_host_bit(app);
> + } else if (err == -ENOENT) {
> + nfp_info(app->cpp,
> + "Telling FW of host capabilities not supported.\n");
Is this really important enough for users to know?
> + err = 0;
No need.
> + } else {
> + return err;
> + }
> +
> + /* Tell the firmware that the driver supports lag. */
> + err = nfp_rtsym_write_le(app->pf->rtbl,
> + "_abi_flower_balance_sync_enable", 1);
> + if (!err) {
> + app_priv->flower_ext_feats |= NFP_FL_FEATS_LAG;
> + nfp_flower_lag_init(&app_priv->nfp_lag);
> + } else if (err == -ENOENT) {
> + nfp_warn(app->cpp, "LAG not supported by FW.\n");
> + err = 0;
> + } else {
> + return err;
> + }
> +
> + if (app_priv->flower_ext_feats & NFP_FL_FEATS_FLOW_MOD) {
> + /* Tell the firmware that the driver supports flow merging. */
> + err = nfp_rtsym_write_le(app->pf->rtbl,
> + "_abi_flower_merge_hint_enable", 1);
> + if (!err) {
> + app_priv->flower_ext_feats |= NFP_FL_FEATS_FLOW_MERGE;
> + nfp_flower_internal_port_init(app_priv);
> + } else if (err == -ENOENT) {
> + nfp_warn(app->cpp,
> + "Flow merge not supported by FW.\n");
> + err = 0;
> + }
Could you just add an else { return err; } here and..
> + } else {
> + nfp_warn(app->cpp, "Flow mod/merge not supported by FW.\n");
> + }
> +
> + return err;
.. make this return 0? Then you won't have to clear err in all the
-ENOENT branches. Someone may forget to do that one day, and we'd have
a bug.
> +}
> +
> static int nfp_flower_init(struct nfp_app *app)
> {
> u64 version, features, ctx_count, num_mems;
> diff --git a/drivers/net/ethernet/netronome/nfp/flower/main.h
> b/drivers/net/ethernet/netronome/nfp/flower/main.h
> index d55d0d33bc45..dfc07611603e 100644
> --- a/drivers/net/ethernet/netronome/nfp/flower/main.h
> +++ b/drivers/net/ethernet/netronome/nfp/flower/main.h
> @@ -44,9 +44,20 @@ struct nfp_app;
> #define NFP_FL_FEATS_FLOW_MOD BIT(5)
> #define NFP_FL_FEATS_PRE_TUN_RULES BIT(6)
> #define NFP_FL_FEATS_IPV6_TUN BIT(7)
> +#define NFP_FL_FEATS_HOST_ACK BIT(31)
> #define NFP_FL_FEATS_FLOW_MERGE BIT(30)
> #define NFP_FL_FEATS_LAG BIT(31)
Could we perhaps rename those two bits and use a different variable to
store them (separate patch)? Looks a little suspicious that we reuse
BIT(31) now..
> +#define NFP_FL_FEATS_HOST \
> + (NFP_FL_FEATS_GENEVE | \
> + NFP_FL_NBI_MTU_SETTING | \
> + NFP_FL_FEATS_GENEVE_OPT | \
> + NFP_FL_FEATS_VLAN_PCP | \
> + NFP_FL_FEATS_VF_RLIM | \
> + NFP_FL_FEATS_FLOW_MOD | \
> + NFP_FL_FEATS_PRE_TUN_RULES | \
> + NFP_FL_FEATS_IPV6_TUN)
> +
> struct nfp_fl_mask_id {
> struct circ_buf mask_id_free_list;
> ktime_t *last_used;
