[PATCH net-next] tg3: Fix advertisement handling

[PATCH net-next] tg3: Fix advertisement handling

[Hiroaki SHIMODA]

Commit 28011cf19b (net: Add ethtool to mii advertisment conversion
helpers) added a helper function ethtool_adv_to_mii_100bt() and
tg3_copper_is_advertising_all(), tg3_phy_autoneg_cfg() were
modified to use this.
Before that commit, ethtool to mii advertisement conversion was
done wrt speed, but now pause operation is also taken account.
So, in tg3_copper_is_advertising_all(), below condition becomes
true and this makes link up fails.

	if ((adv_reg & ADVERTISE_ALL) != all_mask)
		return 0;

To fix this add ADVERTISE_ALL bit and operation to cap speed.

Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
---
 drivers/net/ethernet/broadcom/tg3.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 024ca1d..c00e648 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -3594,7 +3594,7 @@ static int tg3_phy_autoneg_cfg(struct tg3 *tp, u32 advertise, u32 flowctrl)
 	u32 val, new_adv;
 
 	new_adv = ADVERTISE_CSMA;
-	new_adv |= ethtool_adv_to_mii_100bt(advertise);
+	new_adv |= ethtool_adv_to_mii_100bt(advertise) & ADVERTISE_ALL;
 	new_adv |= tg3_advert_flowctrl_1000T(flowctrl);
 
 	err = tg3_writephy(tp, MII_ADVERTISE, new_adv);
@@ -3783,7 +3783,7 @@ static int tg3_copper_is_advertising_all(struct tg3 *tp, u32 mask)
 	if (tg3_readphy(tp, MII_ADVERTISE, &adv_reg))
 		return 0;
 
-	if ((adv_reg & ADVERTISE_ALL) != all_mask)
+	if ((adv_reg & ADVERTISE_ALL) != (all_mask & ADVERTISE_ALL))
 		return 0;
 
 	if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY)) {

Re: [PATCH net-next] tg3: Fix advertisement handling

[Eric Dumazet]

Le lundi 21 novembre 2011 à 08:07 +0900, Hiroaki SHIMODA a écrit :
> Commit 28011cf19b (net: Add ethtool to mii advertisment conversion
> helpers) added a helper function ethtool_adv_to_mii_100bt() and
> tg3_copper_is_advertising_all(), tg3_phy_autoneg_cfg() were
> modified to use this.
> Before that commit, ethtool to mii advertisement conversion was
> done wrt speed, but now pause operation is also taken account.
> So, in tg3_copper_is_advertising_all(), below condition becomes
> true and this makes link up fails.
> 
> 	if ((adv_reg & ADVERTISE_ALL) != all_mask)
> 		return 0;
> 
> To fix this add ADVERTISE_ALL bit and operation to cap speed.
> 
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>

Reported-and-tested-by: Eric Dumazet <eric.dumazet@gmail.com>


Thanks a lot !

Re: [PATCH net-next] tg3: Fix advertisement handling

[Matt Carlson]

On Sun, Nov 20, 2011 at 03:07:20PM -0800, Hiroaki SHIMODA wrote:
> Commit 28011cf19b (net: Add ethtool to mii advertisment conversion
> helpers) added a helper function ethtool_adv_to_mii_100bt() and
> tg3_copper_is_advertising_all(), tg3_phy_autoneg_cfg() were
> modified to use this.
> Before that commit, ethtool to mii advertisement conversion was
> done wrt speed, but now pause operation is also taken account.
> So, in tg3_copper_is_advertising_all(), below condition becomes
> true and this makes link up fails.
> 
> 	if ((adv_reg & ADVERTISE_ALL) != all_mask)
> 		return 0;
> 
> To fix this add ADVERTISE_ALL bit and operation to cap speed.
> 
> Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>

The root cause of this problem can actually be fixed by the following
patch:

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 024ca1d..53e501c 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -13192,8 +13192,7 @@ static u32 __devinit tg3_read_otp_phycfg(struct tg3 *tp)
 
 static void __devinit tg3_phy_init_link_config(struct tg3 *tp)
 {
-	u32 adv = ADVERTISED_Autoneg |
-		  ADVERTISED_Pause;
+	u32 adv = ADVERTISED_Autoneg;
 
 	if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY))
 		adv |= ADVERTISED_1000baseT_Half |

Could you add it as part of this patch?

Technically the above patch should fix the problem, but this patch makes
the code clearer, should the same type of error pop up in the future.

More comments below.

