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: fix rule move for rules of coprime size
>
> This reworks the VCAP rule move logic so that insert and delete compute a
> single uniform address offset, aligned to the LCM of the rule sizes in the
> moved block, instead of per-rule "optimal" offsets. The goal is to keep the
> software rule addresses in sync with what the hardware move command does,
> which becomes necessary once rules of coprime sizes (2 and 3) appear with
> the LPM VCAP.
>
> One question about the delete algorithm description in the changelog.
>
> > On delete:
> >
> > 1. Initialize HW memory at the deleted rule's addresses before
> > moving, since the moved block may not fully cover them due to
> > padding.
> > 2. Reclaim any padding between rules in the block.
>
> [Severity: Low]
> Is step 2 accurate for the new implementation? As far as I can tell the
> padding that sits between rules inside the moved block is preserved, not
> reclaimed, because a single offset is applied to every rule in the block:
>
> drivers/net/ethernet/microchip/vcap/vcap_api.c:vcap_move_rules_sw() {
> list_for_each_entry_continue(pos, &admin->rules, list)
> pos->addr -= move->offset;
> }
>
> Since every rule in the block is shifted by the same move->offset, the gaps
> between rules within the block are structurally invariant across the move.
>
> What appears to be reclaimed is the deleted rule's own space plus the
> padding adjacent to its position, and only down to block_lcm granularity.
> In the delete path unaligned_offset is always negative (block_first->addr +
> block_first->size <= ri->addr <= base_addr), so the truncating branch is
> taken in vcap_move_from_block():
>
> drivers/net/ethernet/microchip/vcap/vcap_api.c:vcap_move_from_block() {
> unaligned_offset = (block_first->addr + block_first->size) -
> base_addr;
> if (unaligned_offset > 0)
> move->offset =
> ((unaligned_offset + (block_lcm - 1)) / block_lcm) *
> block_lcm;
> else
> move->offset = (unaligned_offset / block_lcm) * block_lcm;
> }
>
> Integer division truncating toward zero leaves up to block_lcm - 1 addresses
> of the gap unreclaimed at the boundary.
>
> Could the changelog wording be adjusted to describe what is actually
> reclaimed, for example the deleted rule's addresses plus adjacent padding,
> rounded down to the block LCM?
>
Yes I will update the commit message, this is poorly worded.
> > diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > index 788c0728d7636..75c2ca54fddd2 100644
> > --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> > +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
>
> [ ... ]
>
> > @@ -2106,12 +2108,63 @@ static u32 vcap_set_rule_id(struct
> > vcap_rule_internal *ri)
> > return ri->data.id;
> > }
> >
> > +static void vcap_move_from_block(u32 base_addr,
> > + struct vcap_rule_internal *block_first,
> > + struct vcap_rule_internal *block_last,
> > + int block_lcm, struct vcap_rule_move *move)
> > +{
> > + int unaligned_offset;
> > +
> > + if (!block_first || !block_last)
> > + return;
> > +
> > + move->addr = block_last->addr;
> > + move->count = block_first->addr + block_first->size -
> > block_last->addr;
> > + /* Integer division rounds toward zero. We want to round toward +inf
> > + * for the positive case (insertion) to ensure enough room.
> > + */
> > + unaligned_offset = (block_first->addr + block_first->size) -
> > base_addr;
> > + if (unaligned_offset > 0)
> > + move->offset =
> > + ((unaligned_offset + (block_lcm - 1)) / block_lcm) *
> > + block_lcm;
> > + else
> > + move->offset = (unaligned_offset / block_lcm) * block_lcm;
> > +}
> > +
> > +static void vcap_move_rules_sw(struct vcap_admin *admin,
> > + struct vcap_rule_internal *pos,
> > + struct vcap_rule_move const *move)
> > +{
> > + if (move->count == 0 || move->offset == 0)
> > + return;
> > +
> > + list_for_each_entry_continue(pos, &admin->rules, list)
> > + pos->addr -= move->offset;
> > +}
>
> [ ... ]
> --
> pw-bot: cr
