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: Thu, 5 Nov 2009 15:06:58 +0200 From: "Liran Liss" <liranl@...lanox.co.il> To: "Roland Dreier" <rdreier@...co.com>, "Yevgeny Petrilin" <yevgenyp@...lanox.co.il> Cc: <linux-rdma@...r.kernel.org>, <netdev@...r.kernel.org>, "Tziporet Koren" <tziporet@...lanox.co.il> Subject: RE: [PATCH 03/25] mlx4_core: add multi-function communicationchannel S.B. --Liran > --- a/drivers/net/mlx4/cmd.c > +++ b/drivers/net/mlx4/cmd.c > @@ -41,6 +41,7 @@ > #include <asm/io.h> > > #include "mlx4.h" > +#include "en_port.h" Why does core mlx4 command handling end up depending on stuff from en_port.h? LL: some of the FW commands (e.g., MLX4_CMD_SET_VLAN_FLTR) are defined here and are used in cmd.h. We will move these definitions to cmd.h. > + __be32 status = readl(&priv->mfunc.comm->slave_read); This can't be endian-clean, can it? What does sparse with -D__CHECK_ENDIAN__ say? LL: will fix. > + queue_delayed_work(priv->mfunc.comm_wq, &priv->mfunc.comm_work, > + polled ? HZ / 1000 : HZ / 10); So this is always running at least 10 times a second? That's a lot of wakeups on an idle system. Is there no way to make this event-driven? LL: events are not implemented yet - this is the next step. And HZ/1000 is going to be 0 if HZ is less than 1000 ... so this is just going to run continuously in the polling case. LL: This is what we want as long as there are more pending commands. > + /* Write command */ > + if (cmd == MLX4_COMM_CMD_RESET) > + priv->cmd.comm_toggle = 0; > + else if (++priv->cmd.comm_toggle > 2) > + priv->cmd.comm_toggle = 1; Is this right? comm_toggle goes 0, 1, 2, 1, 2, ...? LL: Yes - we need to distinguish the reset state from all other states to support asych reset (e.g., FLR). The only way to continue from this state is a new boot sequence. > +static struct mlx4_cmd_info { > + u8 opcode; > + u8 has_inbox; > + u8 has_outbox; > + u8 out_is_imm; > + int (*verify)(struct mlx4_dev *dev, int slave, struct mlx4_vhcr *vhcr, > + struct mlx4_cmd_mailbox *inbox); > + int (*wrapper)(struct mlx4_dev *dev, int slave, struct mlx4_vhcr *vhcr, > + struct mlx4_cmd_mailbox *inbox, > + struct mlx4_cmd_mailbox *outbox); > +} cmd_info[] = { > + {MLX4_CMD_QUERY_FW, 0, 1, 0, NULL, NULL}, > + {MLX4_CMD_QUERY_ADAPTER, 0, 1, 0, NULL, NULL}, This big structure would be better with designated initializers. Also instead of u8 for the flags bool would be better probably. Then it becomes more self documenting, ie { .opcode = MLX4_CMD_QUERY_FW, .has_outbox = true }, ... LL: OK. 10x. > +struct mlx4_vhcr { > + u64 in_param; > + u64 out_param; > + u32 in_modifier; > + u32 timeout; > + u16 op; > + u16 token; > + u8 op_modifier; > + int errno; > +}; trivial but can you use tabs to line up the structure field names the way the rest of the mlx4 declarations do? LL: Ack. - R. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists