Re: [PATCH net-next v4] net: marvell: prestera: add firmware v4.0 support

[Volodymyr Mytnyk [C]]

> > +struct prestera_msg_event_port_param {
> > +     union {
> > +             struct {
> > +                     u8 oper;
> > +                     __le32 mode;
> > +                     __le32 speed;
> > +                     u8 duplex;
> > +                     u8 fc;
> > +                     u8 fec;
> 
> Makes more sense to put the 2 le32 first and then the 4 u8s. At the
> moment, the le32 are not word aligned, so the compiler has to issue
> more instructions.
> 
> 
> > +             } __packed mac;
> > +             struct {
> > +                     u8 mdix;
> > +                     __le64 lmode_bmap;
> > +                     u8 fc;
> 
> Same here, le64 first, then to two u8.
> 
> >  union prestera_msg_port_param {
> > -     u8  admin_state;
> > -     u8  oper_state;
> > -     u32 mtu;
> > -     u8  mac[ETH_ALEN];
> > -     u8  accept_frm_type;
> > -     u32 speed;
> > +     u8 admin_state;
> > +     u8 oper_state;
> > +     __le32 mtu;
> 
> 2 u8 followed by a le32? Swap them.
> 
> > +     u8 mac[ETH_ALEN];
> 
> You then get the 6 byte MAC and the 2 u8 giving you word alignment.
> 
> > +     u8 accept_frm_type;
> > +     __le32 speed;
> 
> Swap these two, keeping speed word aligned.
> 
> >        u8 learning;
> >        u8 flood;
> 
> You have 3 u8 in a row, so move another u8 up to keep word alignment, say type.
> 
> > -     u32 link_mode;
> > -     u8  type;
> > -     u8  duplex;
> > -     u8  fec;
> > -     u8  fc;
> > -     struct prestera_msg_port_mdix_param mdix;
> > -     struct prestera_msg_port_autoneg_param autoneg;
> > +     __le32 link_mode;
> > +     u8 type;
> > +     u8 duplex;
> > +     u8 fec;
> > +     u8 fc;
> > +     union {
> 
> With type moved up, this whole union becomes unaligned. So you might
> want to explicitly add a reserved byte here. Make sure it is set to
> zero when sent to the firmware, and ignored on receive.
> 
> > +             struct {
> > +                     u8 admin:1;
> > +                     u8 fc;
> > +                     u8 ap_enable;
> 
> Move these three after the next union, to keep the union aligned.
> 
> > +                     union {
> > +                             struct {
> > +                                     __le32 mode;
> > +                                     u8  inband:1;
> > +                                     __le32 speed;
> 
> speed should be second, so it is aligned.
> 
> > +                                     u8  duplex;
> > +                                     u8  fec;
> > +                                     u8  fec_supp;
> > +                             } __packed reg_mode;
> > +                             struct {
> > +                                     __le32 mode;
> > +                                     __le32 speed;
> > +                                     u8  fec;
> > +                                     u8  fec_supp;
> > +                             } __packed ap_modes[PRESTERA_AP_PORT_MAX];
> > +                     } __packed;
> > +             } __packed mac;
> > +             struct {
> > +                     u8 admin:1;
> > +                     u8 adv_enable;
> > +                     __le64 modes;
> > +                     __le32 mode;
> 
> These two le64 come first to keep them aligned.
> 
> > +                     u8 mdix;
> > +             } __packed phy;
> > +     } __packed link;
> 
> 
> Hopefully you get the idea. Getting alignment correct will produce
> smaller faster code, especially on architectures where none aligned
> accesses are expensive.

  Thanks for the comments. Those structures are packed, so there will not
be any holes (pahole is my friend now :) ). Also, the structure have been
left intentionally unmodified (with few others like prestera_msg_fdb_req,
prestera_msg_event_port_param), to be align with the FW side. The rest structs,
which aren't FW dependent, have been fixed (probably I should have mentioned
this in the patch-set log to make it clear). Will try to fix those also
to make it consistent in all places. I think this is the right time for
such changes.

> 
> > @@ -475,15 +543,15 @@ static int __prestera_cmd_ret(struct prestera_switch *sw,
> >        struct prestera_device *dev = sw->dev;
> >        int err;
> >  
> > -     cmd->type = type;
> > +     cmd->type = __cpu_to_le32(type);
> >  
> > -     err = dev->send_req(dev, cmd, clen, ret, rlen, waitms);
> > +     err = dev->send_req(dev, 0, cmd, clen, ret, rlen, waitms);
> >        if (err)
> >                return err;
> >  
> > -     if (ret->cmd.type != PRESTERA_CMD_TYPE_ACK)
> > +     if (__le32_to_cpu(ret->cmd.type) != PRESTERA_CMD_TYPE_ACK)
> >                return -EBADE;
> 
> Makes more sense to apply the endianness swap to
> PRESTERA_CMD_TYPE_ACK. That can be done at compile time, where as
> swapping ret->cmd.type has to be done a runtime.
>
 
Agree.

> 
> > -     if (ret->status != PRESTERA_CMD_ACK_OK)
> > +     if (__le32_to_cpu(ret->status) != PRESTERA_CMD_ACK_OK)
> >                return -EINVAL;
> 
> So this patch is now doing two different things at once. It is fixing
> your broken endianness support, and it is changing the ABI. Please
> separate these into different patches. Fix the endianness first, then
> change the ABI. That will make it much easier to review.

Makes sense, will do.

> 
>        Andrew
>

    Volodymyr