* [PATCH net-next] tg3: Fix advertisement handling
@ 2011-11-20 23:07 Hiroaki SHIMODA
2011-11-21 6:38 ` Eric Dumazet
2011-11-21 19:29 ` Matt Carlson
0 siblings, 2 replies; 4+ messages in thread
From: Hiroaki SHIMODA @ 2011-11-20 23:07 UTC (permalink / raw)
To: davem, mcarlson, mchan; +Cc: netdev
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)) {
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] tg3: Fix advertisement handling
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
1 sibling, 0 replies; 4+ messages in thread
From: Eric Dumazet @ 2011-11-21 6:38 UTC (permalink / raw)
To: Hiroaki SHIMODA; +Cc: davem, mcarlson, mchan, netdev
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 !
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next] tg3: Fix advertisement handling
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
2011-11-21 22:40 ` Hiroaki SHIMODA
1 sibling, 1 reply; 4+ messages in thread
From: Matt Carlson @ 2011-11-21 19:29 UTC (permalink / raw)
To: Hiroaki SHIMODA
Cc: davem@davemloft.net, Matthew Carlson, Michael Chan,
netdev@vger.kernel.org
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)) {
>
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net-next] tg3: Fix advertisement handling
2011-11-21 19:29 ` Matt Carlson
@ 2011-11-21 22:40 ` Hiroaki SHIMODA
0 siblings, 0 replies; 4+ messages in thread
From: Hiroaki SHIMODA @ 2011-11-21 22:40 UTC (permalink / raw)
To: mcarlson; +Cc: davem, mchan, eric.dumazet, netdev
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)) {
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-11-21 22:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
2011-11-21 22:40 ` Hiroaki SHIMODA
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).