[PATCH net-next 0/3] net: phy: further phylib simplifications after recent changes to the state machine

[PATCH net-next 0/3] net: phy: further phylib simplifications after recent changes to the state machine

[Heiner Kallweit]

After the recent changes to the state machine phylib can be further
simplified (w/o having to make any assumptions).

Heiner Kallweit (3):
  net: phy: don't set state PHY_CHANGELINK in phy_change
  net: phy: simplify phy_mac_interrupt and related functions
  net: phy: improve and inline phy_change

 drivers/net/phy/phy.c        | 67 ++++++++----------------------------
 drivers/net/phy/phy_device.c |  1 -
 include/linux/phy.h          | 10 ++----
 3 files changed, 17 insertions(+), 61 deletions(-)

-- 
2.19.1

[PATCH net-next 1/3] net: phy: don't set state PHY_CHANGELINK in phy_change

[Heiner Kallweit]

State PHY_CHANGELINK isn't needed here, we can call the state machine
directly. We just have to remove the check for phy_polling_mode() to
make this work also in interrupt mode. Removing this check doesn't
cause any overhead because when not polling the state machine is
called only if required by some event.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 8 --------
 include/linux/phy.h   | 7 ++-----
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8dac890f3..da41420df 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -738,11 +738,6 @@ static irqreturn_t phy_change(struct phy_device *phydev)
 				goto phy_err;
 	}
 
-	mutex_lock(&phydev->lock);
-	if ((PHY_RUNNING == phydev->state) || (PHY_NOLINK == phydev->state))
-		phydev->state = PHY_CHANGELINK;
-	mutex_unlock(&phydev->lock);
-
 	/* reschedule state queue work to run as soon as possible */
 	phy_trigger_machine(phydev);
 
@@ -946,9 +941,6 @@ void phy_state_machine(struct work_struct *work)
 		break;
 	case PHY_NOLINK:
 	case PHY_RUNNING:
-		if (!phy_polling_mode(phydev))
-			break;
-		/* fall through */
 	case PHY_CHANGELINK:
 	case PHY_RESUMING:
 		err = phy_check_link_status(phydev);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 59bb31ee1..7db07e69c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -298,7 +298,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  * - timer moves to NOLINK or RUNNING
  *
  * NOLINK: PHY is up, but not currently plugged in.
- * - If the timer notes that the link comes back, we move to RUNNING
+ * - irq or timer will set RUNNING if link comes back
  * - phy_stop moves to HALTED
  *
  * FORCING: PHY is being configured with forced settings
@@ -309,10 +309,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  *
  * RUNNING: PHY is currently up, running, and possibly sending
  * and/or receiving packets
- * - timer will set CHANGELINK if we're polling (this ensures the
- *   link state is polled every other cycle of this state machine,
- *   which makes it every other second)
- * - irq will set CHANGELINK
+ * - irq or timer will set NOLINK if link goes down
  * - phy_stop moves to HALTED
  *
  * CHANGELINK: PHY experienced a change in link state
-- 
2.19.1

[PATCH net-next 2/3] net: phy: simplify phy_mac_interrupt and related functions

[Heiner Kallweit]

When using phy_mac_interrupt() the irq number is set to
PHY_IGNORE_INTERRUPT, therefore phy_interrupt_is_valid() returns false.
As a result phy_change() effectively just calls phy_trigger_machine()
when called from phy_mac_interrupt() via phy_change_work(). So we can
call phy_trigger_machine() from phy_mac_interrupt() directly and
remove some now unneeded code.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index da41420df..ce1e8130a 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -750,18 +750,6 @@ static irqreturn_t phy_change(struct phy_device *phydev)
 	return IRQ_NONE;
 }
 
-/**
- * phy_change_work - Scheduled by the phy_mac_interrupt to handle PHY changes
- * @work: work_struct that describes the work to be done
- */
-void phy_change_work(struct work_struct *work)
-{
-	struct phy_device *phydev =
-		container_of(work, struct phy_device, phy_queue);
-
-	phy_change(phydev);
-}
-
 /**
  * phy_interrupt - PHY interrupt handler
  * @irq: interrupt line
@@ -1005,7 +993,7 @@ void phy_state_machine(struct work_struct *work)
 void phy_mac_interrupt(struct phy_device *phydev)
 {
 	/* Trigger a state machine change */
