<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 22, 2018 at 11:23 AM, Alexander Duyck <span dir="ltr"><<a href="mailto:alexander.duyck@gmail.com" target="_blank">alexander.duyck@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, Jun 22, 2018 at 3:05 AM, Grönke, Christian <<a href="mailto:C.Groenke@infodas.de">C.Groenke@infodas.de</a>> wrote:<br>
> Hello Aaron, Hello Jeff,<br>
><br>
> I have problems with the newer versions of the e1000 (igb) driver of current kernels. I stumbled over this with a vendor kernel[*] after they pulled in some updates. When starting on my board, which has several i210 MACs connected to SFP ports. In case I use the hardware flashed for SGMII and SGMII capable SFP modules I see errors like this:<br>
><br>
>> [ 4776.137522] igb: Intel(R) Gigabit Ethernet Network Driver - version 5.4.0-k<br>
>> [ 4776.137525] igb: Copyright (c) 2007-2014 Intel Corporation.<br>
> [...]<br>
>> [ 4776.337079] igb: probe of 0000:09:00.0 failed with error -3<br>
><br>
> I tracked this down to the failing reset that was introduced by Aaron[*]:<br>
>> <a href="https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/intel/igb/e1000_82575.c?id=182785335447957409282ca745aa5bc3968facee" rel="noreferrer" target="_blank">https://git.kernel.org/pub/<wbr>scm/linux/kernel/git/torvalds/<wbr>linux.git/commit/drivers/net/<wbr>ethernet/intel/igb/e1000_<wbr>82575.c?id=<wbr>182785335447957409282ca745aa5b<wbr>c3968facee</a><br>
>> igb: reset the PHY before reading the PHY ID<br>
><br>
> The problem here, at least for me, is that during this time in the initialization the phy->addr is '0' and thus 'e1000_write_phy_reg_sgmii_<wbr>82575' fails. The SGMII PHY will only be discovered later on in 'igb_get_phy_id_82575'.<br>
><br>
>> [ 4776.336987] igb: IGB: Soft resetting SGMII attached PHY...<br>
>> [ 4776.336999] IGB: PHY I2C Address 0 is out of range.<br>
>> [ 4776.337009] igb: IGB: Error resetting the PHY.<br>
><br>
> When hacking the driver and presenting the PHY to the correct address, or doing a discovery of the PHY in case SGMII is active and MDIO is not used, it works. However, it seems a bit opposed to your patch. I understand your patch note in the way that in some cases the PHY has to be reset as otherwise the PHY ID can not be discovered and the "scan" would thus fail. May this inly be the case if SGMII is not used and thus 'igb_phy_hw_reset' would normally be called? It seems that in case SGMII is used the phy->addr has to be known to do a reset.<br>
><br>
> I know too little about all the possible IGB/i210 configuration to judge what is okay here. This is why I wanted to get in touch. I am not sure what would be the right fix. While I can fix it for myself and our product by disabling your patch and have no reset, I would much prefer an upstream fix that I can backport.<br>
><br>
> Kind regards<br>
> Christian<br>
><br>
> [*] Note: This is still the case in the most current driver on the master branch.<br>
> [*] Note2: I first observed this on a L4Linux 4.9 (<a href="https://l4linux.org/" rel="noreferrer" target="_blank">https://l4linux.org/</a>) running on L4RE. However, I can also reproduce this with a current CentOS 7.5 which has a 3.10++ kernel and a Ubuntu 18.04.<br>
><br>
> --<br>
><br>
> Christian GRÖNKE<br>
<br>
</span>Hi Christian,<br>
<br>
Looking over the code I would say it is probably acceptable to just<br>
revert the patch. In addition you could also probably revert commit<br>
440aeca4b985 "igb: Explicitly select page 0 at initialization". Both<br>
of those patches seem to be redundant as this was also fixed via a<br>
third commit 4e684f59d760 "igb: Workaround for igb i210 firmware<br>
issue" which I feel did a much better job of addressing the issue<br>
without introducing other side effects.<br>
<br>
If you want to submit the patches to revert the first commit you<br>
mentioned, and the one I mentioned I would be willing to ack it and<br>
see if we can get it pushed through the tree.<br>
<br>
Thanks.<br>
<br>
- Alex<br>
</blockquote></div><br></div><div class="gmail_extra">I would be happy to test this patch out. It did solve a specific problem on the machine I tested this on; so I would not want to regress this fix.</div><div class="gmail_extra">Thanks,</div><div class="gmail_extra">--chris</div><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div></div>