Testing of r8169 workaround removal

Testing of r8169 workaround removal

[Heiner Kallweit]

Hi Neil,

you once reported the original issue resulting in this workaround.
This workaround shouldn't be needed any longer, but I have no affected HW
to test on. Do you have the option to apply the patch below to latest
net-next and test link speed after resume from suspend?
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
That would be much appreciated.

Heiner

----------------------------------------------------------------

After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
workaround shouldn't be needed any longer. However I don't have
affected hardware so I can't test it.

This was the bug report leading to the workaround:
https://bugzilla.kernel.org/show_bug.cgi?id=201081

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 383242df0..d4ec08e37 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
 	phy_speed_up(tp->phydev);
 
 	genphy_soft_reset(tp->phydev);
-
-	/* It was reported that several chips end up with 10MBit/Half on a
-	 * 1GBit link after resuming from S3. For whatever reason the PHY on
-	 * these chips doesn't properly start a renegotiation when soft-reset.
-	 * Explicitly requesting a renegotiation fixes this.
-	 */
-	if (tp->phydev->autoneg == AUTONEG_ENABLE)
-		phy_restart_aneg(tp->phydev);
 }
 
 static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
-- 
2.21.0

Re: Testing of r8169 workaround removal

[Neil MacLeod]

Hi Heiner

Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
as in order to test your request I thought I would reproduce the issue
with plain 5.1-rc6 with the workaround removed, however without the
workaround 5.1-rc6 is resuming correctly at 1000Mbps.

I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
I can no longer reproduce the issue when the workaround is removed.

I also tested 5.0.10 without the workaround, and again 5.0.10 is
resuming correctly at 1000Mbps.

I finally tested 4.19.23 without the workaround (the last iteration of
this kernel I published) and this does NOT resume correctly at
1000Mbps (it resumes at 10Mbps).

I'll test a few more iterations of 5.0.y to see if I can identify when
it was "fixed" but if you have any suggestions when it might have been
fixed I can try to confirm this that - currently it's somewhere
between 4.19.24 and 5.0.10!

Regards
Neil




On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Hi Neil,
>
> you once reported the original issue resulting in this workaround.
> This workaround shouldn't be needed any longer, but I have no affected HW
> to test on. Do you have the option to apply the patch below to latest
> net-next and test link speed after resume from suspend?
> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> That would be much appreciated.
>
> Heiner
>
> ----------------------------------------------------------------
>
> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
> workaround shouldn't be needed any longer. However I don't have
> affected hardware so I can't test it.
>
> This was the bug report leading to the workaround:
> https://bugzilla.kernel.org/show_bug.cgi?id=201081
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index 383242df0..d4ec08e37 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>         phy_speed_up(tp->phydev);
>
>         genphy_soft_reset(tp->phydev);
> -
> -       /* It was reported that several chips end up with 10MBit/Half on a
> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
> -        * these chips doesn't properly start a renegotiation when soft-reset.
> -        * Explicitly requesting a renegotiation fixes this.
> -        */
> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
> -               phy_restart_aneg(tp->phydev);
>  }
>
>  static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
> --
> 2.21.0

Re: Testing of r8169 workaround removal

[Heiner Kallweit]

Hi Neil,

thanks for reporting back. Interesting, then the root cause of the
issue seems to have been in a different corner. On my hardware
I'm not able to reproduce the issue. It's not that relevant with which
exact version the issue vanished. Based on your results I'll just
remove the workaround on net-next (adding your Tested-by).

Heiner


On 28.04.2019 19:30, Neil MacLeod wrote:
> Hi Heiner
> 
> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
> as in order to test your request I thought I would reproduce the issue
> with plain 5.1-rc6 with the workaround removed, however without the
> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
> 
> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
> I can no longer reproduce the issue when the workaround is removed.
> 
> I also tested 5.0.10 without the workaround, and again 5.0.10 is
> resuming correctly at 1000Mbps.
> 
> I finally tested 4.19.23 without the workaround (the last iteration of
> this kernel I published) and this does NOT resume correctly at
> 1000Mbps (it resumes at 10Mbps).
> 
> I'll test a few more iterations of 5.0.y to see if I can identify when
> it was "fixed" but if you have any suggestions when it might have been
> fixed I can try to confirm this that - currently it's somewhere
> between 4.19.24 and 5.0.10!
> 
> Regards
> Neil
> 
> 
> 
> 
> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Hi Neil,
>>
>> you once reported the original issue resulting in this workaround.
>> This workaround shouldn't be needed any longer, but I have no affected HW
>> to test on. Do you have the option to apply the patch below to latest
>> net-next and test link speed after resume from suspend?
>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>> That would be much appreciated.
>>
>> Heiner
>>
>> ----------------------------------------------------------------
>>
>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
>> workaround shouldn't be needed any longer. However I don't have
>> affected hardware so I can't test it.
>>
>> This was the bug report leading to the workaround:
>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169.c | 8 --------
>>  1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>> index 383242df0..d4ec08e37 100644
>> --- a/drivers/net/ethernet/realtek/r8169.c
>> +++ b/drivers/net/ethernet/realtek/r8169.c
>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>         phy_speed_up(tp->phydev);
>>
>>         genphy_soft_reset(tp->phydev);
>> -
>> -       /* It was reported that several chips end up with 10MBit/Half on a
>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
>> -        * these chips doesn't properly start a renegotiation when soft-reset.
>> -        * Explicitly requesting a renegotiation fixes this.
>> -        */
>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
>> -               phy_restart_aneg(tp->phydev);
>>  }
>>
>>  static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
>> --
>> 2.21.0
> 

