Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Oct 2022 09:41:04 +0200
From:   Casper Andersson <casper.casan@gmail.com>
To:     Steen Hegelund <steen.hegelund@microchip.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, UNGLinuxDriver@microchip.com,
        Randy Dunlap <rdunlap@infradead.org>,
        Russell King <rmk+kernel@armlinux.org.uk>,
        Wan Jiabing <wanjiabing@vivo.com>,
        Nathan Huckleberry <nhuck@google.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH net-next v2 6/9] net: microchip: sparx5: Adding basic
 rule management in VCAP API
Message-ID: <20221020074104.qmow2fc66v4is2rk@wse-c0155>
Organization: Westermo Network Technologies AB
References: <20221019114215.620969-1-steen.hegelund@microchip.com>
 <20221019114215.620969-7-steen.hegelund@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019114215.620969-7-steen.hegelund@microchip.com>
Precedence: bulk

On 2022-10-19 13:42, Steen Hegelund wrote:
> +/* Write VCAP cache content to the VCAP HW instance */
> +static int vcap_write_rule(struct vcap_rule_internal *ri)
> +{
> +	struct vcap_admin *admin = ri->admin;
> +	int sw_idx, ent_idx = 0, act_idx = 0;
> +	u32 addr = ri->addr;
> +
> +	if (!ri->size || !ri->keyset_sw_regs || !ri->actionset_sw_regs) {
> +		pr_err("%s:%d: rule is empty\n", __func__, __LINE__);
> +		return -EINVAL;
> +	}
> +	/* Use the values in the streams to write the VCAP cache */
> +	for (sw_idx = 0; sw_idx < ri->size; sw_idx++, addr++) {
> +		ri->vctrl->ops->cache_write(ri->ndev, admin,
> +					VCAP_SEL_ENTRY, ent_idx,
> +					ri->keyset_sw_regs);
> +		ri->vctrl->ops->cache_write(ri->ndev, admin,
> +					VCAP_SEL_ACTION, act_idx,
> +					ri->actionset_sw_regs);
> +		ri->vctrl->ops->update(ri->ndev, admin, VCAP_CMD_WRITE,
> +				   VCAP_SEL_ALL, addr);

Arguments not aligned with opening parenthesis.

>  /* Validate a rule with respect to available port keys */
>  int vcap_val_rule(struct vcap_rule *rule, u16 l3_proto)
>  {
>  	struct vcap_rule_internal *ri = to_intrule(rule);
> +	enum vcap_keyfield_set keysets[10];
> +	struct vcap_keyset_list kslist;
> +	int ret;
>  
>  	/* This validation will be much expanded later */
> +	ret = vcap_api_check(ri->vctrl);
> +	if (ret)
> +		return ret;
>  	if (!ri->admin) {
>  		ri->data.exterr = VCAP_ERR_NO_ADMIN;
>  		return -EINVAL;
> @@ -113,14 +304,41 @@ int vcap_val_rule(struct vcap_rule *rule, u16 l3_proto)
>  		ri->data.exterr = VCAP_ERR_NO_KEYSET_MATCH;
>  		return -EINVAL;
>  	}
> +	/* prepare for keyset validation */
> +	keysets[0] = ri->data.keyset;
> +	kslist.keysets = keysets;
> +	kslist.cnt = 1;
> +	/* Pick a keyset that is supported in the port lookups */
> +	ret = ri->vctrl->ops->validate_keyset(ri->ndev, ri->admin, rule, &kslist,
> +					      l3_proto);
> +	if (ret < 0) {
> +		pr_err("%s:%d: keyset validation failed: %d\n",
> +		       __func__, __LINE__, ret);
> +		ri->data.exterr = VCAP_ERR_NO_PORT_KEYSET_MATCH;
> +		return ret;
> +	}
>  	if (ri->data.actionset == VCAP_AFS_NO_VALUE) {
>  		ri->data.exterr = VCAP_ERR_NO_ACTIONSET_MATCH;
>  		return -EINVAL;
>  	}
> -	return 0;
> +	vcap_add_type_keyfield(rule);
> +	/* Add default fields to this rule */
> +	ri->vctrl->ops->add_default_fields(ri->ndev, ri->admin, rule);
> +
> +	/* Rule size is the maximum of the entry and action subword count */
> +	ri->size = max(ri->keyset_sw, ri->actionset_sw);
> +
> +	/* Finally check if there is room for the rule in the VCAP */
> +	return vcap_rule_space(ri->admin, ri->size);
>  }
>  EXPORT_SYMBOL_GPL(vcap_val_rule);

Validating a rule also modifies it. I think validation and modification
should generally be kept apart. But it looks like it might be hard with
the current design since you need to add the fields to then check the
space it takes, and the rule sizes can depend on the hardware.

Tested on Microchip PCB135 switch.

Tested-by: Casper Andersson <casper.casan@gmail.com>
Reviewed-by: Casper Andersson <casper.casan@gmail.com>