Re: [PATCH net-next 3/3] octeontx2-af: add support for custom KPU entries

[Stanislaw Kardach <skardach@marvell.com>]

> ________________________________________
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Friday, September 11, 2020 10:53 PM
> To: Stanislaw Kardach [C]
> Cc: davem@davemloft.net; Sunil Kovvuri Goutham; netdev@vger.kernel.org
> Subject: [EXT] Re: [PATCH net-next 3/3] octeontx2-af: add support for custom KPU entries
>
> External Email
>
> ----------------------------------------------------------------------
> On Fri, 11 Sep 2020 15:21:24 +0200 skardach@marvell.com wrote:
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> > index 6bfb9a9d3003..fe164b85adfb 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> > @@ -148,7 +148,7 @@ struct npc_kpu_profile_cam {
> >       u16 dp1_mask;
> >       u16 dp2;
> >       u16 dp2_mask;
> > -};
> > +} __packed;
> >
> >  struct npc_kpu_profile_action {
> >       u8 errlev;
> > @@ -168,7 +168,7 @@ struct npc_kpu_profile_action {
> >       u8 mask;
> >       u8 right;
> >       u8 shift;
> > -};
> > +} __packed;
> >
> >  struct npc_kpu_profile {
> >       int cam_entries;
> > @@ -323,6 +323,15 @@ struct npc_mcam_kex {
> >       u64 intf_ld_flags[NPC_MAX_INTF][NPC_MAX_LD][NPC_MAX_LFL];
> >  } __packed;
> >
> > +struct npc_kpu_fwdata {
> > +     int     entries;
> > +     /* What follows is:
> > +      * struct npc_kpu_profile_cam[entries];
> > +      * struct npc_kpu_profile_action[entries];
> > +      */
> > +     u8      data[0];
> > +} __packed;
>
> Why do you mark a structure with a single int member as __packed?
>
> Please drop all the __packed attrs you add in this series.

Sorry, will fix in V2.

>
> >  module_param(mkex_profile, charp, 0000);
> >  MODULE_PARM_DESC(mkex_profile, "MKEX profile name string");
> >
> > +static char *kpu_profile; /* KPU profile name */
> > +module_param(kpu_profile, charp, 0000);
> > +MODULE_PARM_DESC(kpu_profile, "KPU profile name string");
>
> Why do you need a module parameter for this?
>
> Just decide on a filename, always request it, and if user doesn't want
> to load a special profile you'll get a -ENOENT and move on.

The use-case I am targeting here is using different profiles in
different parts of the network but re-using a single initramfs+kernel
(say a common set of platform software) via tftpboot. I.e. one custom
protocol for the edge and another for core network. Then boot parameters
are customized based on some configuration embedded in the equipment
(i.e. device tree in flash + customized uboot).
Without module parameter I think that could be done via symlinking and
delaying module loading but that means more hassle when driver is
built-in, hence the parameter.

Thanks for reviewing my patches!

Best Regards,
Stanislaw Kardach