> ---
>  drivers/net/ethernet/broadcom/tg3.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 024ca1d..c00e648 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -3594,7 +3594,7 @@ static int tg3_phy_autoneg_cfg(struct tg3 *tp, u32 advertise, u32 flowctrl)
>  	u32 val, new_adv;
>  
>  	new_adv = ADVERTISE_CSMA;
> -	new_adv |= ethtool_adv_to_mii_100bt(advertise);
> +	new_adv |= ethtool_adv_to_mii_100bt(advertise) & ADVERTISE_ALL;
>  	new_adv |= tg3_advert_flowctrl_1000T(flowctrl);
>  
>  	err = tg3_writephy(tp, MII_ADVERTISE, new_adv);
> @@ -3783,7 +3783,7 @@ static int tg3_copper_is_advertising_all(struct tg3 *tp, u32 mask)
>  	if (tg3_readphy(tp, MII_ADVERTISE, &adv_reg))
>  		return 0;
>  
> -	if ((adv_reg & ADVERTISE_ALL) != all_mask)
> +	if ((adv_reg & ADVERTISE_ALL) != (all_mask & ADVERTISE_ALL))

Rather than ANDing the all_mask here, can you make it look like the 1st
hunk of your patch by ANDing the value returned by
ethtool_adv_to_mii_100bt()?  I plan on further changes in this area and
the uniformity will help.

>  		return 0;
>  
>  	if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY)) {
> 

Re: [PATCH net-next] tg3: Fix advertisement handling

[Hiroaki SHIMODA]

On Mon, 21 Nov 2011 11:29:08 -0800
"Matt Carlson" <mcarlson@broadcom.com> wrote:

> On Sun, Nov 20, 2011 at 03:07:20PM -0800, Hiroaki SHIMODA wrote:
> > Commit 28011cf19b (net: Add ethtool to mii advertisment conversion
> > helpers) added a helper function ethtool_adv_to_mii_100bt() and
> > tg3_copper_is_advertising_all(), tg3_phy_autoneg_cfg() were
> > modified to use this.
> > Before that commit, ethtool to mii advertisement conversion was
> > done wrt speed, but now pause operation is also taken account.
> > So, in tg3_copper_is_advertising_all(), below condition becomes
> > true and this makes link up fails.
> > 
> > 	if ((adv_reg & ADVERTISE_ALL) != all_mask)
> > 		return 0;
> > 
> > To fix this add ADVERTISE_ALL bit and operation to cap speed.
> > 
> > Signed-off-by: Hiroaki SHIMODA <shimoda.hiroaki@gmail.com>
> 
> The root cause of this problem can actually be fixed by the following
> patch:
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 024ca1d..53e501c 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -13192,8 +13192,7 @@ static u32 __devinit tg3_read_otp_phycfg(struct tg3 *tp)
>  
>  static void __devinit tg3_phy_init_link_config(struct tg3 *tp)
>  {
> -	u32 adv = ADVERTISED_Autoneg |
> -		  ADVERTISED_Pause;
> +	u32 adv = ADVERTISED_Autoneg;
>  
>  	if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY))
>  		adv |= ADVERTISED_1000baseT_Half |
> 
> Could you add it as part of this patch?

Ok.

> 
> Technically the above patch should fix the problem, but this patch makes
> the code clearer, should the same type of error pop up in the future.
> 
> More comments below.
> 
> > ---
> >  drivers/net/ethernet/broadcom/tg3.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> > index 024ca1d..c00e648 100644
> > --- a/drivers/net/ethernet/broadcom/tg3.c
> > +++ b/drivers/net/ethernet/broadcom/tg3.c
> > @@ -3594,7 +3594,7 @@ static int tg3_phy_autoneg_cfg(struct tg3 *tp, u32 advertise, u32 flowctrl)
> >  	u32 val, new_adv;
> >  
> >  	new_adv = ADVERTISE_CSMA;
> > -	new_adv |= ethtool_adv_to_mii_100bt(advertise);
> > +	new_adv |= ethtool_adv_to_mii_100bt(advertise) & ADVERTISE_ALL;
> >  	new_adv |= tg3_advert_flowctrl_1000T(flowctrl);
> >  
> >  	err = tg3_writephy(tp, MII_ADVERTISE, new_adv);
> > @@ -3783,7 +3783,7 @@ static int tg3_copper_is_advertising_all(struct tg3 *tp, u32 mask)
> >  	if (tg3_readphy(tp, MII_ADVERTISE, &adv_reg))
> >  		return 0;
> >  
> > -	if ((adv_reg & ADVERTISE_ALL) != all_mask)
> > +	if ((adv_reg & ADVERTISE_ALL) != (all_mask & ADVERTISE_ALL))
> 
> Rather than ANDing the all_mask here, can you make it look like the 1st
> hunk of your patch by ANDing the value returned by
> ethtool_adv_to_mii_100bt()?  I plan on further changes in this area and
> the uniformity will help.

Ok.

I'll send V2 with Eric's Reported-by tag.

Thanks.

> 
> >  		return 0;
> >  
> >  	if (!(tp->phy_flags & TG3_PHYFLG_10_100_ONLY)) {
> > 
>