[PATCH net-next v2 0/4] net: phy: improve starting PHY

[PATCH net-next v2 0/4] net: phy: improve starting PHY

[Heiner Kallweit]

This patch series improves few aspects of starting the PHY.

v2:
- improve a warning in patch 4

Heiner Kallweit (4):
  net: phy: start state machine in phy_start only
  net: phy: warn if phy_start is called from invalid state
  net: phy: start interrupts in phy_start
  net: phy: change phy_start_interrupts to phy_request_interrupt

 drivers/net/phy/phy.c        | 64 ++++++++++++++++++------------------
 drivers/net/phy/phy_device.c |  5 ++-
 drivers/net/phy/phylink.c    |  5 ++-
 include/linux/phy.h          |  2 +-
 4 files changed, 37 insertions(+), 39 deletions(-)

-- 
2.20.1

[PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Heiner Kallweit]

The state machine is a no-op before phy_start() has been called.
Therefore let's enable it in phy_start() only. In phy_start()
let's call phy_start_machine() instead of phy_trigger_machine().
phy_start_machine is an alias for phy_trigger_machine but it makes
clearer that we start the state machine here instead of just
triggering a run.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 1 -
 drivers/net/phy/phylink.c    | 1 -
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 745a705a5..3df6aadc5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -883,7 +883,7 @@ void phy_start(struct phy_device *phydev)
 	}
 	mutex_unlock(&phydev->lock);
 
-	phy_trigger_machine(phydev);
+	phy_start_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e0ceecbed..3e284d596 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -944,7 +944,6 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	phy_start_machine(phydev);
 	if (phydev->irq > 0)
 		phy_start_interrupts(phydev);
 
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e7becc737..e9b8f1037 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -679,7 +679,6 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising);
 
-	phy_start_machine(phy);
 	if (phy->irq > 0)
 		phy_start_interrupts(phy);
 
-- 
2.20.1

[PATCH net-next v2 2/4] net: phy: warn if phy_start is called from invalid state

[Heiner Kallweit]

phy_start() should be called from states PHY_READY or PHY_HALTED only.
Check for this to detect misbehaving drivers. Also the state machine
should be started only when being called from one of the valid states.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3df6aadc5..fd928979b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
+	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
+		WARN(1, "called from state %s\n",
+		     phy_state_to_str(phydev->state));
+		goto out;
+	}
+
 	switch (phydev->state) {
 	case PHY_READY:
 		phydev->state = PHY_UP;
+		phy_start_machine(phydev);
 		break;
 	case PHY_HALTED:
 		/* if phy was suspended, bring the physical link up again */
@@ -877,13 +884,13 @@ void phy_start(struct phy_device *phydev)
 		}
 
 		phydev->state = PHY_RESUMING;
+		phy_start_machine(phydev);
 		break;
 	default:
 		break;
 	}
+out:
 	mutex_unlock(&phydev->lock);
-
-	phy_start_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);
 
-- 
2.20.1

[PATCH net-next v2 3/4] net: phy: start interrupts in phy_start

[Heiner Kallweit]

Interrupts don't have to be enabled before calling phy_start().
Therefore let's enable them in phy_start(). In a subsequent step
we'll remove enabling interrupts from phy_connect_direct().

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index fd928979b..30ba650bb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -857,7 +857,7 @@ EXPORT_SYMBOL(phy_stop);
  */
 void phy_start(struct phy_device *phydev)
 {
-	int err = 0;
+	int err;
 
 	mutex_lock(&phydev->lock);
 
@@ -867,28 +867,22 @@ void phy_start(struct phy_device *phydev)
 		goto out;
 	}
 
-	switch (phydev->state) {
-	case PHY_READY:
-		phydev->state = PHY_UP;
-		phy_start_machine(phydev);
-		break;
-	case PHY_HALTED:
-		/* if phy was suspended, bring the physical link up again */
-		__phy_resume(phydev);
+	/* if phy was suspended, bring the physical link up again */
+	__phy_resume(phydev);
 
-		/* make sure interrupts are re-enabled for the PHY */
-		if (phy_interrupt_is_valid(phydev)) {
-			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
-		}
+	/* make sure interrupts are enabled for the PHY */
+	if (phy_interrupt_is_valid(phydev)) {
+		err = phy_enable_interrupts(phydev);
+		if (err < 0)
+			goto out;
+	}
 
+	if (phydev->state == PHY_READY)
+		phydev->state = PHY_UP;
+	else
 		phydev->state = PHY_RESUMING;
-		phy_start_machine(phydev);
-		break;
-	default:
-		break;
-	}
+
+	phy_start_machine(phydev);
 out:
 	mutex_unlock(&phydev->lock);
 }
