Re: [PATCH net-net] nfp: flower: inform firmware of flower features in the driver

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 13 May 2020 10:17:23 +0200 Simon Horman wrote:
> From: Louis Peens <louis.peens@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.peens@netronome.com>
> Signed-off-by: Simon Horman <simon.horman@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;