Re: Testing of r8169 workaround removal

[Neil MacLeod]

Hi Heiner

I'd already kicked off a 5.0.2 build without the workaround and I've
tested that now, and it resumes at 10Mbps, so it may still be worth
identifying the exact 5.0.y version when it was fixed just in case
that provides some understanding of how it was fixed... I'll test the
remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
work and let you know what I find!

Regards
Neil

On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Hi Neil,
>
> thanks for reporting back. Interesting, then the root cause of the
> issue seems to have been in a different corner. On my hardware
> I'm not able to reproduce the issue. It's not that relevant with which
> exact version the issue vanished. Based on your results I'll just
> remove the workaround on net-next (adding your Tested-by).
>
> Heiner
>
>
> On 28.04.2019 19:30, Neil MacLeod wrote:
> > Hi Heiner
> >
> > Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
> > as in order to test your request I thought I would reproduce the issue
> > with plain 5.1-rc6 with the workaround removed, however without the
> > workaround 5.1-rc6 is resuming correctly at 1000Mbps.
> >
> > I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
> > the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
> > I can no longer reproduce the issue when the workaround is removed.
> >
> > I also tested 5.0.10 without the workaround, and again 5.0.10 is
> > resuming correctly at 1000Mbps.
> >
> > I finally tested 4.19.23 without the workaround (the last iteration of
> > this kernel I published) and this does NOT resume correctly at
> > 1000Mbps (it resumes at 10Mbps).
> >
> > I'll test a few more iterations of 5.0.y to see if I can identify when
> > it was "fixed" but if you have any suggestions when it might have been
> > fixed I can try to confirm this that - currently it's somewhere
> > between 4.19.24 and 5.0.10!
> >
> > Regards
> > Neil
> >
> >
> >
> >
> > On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> Hi Neil,
> >>
> >> you once reported the original issue resulting in this workaround.
> >> This workaround shouldn't be needed any longer, but I have no affected HW
> >> to test on. Do you have the option to apply the patch below to latest
> >> net-next and test link speed after resume from suspend?
> >> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> >> That would be much appreciated.
> >>
> >> Heiner
> >>
> >> ----------------------------------------------------------------
> >>
> >> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
> >> workaround shouldn't be needed any longer. However I don't have
> >> affected hardware so I can't test it.
> >>
> >> This was the bug report leading to the workaround:
> >> https://bugzilla.kernel.org/show_bug.cgi?id=201081
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/net/ethernet/realtek/r8169.c | 8 --------
> >>  1 file changed, 8 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> >> index 383242df0..d4ec08e37 100644
> >> --- a/drivers/net/ethernet/realtek/r8169.c
> >> +++ b/drivers/net/ethernet/realtek/r8169.c
> >> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> >>         phy_speed_up(tp->phydev);
> >>
> >>         genphy_soft_reset(tp->phydev);
> >> -
> >> -       /* It was reported that several chips end up with 10MBit/Half on a
> >> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
> >> -        * these chips doesn't properly start a renegotiation when soft-reset.
> >> -        * Explicitly requesting a renegotiation fixes this.
> >> -        */
> >> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
> >> -               phy_restart_aneg(tp->phydev);
> >>  }
> >>
> >>  static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
> >> --
> >> 2.21.0
> >
>

Re: Testing of r8169 workaround removal

[Heiner Kallweit]

Interesting, thanks for your efforts! I submitted the patch removing
the workaround because it seems now (at least since 5.1-rc1) we're fine.

Heiner

