[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

Hi,

I have an Intel I210 NIC which shows:
'igb: probe of 0000:07:00.0 failed wtih error -2'
Upon boot after power has been completely disconnected.

v4.4 works, while v4.5 fails and I was able to bisect it down to:
commit 2a3cdead8b408351fa1e3079b220fa331480ffbc

Any ideas for a fix? I'm happy to test any patches.

(apologies for the resend, typed the wrong address.)

--chris

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Aaron Sierra]

Chris,
Someone else reported a similar issue back in April. Would you
mind taking a look at the discussion that we had back then to
see if it applies to you?

http://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20160425/005171.html

-- 
--- Extreme Engineering Solutions, Inc (X-ES) ---
Aaron Sierra
Senior Embedded Engineer
X-ES http://www.xes-inc.com
3225 Deming Way, Ste 120
Middleton, WI  53562
Phone: (608) 833-1155 x115
Email: asierra at xes-inc.com

----- Original Message -----
> From: "Chris Arges" <carges@vectranetworks.com>
> To: "Aaron Sierra" <asierra@xes-inc.com>, "jeffrey t kirsher" <jeffrey.t.kirsher@intel.com>
> Cc: "intel-wired-lan" <intel-wired-lan@lists.osuosl.org>
> Sent: Wednesday, October 12, 2016 2:51:41 PM
> Subject: Bug with 'igb: Remove GS40G specific defines/functions'

> Hi,
> 
> I have an Intel I210 NIC which shows:
> 'igb: probe of 0000:07:00.0 failed wtih error -2'
> Upon boot after power has been completely disconnected.
> 
> v4.4 works, while v4.5 fails and I was able to bisect it down to:
> commit 2a3cdead8b408351fa1e3079b220fa331480ffbc
> 
> Any ideas for a fix? I'm happy to test any patches.
> 
> (apologies for the resend, typed the wrong address.)
> 
> --chris

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

On 10/12/16, 3:19 PM, "Aaron Sierra" <asierra@xes-inc.com> wrote:

    Chris,
    Someone else reported a similar issue back in April. Would you
    mind taking a look at the discussion that we had back then to
    see if it applies to you?
    
I?m not sure exactly which part I need to comment out to verify in ?igb_init_phy_params_82575()?.
Is there a fix already planned for this? Any additional information I can provide?
Thanks,
--chris


    http://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20160425/005171.html
    
    -- 
    --- Extreme Engineering Solutions, Inc (X-ES) ---
    Aaron Sierra
    Senior Embedded Engineer
    X-ES http://www.xes-inc.com
    3225 Deming Way, Ste 120
    Middleton, WI  53562
    Phone: (608) 833-1155 x115
    Email: asierra at xes-inc.com
    
    ----- Original Message -----
    > From: "Chris Arges" <carges@vectranetworks.com>
    > To: "Aaron Sierra" <asierra@xes-inc.com>, "jeffrey t kirsher" <jeffrey.t.kirsher@intel.com>
    > Cc: "intel-wired-lan" <intel-wired-lan@lists.osuosl.org>
    > Sent: Wednesday, October 12, 2016 2:51:41 PM
    > Subject: Bug with 'igb: Remove GS40G specific defines/functions'
    
    > Hi,
    > 
    > I have an Intel I210 NIC which shows:
    > 'igb: probe of 0000:07:00.0 failed wtih error -2'
    > Upon boot after power has been completely disconnected.
    > 
    > v4.4 works, while v4.5 fails and I was able to bisect it down to:
    > commit 2a3cdead8b408351fa1e3079b220fa331480ffbc
    > 
    > Any ideas for a fix? I'm happy to test any patches.
    > 
    > (apologies for the resend, typed the wrong address.)
    > 
    > --chris
    

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Aaron Sierra]

Chris,

> I?m not sure exactly which part I need to comment out to verify in
> ?igb_init_phy_params_82575()?.
> Is there a fix already planned for this? Any additional information I can
> provide?

Did you follow any of the "Next message" links? There is more to the
discussion than that first page that I linked to.

In that discussion we determined that the PHY was somehow setup with a
non-zero register page setting. That was treated as a BIOS bug rather
than a driver bug.

If you're having the same problem, then it could be possible to explicitly
set the default page to zero, but I'm not sure where that _should_ be done
or if that is generally the correct/safe thing to do.

-Aaron

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

On 10/12/16, 3:46 PM, "Aaron Sierra" <asierra@xes-inc.com> wrote:

    Chris,
    
    > I?m not sure exactly which part I need to comment out to verify in
    > ?igb_init_phy_params_82575()?.
    > Is there a fix already planned for this? Any additional information I can
    > provide?
    
    Did you follow any of the "Next message" links? There is more to the
    discussion than that first page that I linked to.
    
    In that discussion we determined that the PHY was somehow setup with a
    non-zero register page setting. That was treated as a BIOS bug rather
    than a driver bug.
    
    If you're having the same problem, then it could be possible to explicitly
    set the default page to zero, but I'm not sure where that _should_ be done
    or if that is generally the correct/safe thing to do.
    
    -Aaron
    
