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:48:06 +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 7/9] net: microchip: sparx5: Writing rules to
 the IS2 VCAP
Message-ID: <20221020074806.ys7lyfkn7f7zpkcp@wse-c0155>
Organization: Westermo Network Technologies AB
References: <20221019114215.620969-1-steen.hegelund@microchip.com>
 <20221019114215.620969-8-steen.hegelund@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019114215.620969-8-steen.hegelund@microchip.com>
Precedence: bulk

On 2022-10-19 13:42, Steen Hegelund wrote:
> +static void vcap_iter_skip_tg(struct vcap_stream_iter *itr)
> +{
> +	/* Compensate the field offset for preceding typegroups */
> +	while (itr->tg->width && itr->offset >= itr->tg->offset) {
> +		itr->offset += itr->tg->width;
> +		itr->tg++; /* next typegroup */
> +	}
> +}

It was not immediately obvious to me why it should stop iterating when
tg->width is zero. But after some digging I saw that the tg iterators
always ends with an empty element (all bits zero, and therefore width is
zero). Could this be made clearer? Or maybe this is something common
that I'm just not used to seeing.

> +static void vcap_encode_bit(u32 *stream, struct vcap_stream_iter *itr, bool val)
> +{
> +	/* When intersected by a type group field, stream the type group bits
> +	 * before continuing with the value bit
> +	 */
> +	while (itr->tg->width &&
> +	       itr->offset >= itr->tg->offset &&
> +	       itr->offset < itr->tg->offset + itr->tg->width) {
> +		int tg_bitpos = itr->tg->offset - itr->offset;
> +
> +		vcap_set_bit(stream, itr, (itr->tg->value >> tg_bitpos) & 0x1);
> +		itr->offset++;
> +		vcap_iter_update(itr);
> +	}

Same as above.

> +static void vcap_encode_typegroups(u32 *stream, int sw_width,
> +				   const struct vcap_typegroup *tg,
> +				   bool mask)
> +{
> +	struct vcap_stream_iter iter;
> +	int idx;
> +
> +	/* Mask bits must be set to zeros (inverted later when writing to the
> +	 * mask cache register), so that the mask typegroup bits consist of
> +	 * match-1 or match-0, or both
> +	 */
> +	vcap_iter_set(&iter, sw_width, tg, 0);
> +	while (iter.tg->width) {
> +		/* Set position to current typegroup bit */
> +		iter.offset = iter.tg->offset;
> +		vcap_iter_update(&iter);
> +		for (idx = 0; idx < iter.tg->width; idx++) {
> +			/* Iterate over current typegroup bits. Mask typegroup
> +			 * bits are always set
> +			 */
> +			if (mask)
> +				vcap_set_bit(stream, &iter, 0x1);
> +			else
> +				vcap_set_bit(stream, &iter,
> +					     (iter.tg->value >> idx) & 0x1);
> +			iter.offset++;
> +			vcap_iter_update(&iter);
> +		}
> +		iter.tg++; /* next typegroup */
> +	}

Same as above.

Tested on Microchip PCB135 switch.

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

Best Regards,
Casper