On 28.04.2019 20:40, Neil MacLeod wrote:
> Hi Heiner
> 
> I'd already kicked off a 5.0.2 build without the workaround and I've
> tested that now, and it resumes at 10Mbps, so it may still be worth
> identifying the exact 5.0.y version when it was fixed just in case
> that provides some understanding of how it was fixed... I'll test the
> remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
> work and let you know what I find!
> 
> Regards
> Neil
> 
> On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Hi Neil,
>>
>> thanks for reporting back. Interesting, then the root cause of the
>> issue seems to have been in a different corner. On my hardware
>> I'm not able to reproduce the issue. It's not that relevant with which
>> exact version the issue vanished. Based on your results I'll just
>> remove the workaround on net-next (adding your Tested-by).
>>
>> Heiner
>>
>>
>> On 28.04.2019 19:30, Neil MacLeod wrote:
>>> Hi Heiner
>>>
>>> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
>>> as in order to test your request I thought I would reproduce the issue
>>> with plain 5.1-rc6 with the workaround removed, however without the
>>> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
>>>
>>> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
>>> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
>>> I can no longer reproduce the issue when the workaround is removed.
>>>
>>> I also tested 5.0.10 without the workaround, and again 5.0.10 is
>>> resuming correctly at 1000Mbps.
>>>
>>> I finally tested 4.19.23 without the workaround (the last iteration of
>>> this kernel I published) and this does NOT resume correctly at
>>> 1000Mbps (it resumes at 10Mbps).
>>>
>>> I'll test a few more iterations of 5.0.y to see if I can identify when
>>> it was "fixed" but if you have any suggestions when it might have been
>>> fixed I can try to confirm this that - currently it's somewhere
>>> between 4.19.24 and 5.0.10!
>>>
>>> Regards
>>> Neil
>>>
>>>
>>>
>>>
>>> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>
>>>> Hi Neil,
>>>>
>>>> you once reported the original issue resulting in this workaround.
>>>> This workaround shouldn't be needed any longer, but I have no affected HW
>>>> to test on. Do you have the option to apply the patch below to latest
>>>> net-next and test link speed after resume from suspend?
>>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>>>> That would be much appreciated.
>>>>
>>>> Heiner
>>>>
>>>> ----------------------------------------------------------------
>>>>
>>>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
>>>> workaround shouldn't be needed any longer. However I don't have
>>>> affected hardware so I can't test it.
>>>>
>>>> This was the bug report leading to the workaround:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/net/ethernet/realtek/r8169.c | 8 --------
>>>>  1 file changed, 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>>> index 383242df0..d4ec08e37 100644
>>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>>>         phy_speed_up(tp->phydev);
>>>>
>>>>         genphy_soft_reset(tp->phydev);
>>>> -
>>>> -       /* It was reported that several chips end up with 10MBit/Half on a
>>>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
>>>> -        * these chips doesn't properly start a renegotiation when soft-reset.
>>>> -        * Explicitly requesting a renegotiation fixes this.
>>>> -        */
>>>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
>>>> -               phy_restart_aneg(tp->phydev);
>>>>  }
>>>>
>>>>  static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
>>>> --
>>>> 2.21.0
>>>
>>
> 

Re: Testing of r8169 workaround removal

[Neil MacLeod]

Hi Heiner

5.0.6 is the first kernel that does NOT require the workaround.

In 5.0.6 the only obvious r8169 change (to my untrained eyes) is:

https://github.com/torvalds/linux/commit/4951fc65d9153deded3d066ab371a61977c96e8a

but reverting this change in addition to the workaround makes no
difference, the resulting kernel still resumes at 1000Mbps so I'm not
sure what other change in .5.0.6 might be responsible for this changed
behaviour. If you can think of anything I'll give it a try!

Regards
Neil

PS. A while ago (5 Dec 2018 to be precise!) I emailed you about the
ASPM issue which it looks like you may have fixed in 5.1-rc5[1].
Unfortunately I don't have this issue myself, and I've been trying to
get feedback from the bug reporter[2,3] "Matt Devo" without much
success but will confirm to you if/when he replies.

1, https://bugzilla.kernel.org/show_bug.cgi?id=202945
2. https://forum.kodi.tv/showthread.php?tid=298462&pid=2845944#pid2845944
3. https://forum.kodi.tv/showthread.php?tid=343069&pid=2850123#pid2850123