-- 
2.20.1

[PATCH net-next v2 4/4] net: phy: change phy_start_interrupts to phy_request_interrupt

[Heiner Kallweit]

Now that we enable the interrupts in phy_start() we don't have to do it
before. Therefore remove enabling interrupts from phy_start_interrupts()
and rename this function to reflect the changed functionality.

v2:
- improve warning to clearly state that we fall back to polling

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c        | 23 +++++++++++------------
 drivers/net/phy/phy_device.c |  4 ++--
 drivers/net/phy/phylink.c    |  4 ++--
 include/linux/phy.h          |  2 +-
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 30ba650bb..58ce3dac2 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -790,28 +790,27 @@ static int phy_enable_interrupts(struct phy_device *phydev)
 }
 
 /**
- * phy_start_interrupts - request and enable interrupts for a PHY device
+ * phy_request_interrupt - request interrupt for a PHY device
  * @phydev: target phy_device struct
  *
  * Description: Request the interrupt for the given PHY.
  *   If this fails, then we set irq to PHY_POLL.
- *   Otherwise, we enable the interrupts in the PHY.
  *   This should only be called with a valid IRQ number.
- *   Returns 0 on success or < 0 on error.
  */
-int phy_start_interrupts(struct phy_device *phydev)
+void phy_request_interrupt(struct phy_device *phydev)
 {
-	if (request_threaded_irq(phydev->irq, NULL, phy_interrupt,
-				 IRQF_ONESHOT | IRQF_SHARED,
-				 phydev_name(phydev), phydev) < 0) {
-		phydev_warn(phydev, "Can't get IRQ %d\n", phydev->irq);
+	int err;
+
+	err = request_threaded_irq(phydev->irq, NULL, phy_interrupt,
+				   IRQF_ONESHOT | IRQF_SHARED,
+				   phydev_name(phydev), phydev);
+	if (err) {
+		phydev_warn(phydev, "Error %d requesting IRQ %d, falling back to polling\n",
+			    err, phydev->irq);
 		phydev->irq = PHY_POLL;
-		return 0;
 	}
-
-	return phy_enable_interrupts(phydev);
 }
-EXPORT_SYMBOL(phy_start_interrupts);
+EXPORT_SYMBOL(phy_request_interrupt);
 
 /**
  * phy_stop - Bring down the PHY link, and stop checking the status
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e284d596..dd3a2302f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -944,8 +944,8 @@ int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
 		return rc;
 
 	phy_prepare_link(phydev, handler);
-	if (phydev->irq > 0)
-		phy_start_interrupts(phydev);
+	if (phy_interrupt_is_valid(phydev))
+		phy_request_interrupt(phydev);
 
 	return 0;
 }
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e9b8f1037..1ac832ba0 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -679,8 +679,8 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, pl->supported,
 		   __ETHTOOL_LINK_MODE_MASK_NBITS, phy->advertising);
 
-	if (phy->irq > 0)
-		phy_start_interrupts(phy);
+	if (phy_interrupt_is_valid(phy))
+		phy_request_interrupt(phy);
 
 	return 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 1fa7c367b..d0055adbc 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1045,7 +1045,7 @@ void phy_ethtool_ksettings_get(struct phy_device *phydev,
 int phy_ethtool_ksettings_set(struct phy_device *phydev,
 			      const struct ethtool_link_ksettings *cmd);
 int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd);
-int phy_start_interrupts(struct phy_device *phydev);
+void phy_request_interrupt(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
 int phy_set_max_speed(struct phy_device *phydev, u32 max_speed);
 void phy_remove_link_mode(struct phy_device *phydev, u32 link_mode);
-- 
2.20.1

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Andrew Lunn]

On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote:
> The state machine is a no-op before phy_start() has been called.
> Therefore let's enable it in phy_start() only. In phy_start()
> let's call phy_start_machine() instead of phy_trigger_machine().
> phy_start_machine is an alias for phy_trigger_machine but it makes
> clearer that we start the state machine here instead of just
> triggering a run.

Hi Heiner

Documentation/networking/phy.txt has a section "Doing it all yourself"
It would be good to review that, and make sure that documentation is
still valid. I'm not sure any MAC driver actually does do it all
itself. So it might be worth reviewing the whole document and making
updates to remove parts of the text.

	Andrew
 

Re: [PATCH net-next v2 2/4] net: phy: warn if phy_start is called from invalid state

[Andrew Lunn]

On Sun, Jan 20, 2019 at 10:02:13AM +0100, Heiner Kallweit wrote:
> phy_start() should be called from states PHY_READY or PHY_HALTED only.
> Check for this to detect misbehaving drivers. Also the state machine
> should be started only when being called from one of the valid states.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3df6aadc5..fd928979b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev)
>  
>  	mutex_lock(&phydev->lock);
>  
> +	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
> +		WARN(1, "called from state %s\n",
> +		     phy_state_to_str(phydev->state));
> +		goto out;
> +	}

Hi Heiner

Warning is good. But jumping to out i'm not so sure about. Drivers
which are 'broken' work well enough that users don't know they are
broken. But jumping to out is going to really break them. It seems
better to have the kernel only warn for one cycle so we find out about
such drivers and fix them, and later add the goto out.

     Andrew

Re: [PATCH net-next v2 2/4] net: phy: warn if phy_start is called from invalid state

[Heiner Kallweit]

On 21.01.2019 17:40, Andrew Lunn wrote:
> On Sun, Jan 20, 2019 at 10:02:13AM +0100, Heiner Kallweit wrote:
>> phy_start() should be called from states PHY_READY or PHY_HALTED only.
>> Check for this to detect misbehaving drivers. Also the state machine
>> should be started only when being called from one of the valid states.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 3df6aadc5..fd928979b 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -861,9 +861,16 @@ void phy_start(struct phy_device *phydev)
>>  
>>  	mutex_lock(&phydev->lock);
>>  
>> +	if (phydev->state != PHY_READY && phydev->state != PHY_HALTED) {
>> +		WARN(1, "called from state %s\n",
>> +		     phy_state_to_str(phydev->state));
>> +		goto out;
>> +	}
> 
> Hi Heiner
> 
> Warning is good. But jumping to out i'm not so sure about. Drivers
> which are 'broken' work well enough that users don't know they are
> broken. But jumping to out is going to really break them. It seems
> better to have the kernel only warn for one cycle so we find out about
> such drivers and fix them, and later add the goto out.
> 
For all invalid states phy_start() basically was a no-op. All it did was
triggering a state machine run, but for all "running" states the poll
loop was active anyway. And if called from PHY_DOWN, the state machine
does nothing. Therefore I see no scenario where jumping to out would
break anything.

>      Andrew
> 
Heiner

Re: [PATCH net-next v2 2/4] net: phy: warn if phy_start is called from invalid state

[Andrew Lunn]

> For all invalid states phy_start() basically was a no-op. All it did was
> triggering a state machine run, but for all "running" states the poll
> loop was active anyway. And if called from PHY_DOWN, the state machine
> does nothing. Therefore I see no scenario where jumping to out would
> break anything.

Hi Heiner

It is useful to put this sort of analysis in the commit message. You
then won't get people like me asking about it.

     Andrew

Re: [PATCH net-next v2 2/4] net: phy: warn if phy_start is called from invalid state

[Heiner Kallweit]

On 21.01.2019 19:29, Andrew Lunn wrote:
>> For all invalid states phy_start() basically was a no-op. All it did was
>> triggering a state machine run, but for all "running" states the poll
>> loop was active anyway. And if called from PHY_DOWN, the state machine
>> does nothing. Therefore I see no scenario where jumping to out would
>> break anything.
> 
> Hi Heiner
> 
> It is useful to put this sort of analysis in the commit message. You
> then won't get people like me asking about it.
> 
Will add this to my "best practices" list ;)

>      Andrew
> 
Heiner

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Heiner Kallweit]

On 21.01.2019 17:35, Andrew Lunn wrote:
> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote:
>> The state machine is a no-op before phy_start() has been called.
>> Therefore let's enable it in phy_start() only. In phy_start()
>> let's call phy_start_machine() instead of phy_trigger_machine().
>> phy_start_machine is an alias for phy_trigger_machine but it makes
>> clearer that we start the state machine here instead of just
>> triggering a run.
> 
> Hi Heiner
> 
> Documentation/networking/phy.txt has a section "Doing it all yourself"
> It would be good to review that, and make sure that documentation is
> still valid. I'm not sure any MAC driver actually does do it all
> itself. So it might be worth reviewing the whole document and making
> updates to remove parts of the text.
> 
Right. I figured out that I have update phy.txt anyway because I
recently removed phy_stop_interrupts which is referenced in the
documentation. OK if we leave the patch series as is and I submit
the documentation update as a separate patch?

> 	Andrew
>  
> 
Heiner

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Andrew Lunn]

> Right. I figured out that I have update phy.txt anyway because I
> recently removed phy_stop_interrupts which is referenced in the
> documentation. OK if we leave the patch series as is and I submit
> the documentation update as a separate patch?

Hi Heiner

Fixing the documentation separately is O.K. It might also be a good
time to convert it to rst.

     Andrew

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Heiner Kallweit]

On 21.01.2019 19:42, Andrew Lunn wrote:
>> Right. I figured out that I have update phy.txt anyway because I
>> recently removed phy_stop_interrupts which is referenced in the
>> documentation. OK if we leave the patch series as is and I submit
>> the documentation update as a separate patch?
> 
> Hi Heiner
> 
> Fixing the documentation separately is O.K. It might also be a good
> time to convert it to rst.
> 
Thanks for the hint regarding rst. I converted the document and at
least restview is (syntactically) happy with it. Are you aware of
any checker for kernel rst documentation?

The link to the 802.3 standard is dead. On the official IEEE website
the standards are available for significant money only. Not sure
whether any other link to this standard is legal with regard to
copyright.

>      Andrew
> 
Heiner

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Andrew Lunn]

On Mon, Jan 21, 2019 at 10:52:48PM +0100, Heiner Kallweit wrote:
> On 21.01.2019 19:42, Andrew Lunn wrote:
> >> Right. I figured out that I have update phy.txt anyway because I
> >> recently removed phy_stop_interrupts which is referenced in the
> >> documentation. OK if we leave the patch series as is and I submit
> >> the documentation update as a separate patch?
> > 
> > Hi Heiner
> > 
> > Fixing the documentation separately is O.K. It might also be a good
> > time to convert it to rst.
> > 
> Thanks for the hint regarding rst. I converted the document and at
> least restview is (syntactically) happy with it. Are you aware of
> any checker for kernel rst documentation?

Hi Heiner

Do you try make htmldocs? It might need linking into the documentation
tree for that to work.

> The link to the 802.3 standard is dead. On the official IEEE website
> the standards are available for significant money only. Not sure
> whether any other link to this standard is legal with regard to
> copyright.

I think it should still be available as part of IEEE GET Program.  You
probably cannot give a direct link to it, but maybe a link to the
program could be given?

	Andrew

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Lendacky, Thomas]

On 1/21/19 12:36 PM, Heiner Kallweit wrote:
> On 21.01.2019 17:35, Andrew Lunn wrote:
>> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote:
>>> The state machine is a no-op before phy_start() has been called.
>>> Therefore let's enable it in phy_start() only. In phy_start()
>>> let's call phy_start_machine() instead of phy_trigger_machine().
>>> phy_start_machine is an alias for phy_trigger_machine but it makes
>>> clearer that we start the state machine here instead of just
>>> triggering a run.
>>
>> Hi Heiner
>>
>> Documentation/networking/phy.txt has a section "Doing it all yourself"
>> It would be good to review that, and make sure that documentation is
>> still valid. I'm not sure any MAC driver actually does do it all
>> itself. So it might be worth reviewing the whole document and making
>> updates to remove parts of the text.
>>
> Right. I figured out that I have update phy.txt anyway because I
> recently removed phy_stop_interrupts which is referenced in the
> documentation. OK if we leave the patch series as is and I submit
> the documentation update as a separate patch?

I think you need to be careful here and not break what is allowed in the
"Doing it all yourself" section. The amd-xgbe driver makes use of this
functionality and does not use phy_start()/phy_stop(). Specifically, it
does:
  get_phy_device();
  phy_device_register();
  phy_attach_direct();

At which point it uses phy_start_aneg(), phy_read(), phy_write(),
phy_read_status() and phy_aneg_done().

I'm not sure what other drivers out there that make use of this support
within phylib.

Btw, I did notice this revert that was applied that eliminated a warning
that I started seeing in 5.0, so that is good:
  d9f903f6af3d ("net: phy: fix too strict check in phy_start_aneg")

Thanks,
Tom

> 
>> 	Andrew
>>  
>>
> Heiner
> 

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[Heiner Kallweit]

On 22.01.2019 15:46, Lendacky, Thomas wrote:
> On 1/21/19 12:36 PM, Heiner Kallweit wrote:
>> On 21.01.2019 17:35, Andrew Lunn wrote:
>>> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote:
>>>> The state machine is a no-op before phy_start() has been called.
>>>> Therefore let's enable it in phy_start() only. In phy_start()
>>>> let's call phy_start_machine() instead of phy_trigger_machine().
>>>> phy_start_machine is an alias for phy_trigger_machine but it makes
>>>> clearer that we start the state machine here instead of just
>>>> triggering a run.
>>>
>>> Hi Heiner
>>>
>>> Documentation/networking/phy.txt has a section "Doing it all yourself"
>>> It would be good to review that, and make sure that documentation is
>>> still valid. I'm not sure any MAC driver actually does do it all
>>> itself. So it might be worth reviewing the whole document and making
>>> updates to remove parts of the text.
>>>
>> Right. I figured out that I have update phy.txt anyway because I
>> recently removed phy_stop_interrupts which is referenced in the
>> documentation. OK if we leave the patch series as is and I submit
>> the documentation update as a separate patch?
> 
> I think you need to be careful here and not break what is allowed in the
> "Doing it all yourself" section. The amd-xgbe driver makes use of this
> functionality and does not use phy_start()/phy_stop(). Specifically, it
> does:
>   get_phy_device();
>   phy_device_register();
>   phy_attach_direct();
> 
> At which point it uses phy_start_aneg(), phy_read(), phy_write(),
> phy_read_status() and phy_aneg_done().
> 
Thanks for the hint, Tom. I *think* the changes should be safe.
However, if AMD has a regression test suite I'd appreciate if you could
test the changes upfront or once they reach net-next.


> I'm not sure what other drivers out there that make use of this support
> within phylib.
> 
> Btw, I did notice this revert that was applied that eliminated a warning
> that I started seeing in 5.0, so that is good:
>   d9f903f6af3d ("net: phy: fix too strict check in phy_start_aneg")
> 
> Thanks,
> Tom
> 
>>
>>> 	Andrew
>>>  
>>>
>> Heiner
>>

Re: [PATCH net-next v2 0/4] net: phy: improve starting PHY

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 20 Jan 2019 09:59:46 +0100

> This patch series improves few aspects of starting the PHY.
> 
> v2:
> - improve a warning in patch 4

Please update the commit message as per Andrew's feedback for patch #2.

You might as well do this while we wait for the AMD testing.

Thanks!

Re: [PATCH net-next v2 1/4] net: phy: start state machine in phy_start only

[S-k, Shyam-sundar]

Hi Heiner

On 1/23/2019 12:39 AM, Heiner Kallweit wrote:
> On 22.01.2019 15:46, Lendacky, Thomas wrote:
>> On 1/21/19 12:36 PM, Heiner Kallweit wrote:
>>> On 21.01.2019 17:35, Andrew Lunn wrote:
>>>> On Sun, Jan 20, 2019 at 10:01:15AM +0100, Heiner Kallweit wrote:
>>>>> The state machine is a no-op before phy_start() has been called.
>>>>> Therefore let's enable it in phy_start() only. In phy_start()
>>>>> let's call phy_start_machine() instead of phy_trigger_machine().
>>>>> phy_start_machine is an alias for phy_trigger_machine but it makes
>>>>> clearer that we start the state machine here instead of just
>>>>> triggering a run.
>>>> Hi Heiner
>>>>
>>>> Documentation/networking/phy.txt has a section "Doing it all yourself"
>>>> It would be good to review that, and make sure that documentation is
>>>> still valid. I'm not sure any MAC driver actually does do it all
>>>> itself. So it might be worth reviewing the whole document and making
>>>> updates to remove parts of the text.
>>>>
>>> Right. I figured out that I have update phy.txt anyway because I
>>> recently removed phy_stop_interrupts which is referenced in the
>>> documentation. OK if we leave the patch series as is and I submit
>>> the documentation update as a separate patch?
>> I think you need to be careful here and not break what is allowed in the
>> "Doing it all yourself" section. The amd-xgbe driver makes use of this
>> functionality and does not use phy_start()/phy_stop(). Specifically, it
>> does:
>>   get_phy_device();
>>   phy_device_register();
>>   phy_attach_direct();
>>
>> At which point it uses phy_start_aneg(), phy_read(), phy_write(),
>> phy_read_status() and phy_aneg_done().
>>
> Thanks for the hint, Tom. I *think* the changes should be safe.
> However, if AMD has a regression test suite I'd appreciate if you could
> test the changes upfront or once they reach net-next.
>
Can you please cc: on your changes so you I can verify them before they get pushed upstream?