Aaron,
Ah sorry about that, I thought you were referring just to that particular post.
I?ll forward this along to the BIOS vendor.
Thanks,
--chris

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Jochen Henneberg]

On Mi, 2016-10-12 at 21:18 +0000, Chris Arges wrote:
> 
> On 10/12/16, 3:46 PM, "Aaron Sierra" <asierra@xes-inc.com> wrote:
> 
>     Chris,
>     
>     > I?m not sure exactly which part I need to comment out to verify in
>     > ?igb_init_phy_params_82575()?.
>     > Is there a fix already planned for this? Any additional information I can
>     > provide?
>     
>     Did you follow any of the "Next message" links? There is more to the
>     discussion than that first page that I linked to.
>     
>     In that discussion we determined that the PHY was somehow setup with a
>     non-zero register page setting. That was treated as a BIOS bug rather
>     than a driver bug.
>     
>     If you're having the same problem, then it could be possible to explicitly
>     set the default page to zero, but I'm not sure where that _should_ be done
>     or if that is generally the correct/safe thing to do.
>     
>     -Aaron
>     
> Aaron,
> Ah sorry about that, I thought you were referring just to that particular post.
> I?ll forward this along to the BIOS vendor.
> Thanks,
> --chris

Chris,

I was the original reporter of that issue. Find attached the patch that
I currently use to ensure correct page selection before the
configuration is started (id read).
I am not sure if this is a good fix (though it is quite small which I
would assume makes it good) and it applies to 4.5 kernel, I did not test
with current head.
The chips datasheet says that the initial page selection is 0, so this
is assumed to be obsolete, but the BIOS may mess up things and as not
everybody has means to modify or request a BIOS update it might make
sense to bring this patch into the driver. It comes with almost no cost
from what I can see.
Please let me know if this fixes your problem.

Regards
-Jochen


> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0026-igb-reset-page-select-to-0-on-initial-phy-id-read.patch
Type: text/x-patch
Size: 847 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161013/c9c182c4/attachment.bin>

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

On 10/13/16, 3:58 AM, "Jochen Henneberg" <jh@henneberg-systemdesign.com> wrote:

    On Mi, 2016-10-12 at 21:18 +0000, Chris Arges wrote:
    > 
    > On 10/12/16, 3:46 PM, "Aaron Sierra" <asierra@xes-inc.com> wrote:
    > 
    >     Chris,
    >     
    >     > I?m not sure exactly which part I need to comment out to verify in
    >     > ?igb_init_phy_params_82575()?.
    >     > Is there a fix already planned for this? Any additional information I can
    >     > provide?
    >     
    >     Did you follow any of the "Next message" links? There is more to the
    >     discussion than that first page that I linked to.
    >     
    >     In that discussion we determined that the PHY was somehow setup with a
    >     non-zero register page setting. That was treated as a BIOS bug rather
    >     than a driver bug.
    >     
    >     If you're having the same problem, then it could be possible to explicitly
    >     set the default page to zero, but I'm not sure where that _should_ be done
    >     or if that is generally the correct/safe thing to do.
    >     
    >     -Aaron
    >     
    > Aaron,
    > Ah sorry about that, I thought you were referring just to that particular post.
    > I?ll forward this along to the BIOS vendor.
    > Thanks,
    > --chris
    
    Chris,
    
    I was the original reporter of that issue. Find attached the patch that
    I currently use to ensure correct page selection before the
    configuration is started (id read).
    I am not sure if this is a good fix (though it is quite small which I
    would assume makes it good) and it applies to 4.5 kernel, I did not test
    with current head.
    The chips datasheet says that the initial page selection is 0, so this
    is assumed to be obsolete, but the BIOS may mess up things and as not
    everybody has means to modify or request a BIOS update it might make
    sense to bring this patch into the driver. It comes with almost no cost
    from what I can see.
    Please let me know if this fixes your problem.
    
    Regards
    -Jochen
    

Jochen,

Thanks for this update. I tested this patch and it fixes my issue!
If there are no other ill effects from this patch perhaps it would be a good
candidate for mainline and stable.

--chris



    
    > 
    > _______________________________________________
    > Intel-wired-lan mailing list
    > Intel-wired-lan at lists.osuosl.org
    > http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
    
    -- 
    Henneberg - Systemdesign
    Jochen Henneberg
    Loehnfeld 26
    21423 Winsen (Luhe)
    --
    Fon: +49 4174 668 773
    Mobile: +49 172 160 14 69
    Fax: +49 321 210 761 64
    www: www.henneberg-systemdesign.com
    
    
    

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Jochen Henneberg]

