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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111121192908.GA1198@mcarlson.broadcom.com>
Date:	Mon, 21 Nov 2011 11:29:08 -0800
From:	"Matt Carlson" <mcarlson@...adcom.com>
To:	"Hiroaki SHIMODA" <shimoda.hiroaki@...il.com>
cc:	"davem@...emloft.net" <davem@...emloft.net>,
	"Matthew Carlson" <mcarlson@...adcom.com>,
	"Michael Chan" <mchan@...adcom.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] tg3: Fix advertisement handling

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@...il.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)) {
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html