Re: [PATCH net-next v2 3/9] net: microchip: vcap: make vcap actionset decoding type_id aware

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

On Mon, 2026-08-17 at 14:13 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: microchip: vcap: make vcap actionset decoding type_id aware
> 
> This adds a helper, vcap_find_stream_type_id(), that pulls the type_id
> bits out of stream[0] just after the typegroup bits, and uses it in
> vcap_verify_actionstream_actionset() to compare the decoded type_id
> against info->type_id instead of always returning true.  The code change
> itself looks fine; the comment below is only about the commit message.
> 
> > net: microchip: vcap: make vcap actionset decoding type_id aware
> > 
> > When reading a rule back from hardware, decoding has to identify which
> > actionset the rule was written as. The existing logic was only aware of
> > the actionset subword length, which cannot distinguish actionsets that
> > share a subword length but differ in their type_id field. The LPM VCAP
> > added in a following patch introduces this case: ARP_PTR, L3MC_PTR and
> > ARP_ENTRY all occupy one subword and differ only by type_id.
> 
> [Severity: Low]
> Should the sentence "The LPM VCAP added in a following patch introduces
> this case: ARP_PTR, L3MC_PTR and ARP_ENTRY all occupy one subword and
> differ only by type_id" be reworded?
> 
> Two details don't seem to match the tree.  First, there is no L3MC_PTR
> symbol anywhere at this commit; grepping for L3MC under
> drivers/net/ethernet/microchip/ only matches the unrelated register name
> GA_ANA_L3_VLAN_ARP_L3MC_STICKY.  The one-subword LPM actionsets that do
> exist are just these two, in sparx5_vcap_ag_api.c:
> 
> static const struct vcap_set lpm_actionfield_set[] = {
>         [VCAP_AFS_ARP_PTR] = {
>                 .type_id = 0,
>                 .sw_per_item = 1,
>                 .sw_cnt = 12,
>         },
>         [VCAP_AFS_ARP_ENTRY] = {
>                 .type_id = 2,
>                 .sw_per_item = 1,
>                 .sw_cnt = 12,
>         },
> };
> 
> Second, the colliding actionset metadata is not added in a following
> patch; lpm_actionfield_set, lpm_actionfield_set_map and the
> sparx5_vcaps[VCAP_TYPE_LPM] entry that references them are already
> present at this commit, added by the preceding "net: microchip: vcap: add
> lpm vcap to autogen vcap api".  Only the sparx5/lan969x LPM driver
> enablement lands later.
> 
> Could the message say the LPM actionset model is added by the preceding
> patch, and drop the L3MC_PTR reference?  The premise that two actionsets
> share sw_per_item = 1 and differ only in type_id does hold, so the change
> itself still looks justified.

Yes I will update the message, and remove the stale remarks.