On Do, 2016-10-13 at 13:56 +0000, Chris Arges wrote:
> Jochen,
> 
> Thanks for this update. I tested this patch and it fixes my issue!
> If there are no other ill effects from this patch perhaps it would be
> a good
> candidate for mainline and stable.

The patch is hacky, it does neither use the phy-ops.write_reg() nor does
it check for the correct chip before it applies the write.
I will try to prepare a clean fix and send a patch.

Regards
-Jochen

> 
> --chris

-- 
Henneberg - Systemdesign
Jochen Henneberg
Loehnfeld 26
21423 Winsen (Luhe)
--
Fon: +49 4174 668 773
Mobile: +49 172 160 14 69
Fax: +49 321 210 761 64
www: www.henneberg-systemdesign.com

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

On 10/14/16, 6:24 AM, "Jochen Henneberg" <jh@henneberg-systemdesign.com> wrote:

    On Do, 2016-10-13 at 13:56 +0000, Chris Arges wrote:
    > Jochen,
    > 
    > Thanks for this update. I tested this patch and it fixes my issue!
    > If there are no other ill effects from this patch perhaps it would be
    > a good
    > candidate for mainline and stable.
    
    The patch is hacky, it does neither use the phy-ops.write_reg() nor does
    it check for the correct chip before it applies the write.
    I will try to prepare a clean fix and send a patch.
    
    Regards
    -Jochen
    
What about something as follows? This fixes the issue for me.

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 5b54254..6d42dfe 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -77,6 +77,10 @@ s32 igb_get_phy_id(struct e1000_hw *hw)
        s32 ret_val = 0;
        u16 phy_id;

+       /* ensure phy page selection to fix misconfigured i210 */
+       if (hw->mac.type == e1000_i210)
+               phy->ops.write_reg(hw, I347AT4_PAGE_SELECT, 0);
+
        ret_val = phy->ops.read_reg(hw, PHY_ID1, &phy_id);
        if (ret_val)
                goto out;

    > 
    > --chris
    
    -- 
    Henneberg - Systemdesign
    Jochen Henneberg
    Loehnfeld 26
    21423 Winsen (Luhe)
    --
    Fon: +49 4174 668 773
    Mobile: +49 172 160 14 69
    Fax: +49 321 210 761 64
    www: www.henneberg-systemdesign.com
    
    
    
    

[Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

[Chris Arges]

Jochen,

Not sure if you had time to look at this, do you thing the patch I sent previously is on the right path?

I'm happy to submit a formal patch to the list.

Thanks,

--chris

________________________________
From: Chris Arges
Sent: Monday, October 17, 2016 1:36:38 PM
To: Jochen Henneberg
Cc: Aaron Sierra; intel-wired-lan
Subject: Re: [Intel-wired-lan] Bug with 'igb: Remove GS40G specific defines/functions'

On 10/14/16, 6:24 AM, "Jochen Henneberg" <jh@henneberg-systemdesign.com> wrote:

    On Do, 2016-10-13 at 13:56 +0000, Chris Arges wrote:
    > Jochen,
    >
    > Thanks for this update. I tested this patch and it fixes my issue!
    > If there are no other ill effects from this patch perhaps it would be
    > a good
    > candidate for mainline and stable.

    The patch is hacky, it does neither use the phy-ops.write_reg() nor does
    it check for the correct chip before it applies the write.
    I will try to prepare a clean fix and send a patch.

    Regards
    -Jochen

What about something as follows? This fixes the issue for me.

diff --git a/drivers/net/ethernet/intel/igb/e1000_phy.c b/drivers/net/ethernet/intel/igb/e1000_phy.c
index 5b54254..6d42dfe 100644
--- a/drivers/net/ethernet/intel/igb/e1000_phy.c
+++ b/drivers/net/ethernet/intel/igb/e1000_phy.c
@@ -77,6 +77,10 @@ s32 igb_get_phy_id(struct e1000_hw *hw)
        s32 ret_val = 0;
        u16 phy_id;

+       /* ensure phy page selection to fix misconfigured i210 */
+       if (hw->mac.type == e1000_i210)
+               phy->ops.write_reg(hw, I347AT4_PAGE_SELECT, 0);
+
        ret_val = phy->ops.read_reg(hw, PHY_ID1, &phy_id);
        if (ret_val)
                goto out;

    >
    > --chris

    --
    Henneberg - Systemdesign
    Jochen Henneberg
    Loehnfeld 26
    21423 Winsen (Luhe)
    --
    Fon: +49 4174 668 773
    Mobile: +49 172 160 14 69
    Fax: +49 321 210 761 64
    www: www.henneberg-systemdesign.com<http://www.henneberg-systemdesign.com>





-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161027/1c327c42/attachment-0001.html>