-	queue_work(system_power_efficient_wq, &phydev->phy_queue);
+	phy_trigger_machine(phydev);
 }
 EXPORT_SYMBOL(phy_mac_interrupt);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 00a46218c..0f56d408b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -587,7 +587,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id,
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
-	INIT_WORK(&dev->phy_queue, phy_change_work);
 
 	/* Request the appropriate module unconditionally; don't
 	 * bother trying to do so only if it isn't already loaded,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7db07e69c..17d1f6472 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -369,7 +369,6 @@ struct phy_c45_device_ids {
  * giving up on the current attempt at acquiring a link
  * irq: IRQ number of the PHY's interrupt (-1 if none)
  * phy_timer: The timer for handling the state machine
- * phy_queue: A work_queue for the phy_mac_interrupt
  * attached_dev: The attached enet driver's device instance ptr
  * adjust_link: Callback for the enet controller to respond to
  * changes in the link state.
@@ -454,7 +453,6 @@ struct phy_device {
 	void *priv;
 
 	/* Interrupt and Polling infrastructure */
-	struct work_struct phy_queue;
 	struct delayed_work state_queue;
 
 	struct mutex lock;
@@ -1029,7 +1027,6 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner);
 int phy_drivers_register(struct phy_driver *new_driver, int n,
 			 struct module *owner);
 void phy_state_machine(struct work_struct *work);
-void phy_change_work(struct work_struct *work);
 void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
-- 
2.19.1

[PATCH net-next 3/3] net: phy: improve and inline phy_change

[Heiner Kallweit]

Now that phy_mac_interrupt() doesn't call phy_change() any longer it's
called from phy_interrupt() only. Therefore phy_interrupt_is_valid()
returns true always and the check can be removed.
In case of PHY_HALTED phy_interrupt() bails out immediately,
therefore the second check for PHY_HALTED including the call to
phy_disable_interrupts() can be removed.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ce1e8130a..083977d2f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -722,41 +722,12 @@ static int phy_disable_interrupts(struct phy_device *phydev)
 	return phy_clear_interrupt(phydev);
 }
 
-/**
- * phy_change - Called by the phy_interrupt to handle PHY changes
- * @phydev: phy_device struct that interrupted
- */
-static irqreturn_t phy_change(struct phy_device *phydev)
-{
-	if (phy_interrupt_is_valid(phydev)) {
-		if (phydev->drv->did_interrupt &&
-		    !phydev->drv->did_interrupt(phydev))
-			return IRQ_NONE;
-
-		if (phydev->state == PHY_HALTED)
-			if (phy_disable_interrupts(phydev))
-				goto phy_err;
-	}
-
-	/* reschedule state queue work to run as soon as possible */
-	phy_trigger_machine(phydev);
-
-	if (phy_interrupt_is_valid(phydev) && phy_clear_interrupt(phydev))
-		goto phy_err;
-	return IRQ_HANDLED;
-
-phy_err:
-	phy_error(phydev);
-	return IRQ_NONE;
-}
-
 /**
  * phy_interrupt - PHY interrupt handler
  * @irq: interrupt line
  * @phy_dat: phy_device pointer
  *
- * Description: When a PHY interrupt occurs, the handler disables
- * interrupts, and uses phy_change to handle the interrupt.
+ * Description: Handle PHY interrupt
  */
 static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 {
@@ -765,7 +736,19 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	if (PHY_HALTED == phydev->state)
 		return IRQ_NONE;		/* It can't be ours.  */
 
-	return phy_change(phydev);
+	if (phydev->drv->did_interrupt && !phydev->drv->did_interrupt(phydev))
+		return IRQ_NONE;
+
+	/* reschedule state queue work to run as soon as possible */
+	phy_trigger_machine(phydev);
+
+	if (phy_clear_interrupt(phydev))
+		goto phy_err;
+	return IRQ_HANDLED;
+
+phy_err:
+	phy_error(phydev);
+	return IRQ_NONE;
 }
 
 /**
@@ -846,7 +829,7 @@ void phy_stop(struct phy_device *phydev)
 	phy_state_machine(&phydev->state_queue.work);
 
 	/* Cannot call flush_scheduled_work() here as desired because
-	 * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
+	 * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
 	 * will not reenable interrupts.
 	 */
 }
-- 
2.19.1

Re: [PATCH net-next 0/3] net: phy: further phylib simplifications after recent changes to the state machine

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Fri, 9 Nov 2018 18:54:49 +0100

> After the recent changes to the state machine phylib can be further
> simplified (w/o having to make any assumptions).

Nothing exemplifies understanding of a piece of code like a patch
series like this, nice work.

Series applied.