On Thu, May 28, 2026 at 04:13:59PM +0200, Burakov, Anatoly wrote:
> On 5/28/2026 3:13 PM, Bruce Richardson wrote:
> > On Thu, May 28, 2026 at 02:39:47PM +0200, Burakov, Anatoly wrote:
> > > On 5/27/2026 3:28 PM, Bruce Richardson wrote:
> > > > On Mon, May 25, 2026 at 03:06:23PM +0100, Anatoly Burakov wrote:
> > > > > There are a lot of commonalities between what kinds of flow attr each
> > > > > Intel
> > > > > driver supports. Add a helper function that will validate attr based
> > > > > on
> > > > > common requirements and (optional) parameter checks.
> > > > >
> > > > > Things we check for:
> > > > > - Rejecting NULL attr (obviously)
> > > > > - Default to ingress flows
> > > > > - Transfer, group, priority, and egress are not allowed unless
> > > > > requested
> > > > >
> > > > > Signed-off-by: Anatoly Burakov <[email protected]>
> > > > > ---
> > > > > drivers/net/intel/common/flow_check.h | 69
> > > > > +++++++++++++++++++++++++++
> > > > > 1 file changed, 69 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/intel/common/flow_check.h
> > > > > b/drivers/net/intel/common/flow_check.h
> > > > > index 74fb28ae3d..0572028664 100644
> > > > > --- a/drivers/net/intel/common/flow_check.h
> > > > > +++ b/drivers/net/intel/common/flow_check.h
> > > > > @@ -54,6 +54,7 @@ ci_flow_action_type_in_list(const enum
> > > > > rte_flow_action_type type,
> > > > > /* Forward declarations */
> > > > > struct ci_flow_actions;
> > > > > struct ci_flow_actions_check_param;
> > > > > +struct ci_flow_attr_check_param;
> > > > > static inline const char *
> > > > > ci_flow_action_type_to_str(enum rte_flow_action_type type)
> > > > > @@ -271,6 +272,74 @@ ci_flow_check_actions(const struct
> > > > > rte_flow_action *actions,
> > > > > return parsed_actions->count == 0 ? -EINVAL : 0;
> > > > > }
> > > > > +/**
> > > > > + * Parameter structure for attr check.
> > > > > + */
> > > > > +struct ci_flow_attr_check_param {
> > > > > + bool allow_priority; /**< True if priority attribute is
> > > > > allowed. */
> > > > > + bool allow_transfer; /**< True if transfer attribute is
> > > > > allowed. */
> > > > > + bool allow_group; /**< True if group attribute is allowed. */
> > > > > + bool expect_egress; /**< True if egress attribute is expected.
> > > > > */
> > > > > +};
> > > > > +
> > > > > +/**
> > > > > + * Validate rte_flow_attr structure against specified constraints.
> > > > > + *
> > > > > + * @param attr Pointer to rte_flow_attr structure to validate.
> > > > > + * @param attr_param Pointer to ci_flow_attr_check_param structure
> > > > > specifying constraints.
> > > > > + * @param error Pointer to rte_flow_error structure for error
> > > > > reporting.
> > > > > + *
> > > > > + * @return 0 on success, negative errno on failure.
> > > > > + */
> > > > > +static inline int
> > > > > +ci_flow_check_attr(const struct rte_flow_attr *attr,
> > > > > + const struct ci_flow_attr_check_param *attr_param,
> > > > > + struct rte_flow_error *error)
> > > > > +{
> > > > > + if (attr == NULL) {
> > > > > + return rte_flow_error_set(error, EINVAL,
> > > > > + RTE_FLOW_ERROR_TYPE_ATTR,
> > > > > attr,
> > > > > + "NULL attribute");
> > > > > + }
> > > > > +
> > > > > + /* Direction must be either ingress or egress */
> > > > > + if (attr->ingress == attr->egress) {
> > > > > + return rte_flow_error_set(error, EINVAL,
> > > > > + RTE_FLOW_ERROR_TYPE_ATTR,
> > > > > attr,
> > > > > + "Either ingress or egress
> > > > > must be set");
> > > > > + }
> > > > > +
> > > > > + /* Expect ingress by default */
> > > > > + if (attr->egress && (attr_param == NULL ||
> > > > > !attr_param->expect_egress)) {
> > > > > + return rte_flow_error_set(error, EINVAL,
> > > > > +
> > > > > RTE_FLOW_ERROR_TYPE_ATTR_EGRESS, attr,
> > > > > + "Egress not supported");
> > > > > + }
> > > >
> > > > I think "allow_egress" is possibly a better name here. "expect_egress"
> > > > implies that egress must be present, but the logic here seems to be only
> > > > allowing egress if the flag is provided.
> > >
> > > No, the check right above this prevents unspecified egress *if* ingress is
> > > also specified.
> >
> > So the error message above should be "Either ingress or egress must be set,
> > but not both"?
>
> Sure, yes, a rulle cannot be both egress and ingress.
>
> >
> > > Meaning, one of egress/ingress *must* be specified, and if
> > > it's egress, it is only allowed when we are expecting egress.
> > >
> > Yes, so it's either ingress or egress but egress is only allowed in some
> > cases. But nothing in this check prevents ingress alone when expect_egress
> > is set, so egress is allowed but not required, so I still think
> > "allow_egress" is a better name. "expect_egress" is a bit ambivilent, in
> > that it's unclear how strong that expectation is - is it an error, or just
> > a warning if the expectation is not met, for example?
>
> Rules cannot be both ingress and egress, and they cannot be neither ingress
> and egress. That leaves two options: either the rule is ingress, or the rule
> is egress.
>
> If the rule isn't ingress, it by definition will be egress in order to pass
> earlier checks. The intent is also that a rule that is "expected to be
> egress" must be egress to be accepted.
>
> However, now that I think of it, you're right in that the condition is
> subtly wrong. By default, we expect the rule to be ingress (i.e. not setting
> "expect egress" implies we are expecting ingress), but we only *check* if
> the rule is egress - we would *not* check if the rule is egress. I think the
> condition should be replaced with:
>
> 1) if attr_param == NULL, rule *cannot* ever be expected to be egress
> because we specify egress expectations in attr_param - so, rule being egress
> when attr_param == NULL is an error
> 2) if attr_param != NULL, egress *must* match expect_egress
>
> So, basically:
>
> if ((attr_param == NULL && attr->egress) || (attr->egress !=
> attr_param->expect_egress)
>
> this should better match the intent.
>
Yes, that is clearer. In this case, is "expect_egress" better renamed to
"egress_only" or "require_egress" to make it clear it's an absolute
requirement?
Also, just a suggestion for readability's sake, maybe merge the conditions
for checking ingress and egress but not both or neither into explicit check
branches for each case for clarity.
if (attr_param != NULL && attr_param->require_egress) {
/* this must be an egress rule, ingress unset, egress set */
if (attr->ingress != 0 || attr->egress != 1)
....
} else {
/* this must be an ingress rule, ingress set, egress unset */
if (attr->ingress != 1 || attr->egress != 0)
....
}
/Bruce
