netdev - Re: [PATCH 08/15] i40iw: add files for iwarp interface

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] [day] [month] [year] [list]

Message-ID: <20151217162738.GA12875@infradead.org>
Date:	Thu, 17 Dec 2015 08:27:38 -0800
From:	Christoph Hellwig <hch@...radead.org>
To:	Faisal Latif <faisal.latif@...el.com>
Cc:	dledford@...hat.com, linux-rdma@...r.kernel.org,
	netdev@...r.kernel.org, jeffrey.t.kirsher@...el.com,
	e1000-rdma@...ts.sourceforge.net
Subject: Re: [PATCH 08/15] i40iw: add files for iwarp interface

> +		i40iw_next_iw_state(iwqp, I40IW_QP_STATE_ERROR, 0, 0, 0);
> +
> +	if (!iwqp->user_mode) {
> +		if (iwqp->iwscq)
> +			i40iw_clean_cqes(iwqp, iwqp->iwscq);
> +		if ((iwqp->iwrcq) && (iwqp->iwrcq != iwqp->iwscq))

Please try to do a pass over your code and remove all these pointless
braces.

> +static int i40iw_setup_virt_qp(struct i40iw_device *iwdev,
> +			       struct i40iw_qp *iwqp,
> +			       struct i40iw_qp_init_info *init_info)
> +{
> +	struct i40iw_pbl *iwpbl = iwqp->iwpbl;
> +	struct i40iw_qp_mr *qpmr = &iwpbl->qp_mr;
> +	u64 *sq_base;
> +
> +	sq_base = kmap(qpmr->sq_page);
> +	iwqp->sq_kmapped = 1;


You must never use kmap for any long lived resource.  Just allocate
it it out of lowmem so that you don't need the kmap.

> +	ukinfo->rq = (u64 *)((u8 *)mem->va + (sqdepth * I40IW_QP_WQE_MIN_SIZE));
> +	info->rq_pa = (uintptr_t)((u8 *)mem->pa + (sqdepth * I40IW_QP_WQE_MIN_SIZE));
> +
> +	ukinfo->shadow_area = (u64 *)((u8 *)ukinfo->rq +
> +				      (rqdepth * I40IW_QP_WQE_MIN_SIZE));
> +	info->shadow_area_pa = info->rq_pa + (rqdepth * I40IW_QP_WQE_MIN_SIZE);

Can you please try to get away with less casts here?  Note that Linux does
use GCC extensions for void pointer arithmetics.  Even without that you
never need to use casts to or from void pointers.  All this happes in
lots of places in the code, so a little audit would be useful.

> +/**
> + * i40iw_alloc_mw - Allocate memory window
> + * @ibpd: protection domain
> + * @type: memory window type
> + */
> +static struct ib_mw *i40iw_alloc_mw(struct ib_pd *ibpd,
> +				    enum ib_mw_type type)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
> +/**
> + * i40iw_dealloc_mw - Free a memory window
> + * @ibmw: memory window to free
> + */
> +static int i40iw_dealloc_mw(struct ib_mw *ibmw)
> +{
> +	return -EIO;
> +}
> +
> +/**
> + * i40iw_bind_mw - Bind a memory window to a qp
> + * @ibqp: queue pair
> + * @ibmw: memory window
> + * @ibmw_bind: pointer to bind structure
> + */
> +static int i40iw_bind_mw(struct ib_qp *ibqp,
> +			 struct ib_mw *ibmw,
> +			 struct ib_mw_bind *ibmw_bind)
> +{
> +	return -ENOSYS;
> +}

There shouldn't be any need to stub all these out.

> +/**
> + * i40iw_init_ofa_device - initialization of iwarp device
> + * @iwdev: iwarp device
> + */
> +static struct i40iw_ib_device *i40iw_init_ofa_device(struct i40iw_device *iwdev)

Where is that weird ofa prefix coming from?

> +	iwibdev->ibdev.reg_phys_mr = i40iw_reg_phys_mr;

Please don't add phys MR support in new drivers, it's about to
disappear.

> +	iwibdev->ibdev.detach_mcast = NULL;
> +	iwibdev->ibdev.attach_mcast = NULL;
> +	iwibdev->ibdev.get_protocol_stats = i40iw_get_protocol_stats;
> +	iwibdev->ibdev.process_mad = NULL;

All the unused fields should already be zeroed.
--
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