Re: [PATCH net-next v2 7/9] net: microchip: sparx5: Writing rules to the IS2 VCAP

[Steen Hegelund]

Hi Casper,

On Thu, 2022-10-20 at 09:48 +0200, Casper Andersson wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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.

Yes the empty element is just a list terminator, and it just makes the iteration simpler than using
a count which would have been stored in a separate location.
I could add a comment here to clarify this.

> 
> > +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.

Yes it also checks for the list terminator.

> 
> > +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.
Yes same procedure here.

> 
> Tested on Microchip PCB135 switch.
> 
> Tested-by: Casper Andersson <casper.casan@xxxxxxxxx>
> Reviewed-by: Casper Andersson <casper.casan@xxxxxxxxx>
> 
> Best Regards,
> Casper

Thanks again for the review and the target testing.

BR
Steen