netdev - Re: [PATCH for/net-next 1/8] net/mlx5: Mellanox Connect-IB, core driver part 1/3

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20130628.210329.240918119199058100.davem@davemloft.net>
Date:	Fri, 28 Jun 2013 21:03:29 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	ogerlitz@...lanox.com
Cc:	roland@...nel.org, linux-rdma@...r.kernel.org,
	netdev@...r.kernel.org, eli@....mellanox.co.il,
	moshel@...lanox.com, eli@...lanox.com
Subject: Re: [PATCH for/net-next 1/8] net/mlx5: Mellanox Connect-IB, core
 driver part 1/3

From: Or Gerlitz <ogerlitz@...lanox.com>
Date: Wed, 26 Jun 2013 17:22:10 +0300

> +		buf->direct.buf   = dma_alloc_coherent(&dev->pdev->dev,
> +						       size, &t, GFP_KERNEL);
 ...
> +		memset(buf->direct.buf, 0, size);


This sequence is "dma_zalloc_coherent()", please use it.


> +	i = find_first_bit(pgdir->bitmap, MLX5_DB_PER_PAGE);
> +	if (i >= MLX5_DB_PER_PAGE)
> +		return -ENOMEM;
> +
> +	clear_bit(i, pgdir->bitmap);

You have other synchronizations protecting this bitmap, so you don't
need to use the atomic clear_bit(), please use __clear_bit() instead.

> +
> +	set_bit(db->index, db->u.pgdir->bitmap);

Likewise, use __set_bit().

> +	u8 own;
> +	unsigned long poll_end = jiffies + msecs_to_jiffies(MLX5_CMD_TIMEOUT_MSEC + 1000);

I know it sounds silly, but order local variable declarations from longest
to shortest line, it looks a lot nicer.

> +static int verify_signature(struct mlx5_cmd_work_ent *ent)
> +{
> +	int err;
> +	u8 sig;
> +	struct mlx5_cmd_mailbox *next = ent->out->next;

Likewise.

> +static void dump_buf(void *buf, int size, int data_only, int offset)
> +{
> +	int i;
> +	__be32 *p = buf;

Likewise.  Please go over all of this new driver and take care of this,
thanks.

> +static const char *status_to_str(u8 status)
> +{
> +	switch (status) {
> +	case 0:
> +		return "no errors";

Please add defines for these status codes rather than using magic
constants.

> +	for (i = 0; i < n; ++i) {

"i++", rather than "++i" is canonical in this situations, please.

> +	for (i = 0; i < (1 << cmd->log_sz); ++i) {

Likewise.  Please fix this up in all of this driver's code.

> +			/*
> +			 * for cached lists, we must explicitly state what is
> +			 * the real size
> +			 */

Comments in the networking are to be formatted:

	/* Like
	 * this.
	 */

and not:

	/*
	 * Like
	 * this.
	 */

Please take care of this in the entire driver, thanks.

> +ex_err:
> +	list_for_each_entry_safe(msg, n, &cmd->cache.large.head, list) {
> +		list_del(&msg->list);
> +		mlx5_free_cmd_msg(dev, msg);
> +	}
> +
> +	list_for_each_entry_safe(msg, n, &cmd->cache.med.head, list) {
> +		list_del(&msg->list);
> +		mlx5_free_cmd_msg(dev, msg);
> +	}
> +
> +	return err;

This cleanup code is duplicated...

> +static void destroy_msg_cache(struct mlx5_core_dev *dev)

Here.

Simply declare destroy_msg_cache() earlier and call it, instead
of duplicating all of it's code.

> +static const char *cmd_status_str(u8 status)
> +{
> +	switch (status) {
> +	case 0x0:	return "OK";

Again, make readable defines for these status values, rather than using magic
constants.

> +	switch (hdr->status) {
> +	case 0x0:	return 0;
> +	case 0x1:	return -EIO;

Likewise.

I also question how useful all of these debugfs facilities are after initial
development.  They take up memory even if they are never used.
--
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