On 03/15/2016 11:54 AM, Christoph Lameter wrote:
> Index: linux/drivers/infiniband/hw/cxgb3/iwch_provider.c
> ===================================================================
> --- linux.orig/drivers/infiniband/hw/cxgb3/iwch_provider.c
> +++ linux/drivers/infiniband/hw/cxgb3/iwch_provider.c
> @@ -1218,12 +1218,46 @@ static ssize_t show_board(struct device
>  		       iwch_dev->rdev.rnic_info.pdev->device);
>  }
>  
> +char *names[] = {
> +	"ipInReceives",
> +	"ipInHdrErrors",
> +	"ipInAddrErrors",
> +	"ipInUnknownProtos",
> +	"ipInDiscards",
> +	"ipInDelivers",
> +	"ipOutRequests",
> +	"ipOutDiscards",
> +	"ipOutNoRoutes",
> +	"ipReasmTimeout",
> +	"ipReasmReqds",
> +	"ipReasmOKs",
> +	"ipReasmFails",
> +	"tcpActiveOpens",
> +	"tcpPassiveOpens",
> +	"tcpAttemptFails",
> +	"tcpEstabResets",
> +	"tcpCurrEstab",
> +	"tcpInSegs",
> +	"tcpOutSegs",
> +	"tcpRetransSegs",
> +	"tcpInErrs",
> +	"tcpOutRsts",
> +	"tcpRtoMin",
> +	"tcpRtoMax",
> +	NULL
> +};
> +
>  static int iwch_get_mib(struct ib_device *ibdev,
> -			union rdma_protocol_stats *stats)
> +			struct rdma_protocol_stats *stats,
> +			u8 port)
>  {
>  	struct iwch_dev *dev;
>  	struct tp_mib_stats m;
>  	int ret;
> +	u64 *p;
> +
> +	if (port != 0)
> +		return 0;
>  
>  	PDBG("%s ibdev %p\n", __func__, ibdev);
>  	dev = to_iwch_dev(ibdev);
> @@ -1231,45 +1265,53 @@ static int iwch_get_mib(struct ib_device
>  	if (ret)
>  		return -ENOSYS;
>  
> -	memset(stats, 0, sizeof *stats);
> -	stats->iw.ipInReceives = ((u64) m.ipInReceive_hi << 32) +
> -				m.ipInReceive_lo;
> -	stats->iw.ipInHdrErrors = ((u64) m.ipInHdrErrors_hi << 32) +
> -				  m.ipInHdrErrors_lo;
> -	stats->iw.ipInAddrErrors = ((u64) m.ipInAddrErrors_hi << 32) +
> -				   m.ipInAddrErrors_lo;
> -	stats->iw.ipInUnknownProtos = ((u64) m.ipInUnknownProtos_hi << 32) +
> -				      m.ipInUnknownProtos_lo;
> -	stats->iw.ipInDiscards = ((u64) m.ipInDiscards_hi << 32) +
> -				 m.ipInDiscards_lo;
> -	stats->iw.ipInDelivers = ((u64) m.ipInDelivers_hi << 32) +
> -				 m.ipInDelivers_lo;
> -	stats->iw.ipOutRequests = ((u64) m.ipOutRequests_hi << 32) +
> -				  m.ipOutRequests_lo;
> -	stats->iw.ipOutDiscards = ((u64) m.ipOutDiscards_hi << 32) +
> -				  m.ipOutDiscards_lo;
> -	stats->iw.ipOutNoRoutes = ((u64) m.ipOutNoRoutes_hi << 32) +
> -				  m.ipOutNoRoutes_lo;
> -	stats->iw.ipReasmTimeout = (u64) m.ipReasmTimeout;
> -	stats->iw.ipReasmReqds = (u64) m.ipReasmReqds;
> -	stats->iw.ipReasmOKs = (u64) m.ipReasmOKs;
> -	stats->iw.ipReasmFails = (u64) m.ipReasmFails;
> -	stats->iw.tcpActiveOpens = (u64) m.tcpActiveOpens;
> -	stats->iw.tcpPassiveOpens = (u64) m.tcpPassiveOpens;
> -	stats->iw.tcpAttemptFails = (u64) m.tcpAttemptFails;
> -	stats->iw.tcpEstabResets = (u64) m.tcpEstabResets;
> -	stats->iw.tcpOutRsts = (u64) m.tcpOutRsts;
> -	stats->iw.tcpCurrEstab = (u64) m.tcpCurrEstab;
> -	stats->iw.tcpInSegs = ((u64) m.tcpInSegs_hi << 32) +
> -			      m.tcpInSegs_lo;
> -	stats->iw.tcpOutSegs = ((u64) m.tcpOutSegs_hi << 32) +
> -			       m.tcpOutSegs_lo;
> -	stats->iw.tcpRetransSegs = ((u64) m.tcpRetransSeg_hi << 32) +
> -				  m.tcpRetransSeg_lo;
> -	stats->iw.tcpInErrs = ((u64) m.tcpInErrs_hi << 32) +
> -			      m.tcpInErrs_lo;
> -	stats->iw.tcpRtoMin = (u64) m.tcpRtoMin;
> -	stats->iw.tcpRtoMax = (u64) m.tcpRtoMax;
> +	stats->dirname = "iw_stats";
> +	stats->name = names;
> +
> +	p = stats->value;
> +	*p++ = ((u64)m.ipInReceive_hi << 32) +
> +		m.ipInReceive_lo;
> +	*p++ = ((u64)m.ipInHdrErrors_hi << 32) +
> +		m.ipInHdrErrors_lo;
> +	*p++ = ((u64)m.ipInAddrErrors_hi << 32) +
> +		m.ipInAddrErrors_lo;
> +	*p++ = ((u64)m.ipInUnknownProtos_hi << 32) +
> +		m.ipInUnknownProtos_lo;
> +	*p++ = ((u64)m.ipInDiscards_hi << 32) +
> +		m.ipInDiscards_lo;
> +	*p++ = ((u64)m.ipInDelivers_hi << 32) +
> +		m.ipInDelivers_lo;
> +	*p++ = ((u64)m.ipOutRequests_hi << 32) +
> +		m.ipOutRequests_lo;
> +	*p++ = ((u64)m.ipOutDiscards_hi << 32) +
> +		m.ipOutDiscards_lo;
> +	*p++ = ((u64)m.ipOutNoRoutes_hi << 32) +
> +		m.ipOutNoRoutes_lo;
> +	*p++ = (u64)m.ipReasmTimeout;
> +	*p++ = (u64)m.ipReasmReqds;
> +	*p++ = (u64)m.ipReasmOKs;
> +	*p++ = (u64)m.ipReasmFails;
> +	*p++ = (u64)m.tcpActiveOpens;
> +	*p++ = (u64)m.tcpPassiveOpens;
> +	*p++ = (u64)m.tcpAttemptFails;
> +	*p++ = (u64)m.tcpEstabResets;
> +	*p++ = (u64)m.tcpOutRsts;
> +	*p++ = (u64)m.tcpCurrEstab;
> +	*p++ = ((u64)m.tcpInSegs_hi << 32) +
> +		m.tcpInSegs_lo;
> +	*p++ = ((u64)m.tcpOutSegs_hi << 32) +
> +		m.tcpOutSegs_lo;
> +	*p++ = ((u64)m.tcpRetransSeg_hi << 32) +
> +		m.tcpRetransSeg_lo;
> +	*p++ = ((u64)m.tcpInErrs_hi << 32) +
> +		m.tcpInErrs_lo;
> +	*p++ = (u64)m.tcpRtoMin;
> +	*p++ = (u64)m.tcpRtoMax;
> +
> +	/* Emsure we provided all values */
> +	BUG_ON(p - stats->value !=
> +	       (sizeof(names) / sizeof(char *)) - 1);
> +
>  	return 0;
>  }
>  

I don't like this entire chunk.  Please handle both the cxgb3 and cxgb4
changes like you did the mlx4 changes.  Specifically, use an enum to
define the array index for names and an array of offsets so that the
textual names from the enum can be used to access the array.  The way
things are here is horribly fragile.

Secondly, do *not* use a BUG_ON in this patch.  I saw at least two of
them.  There is nothing in this patch so serious that we should crash
the kernel.  Any failure here is something we can work around and keep
running.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD