Re: [RFC PATCH 2/3] net/ncsi: Fix several packet definitions

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2017-08-10 at 16:33 +0930, Joel Stanley wrote:
> On Wed, Aug 9, 2017 at 6:24 PM, Samuel Mendoza-Jonas
> <sam@mendozajonas.com> wrote:
> > Signed-off-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> > ---
> >  net/ncsi/ncsi-cmd.c | 10 +++++-----
> >  net/ncsi/ncsi-pkt.h |  2 +-
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index db7083bfd476..1fec9fda7f60 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -146,9 +146,9 @@ static int ncsi_cmd_handler_svf(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_svf_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->vlan = htons(nca->words[0]);
> > -       cmd->index = nca->bytes[2];
> > -       cmd->enable = nca->bytes[3];
> > +       cmd->vlan = htons(nca->words[1]);
> > +       cmd->index = nca->bytes[6];
> > +       cmd->enable = nca->bytes[7];
> 
> These look like straight up bugs. Should we send them off as fixes?

This shows how completely wrong that whole "magic byte/words array
argumetns" thing is, especially when there *is* a proper struct
definitions for the packets :-( We definitely need that fixed too asap.

> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -161,7 +161,7 @@ static int ncsi_cmd_handler_ev(struct sk_buff *skb,
> > 
> >         cmd = (struct ncsi_cmd_ev_pkt *)skb_put(skb, sizeof(*cmd));
> >         memset(cmd, 0, sizeof(*cmd));
> > -       cmd->mode = nca->bytes[0];
> > +       cmd->mode = nca->bytes[3];
> >         ncsi_cmd_build_header(&cmd->cmd.common, nca);
> > 
> >         return 0;
> > @@ -240,7 +240,7 @@ static struct ncsi_cmd_handler {
> >         { NCSI_PKT_CMD_AE,     8, ncsi_cmd_handler_ae      },
> >         { NCSI_PKT_CMD_SL,     8, ncsi_cmd_handler_sl      },
> >         { NCSI_PKT_CMD_GLS,    0, ncsi_cmd_handler_default },
> > -       { NCSI_PKT_CMD_SVF,    4, ncsi_cmd_handler_svf     },
> > +       { NCSI_PKT_CMD_SVF,    8, ncsi_cmd_handler_svf     },
> >         { NCSI_PKT_CMD_EV,     4, ncsi_cmd_handler_ev      },
> >         { NCSI_PKT_CMD_DV,     0, ncsi_cmd_handler_default },
> >         { NCSI_PKT_CMD_SMA,    8, ncsi_cmd_handler_sma     },
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 3ea49ed0a935..91b4b66438df 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -104,7 +104,7 @@ struct ncsi_cmd_svf_pkt {
> >         unsigned char           index;     /* VLAN table index  */
> >         unsigned char           enable;    /* Enable or disable */
> >         __be32                  checksum;  /* Checksum          */
> > -       unsigned char           pad[14];
> > +       unsigned char           pad[18];
> >  };
> > 
> >  /* Enable VLAN */
> > --
> > 2.13.3
> >