[openib-general] HP ZX1 and HP IB cards...

[Grant Grundler]

On Fri, Dec 03, 2004 at 06:25:27PM -0800, Roland Dreier wrote:
>     Roland> Hmm, I guess that wasn't the problem (or I didn't fix it
>     Roland> properly).
> 
> The latter (fix was bogus, I made a cut-and-paste error).  Here's a
> better version:

Yes - that works. Please commit.

I was still trying to sort out how set_ci when I gave up on friday.
You explanation helped though I'm familiar with consumer/producer
(tg3 has a similar construct) indexes.

> Index: infiniband/hw/mthca/mthca_eq.c
> ===================================================================
> --- infiniband/hw/mthca/mthca_eq.c	(revision 1310)
> +++ infiniband/hw/mthca/mthca_eq.c	(working copy)
> @@ -219,11 +219,14 @@
>  	struct mthca_eqe *eqe;
>  	int disarm_cqn;
>  	int work = 0;
> +	int set_ci = 0;
>  
>  	while (1) {
>  		if (!next_eqe_sw(eq))
>  			break;
>  
> +		set_ci = 0;
> +
>  		eqe = get_eqe(eq, eq->cons_index);
>  		work = 1;
>  
> @@ -274,6 +277,13 @@
>  					be16_to_cpu(eqe->event.cmd.token),
>  					eqe->event.cmd.status,
>  					be64_to_cpu(eqe->event.cmd.out_param));
> +			/*
> +			 * Need to set the CI inside the loop for
> +			 * command completion events, because this
> +			 * event allows another command to be posted
> +			 * and we may overflow the EQ.
> +			 */

The comment inside "case MTHCA_EVENT_TYPE_CMD" now makes sense
and it didn't make sense to me on friday...perhaps showing it
was indeed time for a break.

> +			set_ci = 1;
>  			break;
>  
>  		case MTHCA_EVENT_TYPE_PORT_CHANGE:

> @@ -296,9 +306,14 @@
>  
>  		set_eqe_hw(eq, eq->cons_index);
>  		eq->cons_index = (eq->cons_index + 1) & (eq->nent - 1);

This line of code assumes eq->nent is a power of 2.
I didn't see anything in mthca_create_eq() that enforces
the assumption that nent is a power of 2.
It mthca_create_eq() isn't in the perf path, I think it would be good
to enforcement the requirement or at least check for it (e.g. WARN_ON).
I just don't want to make the performance patch longer.

> +
> +		if (set_ci) {
> +			wmb();

this wmb() isn't necessary since set_eq_ci() calls mthca_write64()
and the latter *must* enforce wmb() for the MMIO write.

I'd also like to see doorbell[2] go away and just pass the two u32 values
to mthca_write64().  Perhaps combine them explicitly instead depending on
load/store model of the stack.
While it seems to work, I'm very nervous about this dependency and
wonder if it would even work on parisc-linux (ok, I'm the only who cares :^)
which has an upward growing stack.


> +			set_eq_ci(dev, eq->eqn, eq->cons_index);
> +		}
>  	}
>  
> -	if (work) {
> +	if (work && !set_ci) {
>  		wmb();

ditto.

>  		set_eq_ci(dev, eq->eqn, eq->cons_index);
>  	}

thanks,
grant