Re: [PATCH net-next v2 1/9] net: microchip: vcap: fix rule move for rules of coprime size

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