On Sun, 28 Apr 2019 at 19:43, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Interesting, thanks for your efforts! I submitted the patch removing
> the workaround because it seems now (at least since 5.1-rc1) we're fine.
>
> Heiner
>
> On 28.04.2019 20:40, Neil MacLeod wrote:
> > Hi Heiner
> >
> > I'd already kicked off a 5.0.2 build without the workaround and I've
> > tested that now, and it resumes at 10Mbps, so it may still be worth
> > identifying the exact 5.0.y version when it was fixed just in case
> > that provides some understanding of how it was fixed... I'll test the
> > remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
> > work and let you know what I find!
> >
> > Regards
> > Neil
> >
> > On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> Hi Neil,
> >>
> >> thanks for reporting back. Interesting, then the root cause of the
> >> issue seems to have been in a different corner. On my hardware
> >> I'm not able to reproduce the issue. It's not that relevant with which
> >> exact version the issue vanished. Based on your results I'll just
> >> remove the workaround on net-next (adding your Tested-by).
> >>
> >> Heiner
> >>
> >>
> >> On 28.04.2019 19:30, Neil MacLeod wrote:
> >>> Hi Heiner
> >>>
> >>> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
> >>> as in order to test your request I thought I would reproduce the issue
> >>> with plain 5.1-rc6 with the workaround removed, however without the
> >>> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
> >>>
> >>> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
> >>> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
> >>> I can no longer reproduce the issue when the workaround is removed.
> >>>
> >>> I also tested 5.0.10 without the workaround, and again 5.0.10 is
> >>> resuming correctly at 1000Mbps.
> >>>
> >>> I finally tested 4.19.23 without the workaround (the last iteration of
> >>> this kernel I published) and this does NOT resume correctly at
> >>> 1000Mbps (it resumes at 10Mbps).
> >>>
> >>> I'll test a few more iterations of 5.0.y to see if I can identify when
> >>> it was "fixed" but if you have any suggestions when it might have been
> >>> fixed I can try to confirm this that - currently it's somewhere
> >>> between 4.19.24 and 5.0.10!
> >>>
> >>> Regards
> >>> Neil
> >>>
> >>>
> >>>
> >>>
> >>> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>>>
> >>>> Hi Neil,
> >>>>
> >>>> you once reported the original issue resulting in this workaround.
> >>>> This workaround shouldn't be needed any longer, but I have no affected HW
> >>>> to test on. Do you have the option to apply the patch below to latest
> >>>> net-next and test link speed after resume from suspend?
> >>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> >>>> That would be much appreciated.
> >>>>
> >>>> Heiner
> >>>>
> >>>> ----------------------------------------------------------------
> >>>>
> >>>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
> >>>> workaround shouldn't be needed any longer. However I don't have
> >>>> affected hardware so I can't test it.
> >>>>
> >>>> This was the bug report leading to the workaround:
> >>>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
> >>>>
> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>> ---
> >>>>  drivers/net/ethernet/realtek/r8169.c | 8 --------
> >>>>  1 file changed, 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> >>>> index 383242df0..d4ec08e37 100644
> >>>> --- a/drivers/net/ethernet/realtek/r8169.c
> >>>> +++ b/drivers/net/ethernet/realtek/r8169.c
> >>>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> >>>>         phy_speed_up(tp->phydev);
> >>>>
> >>>>         genphy_soft_reset(tp->phydev);
> >>>> -
> >>>> -       /* It was reported that several chips end up with 10MBit/Half on a
> >>>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
> >>>> -        * these chips doesn't properly start a renegotiation when soft-reset.
> >>>> -        * Explicitly requesting a renegotiation fixes this.
> >>>> -        */
> >>>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
> >>>> -               phy_restart_aneg(tp->phydev);
> >>>>  }
> >>>>
> >>>>  static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
> >>>> --
> >>>> 2.21.0
> >>>
> >>
> >
>

Re: Testing of r8169 workaround removal

[Phil Reid]

On 29/04/2019 6:05 am, Neil MacLeod wrote:
> Hi Heiner
> 
> 5.0.6 is the first kernel that does NOT require the workaround.
> 
> In 5.0.6 the only obvious r8169 change (to my untrained eyes) is:
> 
> https://github.com/torvalds/linux/commit/4951fc65d9153deded3d066ab371a61977c96e8a
> 
> but reverting this change in addition to the workaround makes no
> difference, the resulting kernel still resumes at 1000Mbps so I'm not
> sure what other change in .5.0.6 might be responsible for this changed
> behaviour. If you can think of anything I'll give it a try!
> 
> Regards
> Neil

The symptom sounds very similar to a problem I had with 1G link only linking at 10M.

Perhaps have a look at:
net: phy: don't clear BMCR in genphy_soft_reset

https://www.spinics.net/lists/netdev/msg559627.html

Which looks to have been added in 5.0.6
commit	fc8f36de77111bf925d19f347c21134542941a3c


