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: Wed, 04 Nov 2009 10:04:59 -0800 From: Roland Dreier <rdreier@...co.com> To: Yevgeny Petrilin <yevgenyp@...lanox.co.il> Cc: linux-rdma@...r.kernel.org, netdev@...r.kernel.org, liranl@...lanox.co.il, tziporet@...lanox.co.il Subject: Re: [PATCH 03/25] mlx4_core: add multi-function communication channel > --- 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? > + __be32 status = readl(&priv->mfunc.comm->slave_read); This can't be endian-clean, can it? What does sparse with -D__CHECK_ENDIAN__ say? > + 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? 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. > + /* 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, ...? > +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 }, ... > +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? - 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