[ofw] mlx4 patch - supporting multiple devices

[Tzachi Dar]

Thanks Sean for reading my patch and commenting.

As for the first issue: This is  definitely a bug that I'll fix (props->max_qp = ...)

As for the rest of the issues:
1) Using constructs from mlx4 on  the Windows code base ( __be data types for example).
2) defines in the middle of functions and structures are a little odd to me.
3) Passing parameters that are currently not in used.

These issues are mainly issues of the style of the code.

I must say that this is not exactly the way I wanted to see that as well.

In any case, the main reason that we have chosen to write things this way is to allow our driver to be as close to the Linux version of mlx4 as possibale. This allows us to take patches with minimum effort.
As a result, we will not be able to change the coding style of the mlx4 driver.

Thanks
Tzachi

> -----Original Message-----
> From: Sean Hefty [mailto:sean.hefty at intel.com] 
> Sent: Thursday, June 12, 2008 12:35 AM
> To: Tzachi Dar; ofw at lists.openfabrics.org
> Subject: RE: [ofw] mlx4 patch - supporting multiple devices
> 
> --- mlx4/kernel/bus/ib/main.c (revision 1261)
> +++ mlx4/kernel/bus/ib/main.c (working copy)
> @@ -91,7 +91,7 @@
>   props->max_mr_size    = ~0ull;
>   props->page_size_cap    = dev->dev->caps.page_size_cap;
> - props->max_qp     = dev->dev->caps.num_qps - 
> dev->dev->caps.reserved_qps;
> +//?????????????????????? props->max_qp     = dev->dev->caps.num_qps -
> dev->dev->caps.reserved_qps;
>   props->max_qp_wr    = dev->dev->caps.max_wqes - 
> MLX4_IB_SQ_MAX_SPARE;
> 
> 
> Please clean this up before committing.
> 
>  
>  EXPORT_SYMBOL_GPL(mlx4_buf_free);
> 
> Are you saying that this function is indeed GPL?
> 
> 
> 
> +EXPORT_SYMBOL_GPL(mlx4_alloc_hwq_res);
> 
> Same here.  (Besides that this doesn't mean anything on Windows.)
> 
> 
> 
> +EXPORT_SYMBOL_GPL(mlx4_free_hwq_res);
> 
> And here, and several other places.  It would be nice to 
> avoid adding yet more Linux constructs to the Windows code 
> base (list_head, __be data types, etc.).
> It just makes understanding the Windows code that much more difficult.
> 
> 
> 
>  int mlx4_cq_alloc(struct mlx4_dev *dev, int nent, struct 
> mlx4_mtt *mtt,
> -    struct mlx4_uar *uar, u64 db_rec, struct mlx4_cq *cq)
> +    struct mlx4_uar *uar, u64 db_rec, struct mlx4_cq *cq,
> +    unsigned vector, int collapsed)
>  {
>   struct mlx4_priv *priv = mlx4_priv(dev);
>   struct mlx4_cq_table *cq_table = &priv->cq_table; @@ -126,6 
> +127,10 @@
>  
>   u64 mtt_addr;
>   int err;
>  
> + UNREFERENCED_PARAMETER(vector);
> 
> Why pass the parameter in then?  Just remove it from the function.
> 
> 
> 
> +#define COLLAPSED_SHIFT 18
> +#define ENTRIES_SHIFT 24
> 
> #defines in the middle of functions and structures are a 
> little odd to me. 
> 
> 
> +
>   cq->cqn = mlx4_bitmap_alloc(&cq_table->bitmap);
>   if (cq->cqn == -1)
>    return -ENOMEM;
> @@ -153,7 +158,9 @@
>  
>   cq_context = (struct mlx4_cq_context *)mailbox->buf;
>   memset(cq_context, 0, sizeof *cq_context);
>  
> - cq_context->logsize_usrpage = cpu_to_be32((ilog2(nent) << 
> 24) | uar->index);
> + cq_context->flags = cpu_to_be32(!!collapsed << COLLAPSED_SHIFT);
> + cq_context->logsize_usrpage = cpu_to_be32(
> +      (ilog2(nent) << ENTRIES_SHIFT) | uar->index);
>   cq_context->comp_eqn        = 
> (u8)priv->eq_table.eq[MLX4_EQ_COMP].eqn;
>   cq_context->log_page_size   = (u8)(mtt->page_shift - 
> MLX4_ICM_PAGE_SHIFT);
>  
> Index: mlx4/kernel/bus/net/fw.c
>  
> ===================================================================
>  
> --- mlx4/kernel/bus/net/fw.c (revision 1261)
>  
> +++ mlx4/kernel/bus/net/fw.c (working copy)
>  
> @@ -308,7 +308,7 @@
>  
>     MLX4_GET(field, outbox, QUERY_DEV_CAP_VL_PORT_OFFSET);
>     dev_cap->max_vl[i]    = field >> 4;
>     MLX4_GET(field, outbox, QUERY_DEV_CAP_MTU_WIDTH_OFFSET);
> -   dev_cap->max_mtu[i]    = field >> 4;
> +   dev_cap->ib_mtu[i]    = field >> 4;
>     dev_cap->max_port_width[i] = field & 0xf;
>     MLX4_GET(field, outbox, QUERY_DEV_CAP_MAX_GID_OFFSET);
>     dev_cap->max_gids[i]    = 1 << (field & 0xf); @@ -316,9 +316,11 @@
>  
>     dev_cap->max_pkeys[i]    = 1 << (field & 0xf);
>    }
>   } else {
> +#define QUERY_PORT_SUPPORTED_TYPE_OFFSET 0x00
>  #define QUERY_PORT_MTU_OFFSET   0x01
>  #define QUERY_PORT_WIDTH_OFFSET   0x06
>  #define QUERY_PORT_MAX_GID_PKEY_OFFSET  0x07
> +#define QUERY_PORT_MAX_MACVLAN_OFFSET  0x0a
>  #define QUERY_PORT_MAX_VL_OFFSET  0x0b
> 
> 
> And these?  It makes the resulting code difficult to read.  
> There are a lot of #defines in the code that appear to only 
> be used in 1 place.  Why not just use the value and add a 
> comment if that's the case?  This avoids any chance of 
> duplicate #define names.
> 
>