> 
> PS. A while ago (5 Dec 2018 to be precise!) I emailed you about the
> ASPM issue which it looks like you may have fixed in 5.1-rc5[1].
> Unfortunately I don't have this issue myself, and I've been trying to
> get feedback from the bug reporter[2,3] "Matt Devo" without much
> success but will confirm to you if/when he replies.
> 
> 1, https://bugzilla.kernel.org/show_bug.cgi?id=202945
> 2. https://forum.kodi.tv/showthread.php?tid=298462&pid=2845944#pid2845944
> 3. https://forum.kodi.tv/showthread.php?tid=343069&pid=2850123#pid2850123
> 
> On Sun, 28 Apr 2019 at 19:43, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Interesting, thanks for your efforts! I submitted the patch removing
>> the workaround because it seems now (at least since 5.1-rc1) we're fine.
>>
>> Heiner
>>
>> On 28.04.2019 20:40, Neil MacLeod wrote:
>>> Hi Heiner
>>>
>>> I'd already kicked off a 5.0.2 build without the workaround and I've
>>> tested that now, and it resumes at 10Mbps, so it may still be worth
>>> identifying the exact 5.0.y version when it was fixed just in case
>>> that provides some understanding of how it was fixed... I'll test the
>>> remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
>>> work and let you know what I find!
>>>
>>> Regards
>>> Neil
>>>
>>> On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>
>>>> Hi Neil,
>>>>
>>>> thanks for reporting back. Interesting, then the root cause of the
>>>> issue seems to have been in a different corner. On my hardware
>>>> I'm not able to reproduce the issue. It's not that relevant with which
>>>> exact version the issue vanished. Based on your results I'll just
>>>> remove the workaround on net-next (adding your Tested-by).
>>>>
>>>> Heiner
>>>>
>>>>
>>>> On 28.04.2019 19:30, Neil MacLeod wrote:
>>>>> Hi Heiner
>>>>>
>>>>> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
>>>>> as in order to test your request I thought I would reproduce the issue
>>>>> with plain 5.1-rc6 with the workaround removed, however without the
>>>>> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
>>>>>
>>>>> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
>>>>> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
>>>>> I can no longer reproduce the issue when the workaround is removed.
>>>>>
>>>>> I also tested 5.0.10 without the workaround, and again 5.0.10 is
>>>>> resuming correctly at 1000Mbps.
>>>>>
>>>>> I finally tested 4.19.23 without the workaround (the last iteration of
>>>>> this kernel I published) and this does NOT resume correctly at
>>>>> 1000Mbps (it resumes at 10Mbps).
>>>>>
>>>>> I'll test a few more iterations of 5.0.y to see if I can identify when
>>>>> it was "fixed" but if you have any suggestions when it might have been
>>>>> fixed I can try to confirm this that - currently it's somewhere
>>>>> between 4.19.24 and 5.0.10!
>>>>>
>>>>> Regards
>>>>> Neil
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>>>>
>>>>>> Hi Neil,
>>>>>>
>>>>>> you once reported the original issue resulting in this workaround.
>>>>>> This workaround shouldn't be needed any longer, but I have no affected HW
>>>>>> to test on. Do you have the option to apply the patch below to latest
>>>>>> net-next and test link speed after resume from suspend?
>>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
>>>>>> That would be much appreciated.
>>>>>>
>>>>>> Heiner
>>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>>
>>>>>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
>>>>>> workaround shouldn't be needed any longer. However I don't have
>>>>>> affected hardware so I can't test it.
>>>>>>
>>>>>> This was the bug report leading to the workaround:
>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> ---
>>>>>>   drivers/net/ethernet/realtek/r8169.c | 8 --------
>>>>>>   1 file changed, 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
>>>>>> index 383242df0..d4ec08e37 100644
>>>>>> --- a/drivers/net/ethernet/realtek/r8169.c
>>>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
>>>>>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
>>>>>>          phy_speed_up(tp->phydev);
>>>>>>
>>>>>>          genphy_soft_reset(tp->phydev);
>>>>>> -
>>>>>> -       /* It was reported that several chips end up with 10MBit/Half on a
>>>>>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
>>>>>> -        * these chips doesn't properly start a renegotiation when soft-reset.
>>>>>> -        * Explicitly requesting a renegotiation fixes this.
>>>>>> -        */
>>>>>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
>>>>>> -               phy_restart_aneg(tp->phydev);
>>>>>>   }
>>>>>>
>>>>>>   static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
>>>>>> --
>>>>>> 2.21.0
>>>>>
>>>>
>>>
>>
> 
> 


-- 
Regards
Phil Reid

ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au

3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid@electromag.com.au

Re: Testing of r8169 workaround removal

[Neil MacLeod]

Phil

Many thanks - you've nailed it!

Reverting the workaround from Heiner, and also
fc8f36de77111bf925d19f347c2113, resulted in 5.0.6 syncing at 10Mbps
after resuming from S3 instead of the 1000Mbps it syncs at boot.

Hopefully this is helpful in understanding why the workaround is no
longer required.

Thanks all
Neil

