[openib-general] [PATCH 3/6] [RFC] iser initiator

[Christoph Hellwig]

> +/* Constant PDU lengths calculations */
> +#define ISER_HDR_LEN            sizeof (struct iser_hdr)
> +#define ISER_PDU_BHS_LENGTH     sizeof (struct iscsi_hdr)

these two macros are just use in ISER_TOTAL_HEADERS_LEN below,
just kill them.

> +#define USE_OFFSET(offset)      (offset)
> +#define USE_NO_OFFSET           0
> +#define USE_SIZE(size)          (size)
> +#define USE_ENTIRE_SIZE         0

please kill these macros.

> +/* iser_dto_add_regd_buff - increments the reference count for *
> + * the registered buffer & adds it to the DTO object           */
> +static void iser_dto_add_regd_buff(struct iser_dto *p_dto,
> +				   struct iser_regd_buf *p_regd_buf,
> +				   unsigned long use_offset,
> +				   unsigned long use_size)
> +{
> +	int add_idx;
> +
> +	atomic_inc(&p_regd_buf->ref_count);

Please kill the p_ prefix for pointer types all over the code.

> +static int iser_dma_map_task_data(struct iscsi_iser_cmd_task *p_iser_task,
> +				  struct iser_data_buf       *p_data,
> +				  enum   iser_data_dir       iser_dir,
> +				  enum   dma_data_direction  dma_dir)
> +{
> +	struct device *dma_device;
> +	dma_addr_t     dma_addr;
> +	int            dma_nents;
> +
> +	p_iser_task->dir[iser_dir] = 1;
> +	dma_device = p_iser_task->conn->ib_conn->p_adaptor->device->dma_device;
> +
> +	if (p_data->type == ISER_BUF_TYPE_SINGLE) {
> +		p_iser_task->data_len[iser_dir] = p_data->size;
> +		dma_addr = dma_map_single(dma_device,p_data->p_buf, p_data->size,
> +					  dma_dir);
> +		if (dma_mapping_error(dma_addr)) {
> +			iser_err("dma_map_single failed at %p\n", p_data->p_buf);
> +			return -EINVAL;
> +		}
> +		p_data->dma_addr = dma_addr;
> +	} else {

I'd say kill the non-SG case.  We're in the progress of removing non-SG
commands in the scsi midlayer, and I'm pretty sure they won't exist
anymore before the iser code merged.

> +static int iser_post_receive_control(struct iscsi_iser_conn *p_iser_conn)
> +{
> +	struct iser_desc     *rx_desc;
> +	struct iser_regd_buf *p_regd_hdr;
> +	struct iser_regd_buf *p_regd_data;
> +	struct iser_dto      *p_recv_dto = NULL;
> +	struct iser_adaptor  *p_iser_adaptor = p_iser_conn->ib_conn->p_adaptor;
> +	int rx_data_size, err = 0;
> +
> +	rx_desc = kmem_cache_alloc(ig.desc_cache,
> +				      GFP_KERNEL | __GFP_NOFAIL);

__GFP_NOFAIL doesn't work for slab (kmem_cache_alloc/kmalloc/kzalloc/kcalloc)
allocations

> +send_data_out_error:
> +	if (p_send_dto != NULL)
> +		iser_dto_buffs_release(p_send_dto);
> +	if (tx_desc != NULL)
> +		kmem_cache_free(ig.desc_cache, tx_desc);

could you please do the same goto-unwinding style we use elsewhere
in the kernel?  That is one label before each unwind step and jump
directly to that instead of adding tons of conditionals in the error path.