Re: [PATCH net-next v3 1/4] net: microchip: vcap: Add vcap_get_rule

[Horatiu Vultur]

The 12/06/2022 13:31, Paolo Abeni wrote:
> 
> Hello,

Hi Paolo,

> 
> On Sat, 2022-12-03 at 11:43 +0100, Horatiu Vultur wrote:
> > @@ -632,32 +264,22 @@ static int vcap_show_admin(struct vcap_control *vctrl,
> >                          struct vcap_admin *admin,
> >                          struct vcap_output_print *out)
> >  {
> > -     struct vcap_rule_internal *elem, *ri;
> > +     struct vcap_rule_internal *elem;
> > +     struct vcap_rule *vrule;
> >       int ret = 0;
> >
> >       vcap_show_admin_info(vctrl, admin, out);
> > -     mutex_lock(&admin->lock);
> >       list_for_each_entry(elem, &admin->rules, list) {
> 
> Not strictly related to this patch, as the patter is AFAICS already
> there in other places, but I'd like to understand the admin->rules
> locking schema.

According to the commit message that introduced this lock [0] and the
comment to the lock, this lock is used to protect the access to the
admin->rules, admin->enabled and the caches which means the access to
the HW (to read/write the rules).
> 
> It looks like addition/removal are protected by admin->lock, but
> traversal is usually lockless, which in turn looks buggy ?!?

Thanks for the observation! You are correct, there seems to be some bugs
regarding the usage of this lock. We will look over this and will send a
patch to fix this.

> 
> Note: as this patch does not introduce the mentioned behavior, I'm not
> going to block the series for the above.

[0] 71c9de995260 ("net: microchip: sparx5: Add VCAP locking to protect rules")
> 
> Thanks,
> 
> Paolo
> > -             ri = vcap_dup_rule(elem);
> > -             if (IS_ERR(ri)) {
> > -                     ret = PTR_ERR(ri);
> > -                     goto err_unlock;
> > +             vrule = vcap_get_rule(vctrl, elem->data.id);
> > +             if (IS_ERR_OR_NULL(vrule)) {
> > +                     ret = PTR_ERR(vrule);
> > +                     break;
> >               }
> > -             /* Read data from VCAP */
> > -             ret = vcap_read_rule(ri);
> > -             if (ret)
> > -                     goto err_free_rule;
> > +
> >               out->prf(out->dst, "\n");
> > -             vcap_show_admin_rule(vctrl, admin, out, ri);
> > -             vcap_free_rule((struct vcap_rule *)ri);
> > +             vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
> > +             vcap_free_rule(vrule);
> >       }
> > -     mutex_unlock(&admin->lock);
> > -     return 0;
> > -
> > -err_free_rule:
> > -     vcap_free_rule((struct vcap_rule *)ri);
> > -err_unlock:
> > -     mutex_unlock(&admin->lock);
> >       return ret;
> >  }
> 

-- 
/Horatiu