On Mon, 29 Apr 2019 at 04:20, Phil Reid <preid@electromag.com.au> wrote:
>
> On 29/04/2019 6:05 am, Neil MacLeod wrote:
> > Hi Heiner
> >
> > 5.0.6 is the first kernel that does NOT require the workaround.
> >
> > In 5.0.6 the only obvious r8169 change (to my untrained eyes) is:
> >
> > https://github.com/torvalds/linux/commit/4951fc65d9153deded3d066ab371a61977c96e8a
> >
> > but reverting this change in addition to the workaround makes no
> > difference, the resulting kernel still resumes at 1000Mbps so I'm not
> > sure what other change in .5.0.6 might be responsible for this changed
> > behaviour. If you can think of anything I'll give it a try!
> >
> > Regards
> > Neil
>
> The symptom sounds very similar to a problem I had with 1G link only linking at 10M.
>
> Perhaps have a look at:
> net: phy: don't clear BMCR in genphy_soft_reset
>
> https://www.spinics.net/lists/netdev/msg559627.html
>
> Which looks to have been added in 5.0.6
> commit  fc8f36de77111bf925d19f347c21134542941a3c
>
>
> >
> > PS. A while ago (5 Dec 2018 to be precise!) I emailed you about the
> > ASPM issue which it looks like you may have fixed in 5.1-rc5[1].
> > Unfortunately I don't have this issue myself, and I've been trying to
> > get feedback from the bug reporter[2,3] "Matt Devo" without much
> > success but will confirm to you if/when he replies.
> >
> > 1, https://bugzilla.kernel.org/show_bug.cgi?id=202945
> > 2. https://forum.kodi.tv/showthread.php?tid=298462&pid=2845944#pid2845944
> > 3. https://forum.kodi.tv/showthread.php?tid=343069&pid=2850123#pid2850123
> >
> > On Sun, 28 Apr 2019 at 19:43, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>
> >> Interesting, thanks for your efforts! I submitted the patch removing
> >> the workaround because it seems now (at least since 5.1-rc1) we're fine.
> >>
> >> Heiner
> >>
> >> On 28.04.2019 20:40, Neil MacLeod wrote:
> >>> Hi Heiner
> >>>
> >>> I'd already kicked off a 5.0.2 build without the workaround and I've
> >>> tested that now, and it resumes at 10Mbps, so it may still be worth
> >>> identifying the exact 5.0.y version when it was fixed just in case
> >>> that provides some understanding of how it was fixed... I'll test the
> >>> remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
> >>> work and let you know what I find!
> >>>
> >>> Regards
> >>> Neil
> >>>
> >>> On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>>>
> >>>> Hi Neil,
> >>>>
> >>>> thanks for reporting back. Interesting, then the root cause of the
> >>>> issue seems to have been in a different corner. On my hardware
> >>>> I'm not able to reproduce the issue. It's not that relevant with which
> >>>> exact version the issue vanished. Based on your results I'll just
> >>>> remove the workaround on net-next (adding your Tested-by).
> >>>>
> >>>> Heiner
> >>>>
> >>>>
> >>>> On 28.04.2019 19:30, Neil MacLeod wrote:
> >>>>> Hi Heiner
> >>>>>
> >>>>> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
> >>>>> as in order to test your request I thought I would reproduce the issue
> >>>>> with plain 5.1-rc6 with the workaround removed, however without the
> >>>>> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
> >>>>>
> >>>>> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
> >>>>> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
> >>>>> I can no longer reproduce the issue when the workaround is removed.
> >>>>>
> >>>>> I also tested 5.0.10 without the workaround, and again 5.0.10 is
> >>>>> resuming correctly at 1000Mbps.
> >>>>>
> >>>>> I finally tested 4.19.23 without the workaround (the last iteration of
> >>>>> this kernel I published) and this does NOT resume correctly at
> >>>>> 1000Mbps (it resumes at 10Mbps).
> >>>>>
> >>>>> I'll test a few more iterations of 5.0.y to see if I can identify when
> >>>>> it was "fixed" but if you have any suggestions when it might have been
> >>>>> fixed I can try to confirm this that - currently it's somewhere
> >>>>> between 4.19.24 and 5.0.10!
> >>>>>
> >>>>> Regards
> >>>>> Neil
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> >>>>>>
> >>>>>> Hi Neil,
> >>>>>>
> >>>>>> you once reported the original issue resulting in this workaround.
> >>>>>> This workaround shouldn't be needed any longer, but I have no affected HW
> >>>>>> to test on. Do you have the option to apply the patch below to latest
> >>>>>> net-next and test link speed after resume from suspend?
> >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> >>>>>> That would be much appreciated.
> >>>>>>
> >>>>>> Heiner
> >>>>>>
> >>>>>> ----------------------------------------------------------------
> >>>>>>
> >>>>>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
> >>>>>> workaround shouldn't be needed any longer. However I don't have
> >>>>>> affected hardware so I can't test it.
> >>>>>>
> >>>>>> This was the bug report leading to the workaround:
> >>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
> >>>>>>
> >>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>>>> ---
> >>>>>>   drivers/net/ethernet/realtek/r8169.c | 8 --------
> >>>>>>   1 file changed, 8 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> >>>>>> index 383242df0..d4ec08e37 100644
> >>>>>> --- a/drivers/net/ethernet/realtek/r8169.c
> >>>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
> >>>>>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> >>>>>>          phy_speed_up(tp->phydev);
> >>>>>>
> >>>>>>          genphy_soft_reset(tp->phydev);
> >>>>>> -
> >>>>>> -       /* It was reported that several chips end up with 10MBit/Half on a
> >>>>>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
> >>>>>> -        * these chips doesn't properly start a renegotiation when soft-reset.
> >>>>>> -        * Explicitly requesting a renegotiation fixes this.
> >>>>>> -        */
> >>>>>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
> >>>>>> -               phy_restart_aneg(tp->phydev);
> >>>>>>   }
> >>>>>>
> >>>>>>   static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
> >>>>>> --
> >>>>>> 2.21.0
> >>>>>
> >>>>
> >>>
> >>
> >
> >
>
>
> --
> Regards
> Phil Reid
>
> ElectroMagnetic Imaging Technology Pty Ltd
> Development of Geophysical Instrumentation & Software
> www.electromag.com.au
>
> 3 The Avenue, Midland WA 6056, AUSTRALIA
> Ph: +61 8 9250 8100
> Fax: +61 8 9250 7100
> Email: preid@electromag.com.au

