Re: [PATCH net-next v3 2/5] vxlan: Expose helper vxlan_build_gbp_hdr

[Gavin Li]

On 2/20/2023 2:40 PM, Simon Horman wrote:
> External email: Use caution opening links or attachments
>
>
> On Mon, Feb 20, 2023 at 10:05:00AM +0800, Gavin Li wrote:
> > On 2/20/2023 4:32 AM, Simon Horman wrote:
> > > External email: Use caution opening links or attachments
> > >
> > >
> > > On Fri, Feb 17, 2023 at 05:39:22AM +0200, Gavin Li wrote:
> > > > vxlan_build_gbp_hdr will be used by other modules to build gbp option in
> > > > vxlan header according to gbp flags.
> > > >
> > > > Signed-off-by: Gavin Li <gavinl@xxxxxxxxxx>
> > > > Reviewed-by: Gavi Teitz <gavi@xxxxxxxxxx>
> > > > Reviewed-by: Roi Dayan <roid@xxxxxxxxxx>
> > > > Reviewed-by: Maor Dickman <maord@xxxxxxxxxx>
> > > > Acked-by: Saeed Mahameed <saeedm@xxxxxxxxxx>
> > > I do wonder if this needs to be a static inline function.
> > > But nonetheless,
> > Will get "unused-function" from gcc without "inline"
> >
> > ./include/net/vxlan.h:569:13: warning: ‘vxlan_build_gbp_hdr’ defined but not
> > used [-Wunused-function]
> >   static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct
> > vxlan_metadata *md)
> Right. But what I was really wondering is if the definition
> of the function could stay in drivers/net/vxlan/vxlan_core.c,
> without being static. And have a declaration in include/net/vxlan.h

Tried that the first time the function was called by driver code. It 
would introduce dependency in linking between the driver and the kernel 
module.

Do you think it's OK to have such dependency?

> > > Reviewed-by: Simon Horman <simon.horman@xxxxxxxxxxxx>
> > >
> > > > ---
> > > >    drivers/net/vxlan/vxlan_core.c | 19 -------------------
> > > >    include/net/vxlan.h            | 19 +++++++++++++++++++
> > > >    2 files changed, 19 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > > > index 86967277ab97..13faab36b3e1 100644
> > > > --- a/drivers/net/vxlan/vxlan_core.c
> > > > +++ b/drivers/net/vxlan/vxlan_core.c
> > > > @@ -2140,25 +2140,6 @@ static bool route_shortcircuit(struct net_device *dev, struct sk_buff *skb)
> > > >         return false;
> > > >    }
> > > >
> > > > -static void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, struct vxlan_metadata *md)
> > > > -{
> > > > -     struct vxlanhdr_gbp *gbp;
> > > > -
> > > > -     if (!md->gbp)
> > > > -             return;
> > > > -
> > > > -     gbp = (struct vxlanhdr_gbp *)vxh;
> > > > -     vxh->vx_flags |= VXLAN_HF_GBP;
> > > > -
> > > > -     if (md->gbp & VXLAN_GBP_DONT_LEARN)
> > > > -             gbp->dont_learn = 1;
> > > > -
> > > > -     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> > > > -             gbp->policy_applied = 1;
> > > > -
> > > > -     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> > > > -}
> > > > -
> > > >    static int vxlan_build_gpe_hdr(struct vxlanhdr *vxh, __be16 protocol)
> > > >    {
> > > >         struct vxlanhdr_gpe *gpe = (struct vxlanhdr_gpe *)vxh;
> > > > diff --git a/include/net/vxlan.h b/include/net/vxlan.h
> > > > index bca5b01af247..b6d419fa7ab1 100644
> > > > --- a/include/net/vxlan.h
> > > > +++ b/include/net/vxlan.h
> > > > @@ -566,4 +566,23 @@ static inline bool vxlan_fdb_nh_path_select(struct nexthop *nh,
> > > >         return true;
> > > >    }
> > > >
> > > > +static inline void vxlan_build_gbp_hdr(struct vxlanhdr *vxh, const struct vxlan_metadata *md)
> > > > +{
> > > > +     struct vxlanhdr_gbp *gbp;
> > > > +
> > > > +     if (!md->gbp)
> > > > +             return;
> > > > +
> > > > +     gbp = (struct vxlanhdr_gbp *)vxh;
> > > > +     vxh->vx_flags |= VXLAN_HF_GBP;
> > > > +
> > > > +     if (md->gbp & VXLAN_GBP_DONT_LEARN)
> > > > +             gbp->dont_learn = 1;
> > > > +
> > > > +     if (md->gbp & VXLAN_GBP_POLICY_APPLIED)
> > > > +             gbp->policy_applied = 1;
> > > > +
> > > > +     gbp->policy_id = htons(md->gbp & VXLAN_GBP_ID_MASK);
> > > > +}
> > > > +
> > > >    #endif
> > > > --
> > > > 2.31.1