5717 support commit is buggy

5717 support commit is buggy

[David Miller]

The change:

commit f6eb9b1fc1411d22c073f5264e5630a541d0f7df
Author: Matt Carlson <mcarlson@broadcom.com>
Date:   Tue Sep 1 13:19:53 2009 +0000

    tg3: Add 5717 asic rev
    
    This patch adds the 5717 asic rev.
    
    Signed-off-by: Matt Carlson <mcarlson@broadcom.com>
    Reviewed-by: Benjamin Li <benli@broadcom.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

breaks 5703 chips on my workstation.

I suspect it breaks a lot of other chips too.

I'm about to do some tests, but I suspect it's this change:

@@ -111,7 +111,8 @@
  * replace things like '% foo' with '& (foo - 1)'.
  */
 #define TG3_RX_RCB_RING_SIZE(tp)	\
-	((tp->tg3_flags2 & TG3_FLG2_5705_PLUS) ?  512 : 1024)
+	(((tp->tg3_flags & TG3_FLAG_JUMBO_CAPABLE) && \
+	  !(tp->tg3_flags2 & TG3_FLG2_5780_CLASS)) ? 512 : 1024)
 
 #define TG3_TX_RING_SIZE		512
 #define TG3_DEF_TX_RING_PENDING		(TG3_TX_RING_SIZE - 1)

and thus an incorrect RCB ring size is being used which eventually
locks up the card.

Also:

@@ -13486,7 +13556,8 @@ static void __devinit tg3_init_link_config(struct tg3 *tp)
 
 static void __devinit tg3_init_bufmgr_config(struct tg3 *tp)
 {
-	if (tp->tg3_flags2 & TG3_FLG2_5705_PLUS) {
+	if (tp->tg3_flags2 & TG3_FLG2_5705_PLUS &&
+	    GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5717) {
 		tp->bufmgr_config.mbuf_read_dma_low_water =
 			DEFAULT_MB_RDMA_LOW_WATER_5705;
 		tp->bufmgr_config.mbuf_mac_rx_low_water =

I wonder what that does with C precedence rules.  Probably need
parenhesis around "tp->tg3_flags2 & TG3_FLG2_5705_PLUS" for
safety.

Re: 5717 support commit is buggy

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Fri, 11 Sep 2009 15:15:54 -0700 (PDT)

> I'm about to do some tests, but I suspect it's this change:
> 
> @@ -111,7 +111,8 @@
>   * replace things like '% foo' with '& (foo - 1)'.
>   */
>  #define TG3_RX_RCB_RING_SIZE(tp)	\
> -	((tp->tg3_flags2 & TG3_FLG2_5705_PLUS) ?  512 : 1024)
> +	(((tp->tg3_flags & TG3_FLAG_JUMBO_CAPABLE) && \
> +	  !(tp->tg3_flags2 & TG3_FLG2_5780_CLASS)) ? 512 : 1024)
>  
>  #define TG3_TX_RING_SIZE		512
>  #define TG3_DEF_TX_RING_PENDING		(TG3_TX_RING_SIZE - 1)
> 
> and thus an incorrect RCB ring size is being used which eventually
> locks up the card.

Confirmed, reverting that part of the change makes my card
work again.

Broadcom folks, that is the correct test?  This needs to be fixed
urgently.

Re: 5717 support commit is buggy

[Matt Carlson]

On Fri, Sep 11, 2009 at 03:41:25PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Fri, 11 Sep 2009 15:15:54 -0700 (PDT)
> 
> > I'm about to do some tests, but I suspect it's this change:
> > 
> > @@ -111,7 +111,8 @@
> >   * replace things like '% foo' with '& (foo - 1)'.
> >   */
> >  #define TG3_RX_RCB_RING_SIZE(tp)	\
> > -	((tp->tg3_flags2 & TG3_FLG2_5705_PLUS) ?  512 : 1024)
> > +	(((tp->tg3_flags & TG3_FLAG_JUMBO_CAPABLE) && \
> > +	  !(tp->tg3_flags2 & TG3_FLG2_5780_CLASS)) ? 512 : 1024)
> >  
> >  #define TG3_TX_RING_SIZE		512
> >  #define TG3_DEF_TX_RING_PENDING		(TG3_TX_RING_SIZE - 1)
> > 
> > and thus an incorrect RCB ring size is being used which eventually
> > locks up the card.
> 
> Confirmed, reverting that part of the change makes my card
> work again.
> 
> Broadcom folks, that is the correct test?  This needs to be fixed
> urgently.

The test is correct, but the results are transposed.  The 512 and 1024
should be reversed.  I'll cook up a patch.

Re: 5717 support commit is buggy

[Matt Carlson]

On Fri, Sep 11, 2009 at 03:15:54PM -0700, David Miller wrote:
> Also:
> 
> @@ -13486,7 +13556,8 @@ static void __devinit tg3_init_link_config(struct tg3 *tp)
>  
>  static void __devinit tg3_init_bufmgr_config(struct tg3 *tp)
>  {
> -	if (tp->tg3_flags2 & TG3_FLG2_5705_PLUS) {
> +	if (tp->tg3_flags2 & TG3_FLG2_5705_PLUS &&
> +	    GET_ASIC_REV(tp->pci_chip_rev_id) != ASIC_REV_5717) {
>  		tp->bufmgr_config.mbuf_read_dma_low_water =
>  			DEFAULT_MB_RDMA_LOW_WATER_5705;
>  		tp->bufmgr_config.mbuf_mac_rx_low_water =
> 
> I wonder what that does with C precedence rules.  Probably need
> parenhesis around "tp->tg3_flags2 & TG3_FLG2_5705_PLUS" for
> safety.

Harbison & Steele says '&' takes precedence over '&&'.  That said, I'm
not shy about adding parens to remove any doubt.

I'm brewing another patchset that contains a patch to address things
like this.  I'll add this to that patch.

Re: 5717 support commit is buggy

[David Miller]

From: "Matt Carlson" <mcarlson@broadcom.com>
Date: Fri, 11 Sep 2009 16:33:52 -0700

> I'm brewing another patchset that contains a patch to address things
> like this.  I'll add this to that patch.

The merge window has just opened, therefore please don't send me any
more feature patches at this time.  Such things will have to wait
for 2.6.33