Re: Testing of r8169 workaround removal

[Neil MacLeod]

Hi, just re-sending this email as it doesn't appear to have arrived at
the mailing list.

Phil

Many thanks - you've nailed it!

Reverting the workaround from Heiner, and also
fc8f36de77111bf925d19f347c2113, resulted in 5.0.6 syncing at 10Mbps
after resuming from S3 instead of the 1000Mbps it syncs at boot.

Hopefully this is helpful in understanding why the workaround is no
longer required.

Thanks all
Neil

On Mon, 29 Apr 2019 at 04:55, Neil MacLeod <neil@nmacleod.com> wrote:
>
> Phil
>
> Many thanks - you've nailed it!
>
> Reverting the workaround from Heiner, and also
> fc8f36de77111bf925d19f347c2113, resulted in 5.0.6 syncing at 10Mbps
> after resuming from S3 instead of the 1000Mbps it syncs at boot.
>
> Hopefully this is helpful in understanding why the workaround is no
> longer required.
>
> Thanks all
> Neil
>
> On Mon, 29 Apr 2019 at 04:20, Phil Reid <preid@electromag.com.au> wrote:
> >
> > On 29/04/2019 6:05 am, Neil MacLeod wrote:
> > > Hi Heiner
> > >
> > > 5.0.6 is the first kernel that does NOT require the workaround.
> > >
> > > In 5.0.6 the only obvious r8169 change (to my untrained eyes) is:
> > >
> > > https://github.com/torvalds/linux/commit/4951fc65d9153deded3d066ab371a61977c96e8a
> > >
> > > but reverting this change in addition to the workaround makes no
> > > difference, the resulting kernel still resumes at 1000Mbps so I'm not
> > > sure what other change in .5.0.6 might be responsible for this changed
> > > behaviour. If you can think of anything I'll give it a try!
> > >
> > > Regards
> > > Neil
> >
> > The symptom sounds very similar to a problem I had with 1G link only linking at 10M.
> >
> > Perhaps have a look at:
> > net: phy: don't clear BMCR in genphy_soft_reset
> >
> > https://www.spinics.net/lists/netdev/msg559627.html
> >
> > Which looks to have been added in 5.0.6
> > commit  fc8f36de77111bf925d19f347c21134542941a3c
> >
> >
> > >
> > > PS. A while ago (5 Dec 2018 to be precise!) I emailed you about the
> > > ASPM issue which it looks like you may have fixed in 5.1-rc5[1].
> > > Unfortunately I don't have this issue myself, and I've been trying to
> > > get feedback from the bug reporter[2,3] "Matt Devo" without much
> > > success but will confirm to you if/when he replies.
> > >
> > > 1, https://bugzilla.kernel.org/show_bug.cgi?id=202945
> > > 2. https://forum.kodi.tv/showthread.php?tid=298462&pid=2845944#pid2845944
> > > 3. https://forum.kodi.tv/showthread.php?tid=343069&pid=2850123#pid2850123
> > >
> > > On Sun, 28 Apr 2019 at 19:43, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> > >>
> > >> Interesting, thanks for your efforts! I submitted the patch removing
> > >> the workaround because it seems now (at least since 5.1-rc1) we're fine.
> > >>
> > >> Heiner
> > >>
> > >> On 28.04.2019 20:40, Neil MacLeod wrote:
> > >>> Hi Heiner
> > >>>
> > >>> I'd already kicked off a 5.0.2 build without the workaround and I've
> > >>> tested that now, and it resumes at 10Mbps, so it may still be worth
> > >>> identifying the exact 5.0.y version when it was fixed just in case
> > >>> that provides some understanding of how it was fixed... I'll test the
> > >>> remaining kernels between 5.0.3 and 5.0.10 as that's not much extra
> > >>> work and let you know what I find!
> > >>>
> > >>> Regards
> > >>> Neil
> > >>>
> > >>> On Sun, 28 Apr 2019 at 18:39, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> > >>>>
> > >>>> Hi Neil,
> > >>>>
> > >>>> thanks for reporting back. Interesting, then the root cause of the
> > >>>> issue seems to have been in a different corner. On my hardware
> > >>>> I'm not able to reproduce the issue. It's not that relevant with which
> > >>>> exact version the issue vanished. Based on your results I'll just
> > >>>> remove the workaround on net-next (adding your Tested-by).
> > >>>>
> > >>>> Heiner
> > >>>>
> > >>>>
> > >>>> On 28.04.2019 19:30, Neil MacLeod wrote:
> > >>>>> Hi Heiner
> > >>>>>
> > >>>>> Do you know if this is already fixed in 5.1-rc6 (Linus Torvalds tree),
> > >>>>> as in order to test your request I thought I would reproduce the issue
> > >>>>> with plain 5.1-rc6 with the workaround removed, however without the
> > >>>>> workaround 5.1-rc6 is resuming correctly at 1000Mbps.
> > >>>>>
> > >>>>> I went back to 4.19-rc4 (which we know is brroken) and I can reproduce
> > >>>>> the issue with the PC (Revo 3700) resuming at 10Mbps, but with 5.1-rc6
> > >>>>> I can no longer reproduce the issue when the workaround is removed.
> > >>>>>
> > >>>>> I also tested 5.0.10 without the workaround, and again 5.0.10 is
> > >>>>> resuming correctly at 1000Mbps.
> > >>>>>
> > >>>>> I finally tested 4.19.23 without the workaround (the last iteration of
> > >>>>> this kernel I published) and this does NOT resume correctly at
> > >>>>> 1000Mbps (it resumes at 10Mbps).
> > >>>>>
> > >>>>> I'll test a few more iterations of 5.0.y to see if I can identify when
> > >>>>> it was "fixed" but if you have any suggestions when it might have been
> > >>>>> fixed I can try to confirm this that - currently it's somewhere
> > >>>>> between 4.19.24 and 5.0.10!
> > >>>>>
> > >>>>> Regards
> > >>>>> Neil
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Sun, 28 Apr 2019 at 14:33, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> > >>>>>>
> > >>>>>> Hi Neil,
> > >>>>>>
> > >>>>>> you once reported the original issue resulting in this workaround.
> > >>>>>> This workaround shouldn't be needed any longer, but I have no affected HW
> > >>>>>> to test on. Do you have the option to apply the patch below to latest
> > >>>>>> net-next and test link speed after resume from suspend?
> > >>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
> > >>>>>> That would be much appreciated.
> > >>>>>>
> > >>>>>> Heiner
> > >>>>>>
> > >>>>>> ----------------------------------------------------------------
> > >>>>>>
> > >>>>>> After 8c90b795e90f ("net: phy: improve genphy_soft_reset") this
> > >>>>>> workaround shouldn't be needed any longer. However I don't have
> > >>>>>> affected hardware so I can't test it.
> > >>>>>>
> > >>>>>> This was the bug report leading to the workaround:
> > >>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=201081
> > >>>>>>
> > >>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > >>>>>> ---
> > >>>>>>   drivers/net/ethernet/realtek/r8169.c | 8 --------
> > >>>>>>   1 file changed, 8 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> > >>>>>> index 383242df0..d4ec08e37 100644
> > >>>>>> --- a/drivers/net/ethernet/realtek/r8169.c
> > >>>>>> +++ b/drivers/net/ethernet/realtek/r8169.c
> > >>>>>> @@ -4083,14 +4083,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
> > >>>>>>          phy_speed_up(tp->phydev);
> > >>>>>>
> > >>>>>>          genphy_soft_reset(tp->phydev);
> > >>>>>> -
> > >>>>>> -       /* It was reported that several chips end up with 10MBit/Half on a
> > >>>>>> -        * 1GBit link after resuming from S3. For whatever reason the PHY on
> > >>>>>> -        * these chips doesn't properly start a renegotiation when soft-reset.
> > >>>>>> -        * Explicitly requesting a renegotiation fixes this.
> > >>>>>> -        */
> > >>>>>> -       if (tp->phydev->autoneg == AUTONEG_ENABLE)
> > >>>>>> -               phy_restart_aneg(tp->phydev);
> > >>>>>>   }
> > >>>>>>
> > >>>>>>   static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
> > >>>>>> --
> > >>>>>> 2.21.0
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> > >
> >
> >
> > --
> > Regards
> > Phil Reid
> >
> > ElectroMagnetic Imaging Technology Pty Ltd
> > Development of Geophysical Instrumentation & Software
> > www.electromag.com.au
> >
> > 3 The Avenue, Midland WA 6056, AUSTRALIA
> > Ph: +61 8 9250 8100
> > Fax: +61 8 9250 7100
> > Email: preid@electromag.com.au