| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 29 Oct 2021 14:45:10 +0000
From: "Volodymyr Mytnyk [C]" <vmytnyk@...vell.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "kuba@...nel.org" <kuba@...nel.org>,
Mickey Rachamim <mickeyr@...vell.com>,
Serhiy Pshyk <serhiy.pshyk@...ision.eu>,
Taras Chornyi <taras.chornyi@...ision.eu>,
"Vadym Kochan [C]" <vkochan@...vell.com>,
Yevhen Orlov <yevhen.orlov@...ision.eu>,
"Taras Chornyi [C]" <tchornyi@...vell.com>,
"David S. Miller" <davem@...emloft.net>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v4] net: marvell: prestera: add firmware v4.0
support
> > +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
Powered by blists - more mailing lists