From: "Matt Carlson" <mcarlson@broadcom•com>
To: "Hiroaki SHIMODA" <shimoda.hiroaki@gmail•com>
Cc: "davem@davemloft•net" <davem@davemloft•net>,
"Matthew Carlson" <mcarlson@broadcom•com>,
"Michael Chan" <mchan@broadcom•com>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>
Subject: Re: [PATCH net-next] tg3: Fix advertisement handling
Date: Mon, 21 Nov 2011 11:29:08 -0800 [thread overview]
Message-ID: <20111121192908.GA1198@mcarlson.broadcom.com> (raw)
In-Reply-To: <20111121080720.0e68f096.shimoda.hiroaki@gmail.com>
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)) {
>
next prev parent reply other threads:[~2011-11-21 19:28 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-20 23:07 [PATCH net-next] tg3: Fix advertisement handling Hiroaki SHIMODA
2011-11-21 6:38 ` Eric Dumazet
2011-11-21 19:29 ` Matt Carlson [this message]
2011-11-21 22:40 ` Hiroaki SHIMODA
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20111121192908.GA1198@mcarlson.broadcom.com \
--to=mcarlson@broadcom$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=mchan@broadcom$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=shimoda.hiroaki@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox