drivers: eth: phy_mii: Don't block system workqueue

Looping while waiting for auto-negotiation to complete can block the
system workqueue for several seconds.

Signed-off-by: Kevin ORourke <[email protected]>

Author: kevinior

drivers/ethernet/phy/phy_mii.c

@@ -31,16 +31,21 @@ struct phy_mii_dev_data {
 	phy_callback_t cb;
 	void *cb_data;
 	struct k_work_delayable monitor_work;
+	struct k_work_delayable autoneg_work;
 	struct phy_link_state state;
 	struct k_sem sem;
 	bool gigabit_supported;
+	uint32_t autoneg_timeout;
 };
 
 /* Offset to align capabilities bits of 1000BASE-T Control and Status regs */
 #define MII_1KSTSR_OFFSET 2
 
 #define MII_INVALID_PHY_ID UINT32_MAX
 
+/* How often to poll auto-negotiation status while waiting for it to complete */
+#define MII_AUTONEG_POLL_INTERVAL_MS 100
+
 static int phy_mii_get_link_state(const struct device *dev,
 				  struct phy_link_state *state);
 
@@ -145,13 +150,8 @@ static int update_link_state(const struct device *dev)
 	struct phy_mii_dev_data *const data = dev->data;
 	bool link_up;
 
-	uint16_t anar_reg = 0;
 	uint16_t bmcr_reg = 0;
 	uint16_t bmsr_reg = 0;
-	uint16_t anlpar_reg = 0;
-	uint16_t c1kt_reg = 0;
-	uint16_t s1kt_reg = 0;
-	uint32_t timeout = CONFIG_PHY_AUTONEG_TIMEOUT_MS / 100;
 
 	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
 		return -EIO;
@@ -178,11 +178,6 @@ static int update_link_state(const struct device *dev)
 	LOG_DBG("PHY (%d) Starting MII PHY auto-negotiate sequence",
 		cfg->phy_addr);
 
-	/* Read PHY default advertising parameters */
-	if (phy_mii_reg_read(dev, MII_ANAR, &anar_reg) < 0) {
-		return -EIO;
-	}
-
 	/* Configure and start auto-negotiation process */
 	if (phy_mii_reg_read(dev, MII_BMCR, &bmcr_reg) < 0) {
 		return -EIO;
@@ -195,32 +190,51 @@ static int update_link_state(const struct device *dev)
 		return -EIO;
 	}
 
-	/* Wait for the auto-negotiation process to complete */
-	do {
-		if (timeout-- == 0U) {
-			LOG_DBG("PHY (%d) auto-negotiate timedout",
-				cfg->phy_addr);
-			return -ETIMEDOUT;
-		}
+	/* We have to wait for the auto-negotiation process to complete */
+	data->autoneg_timeout = CONFIG_PHY_AUTONEG_TIMEOUT_MS / MII_AUTONEG_POLL_INTERVAL_MS;
+	return -EINPROGRESS;
+}
 
-		k_sleep(K_MSEC(100));
+static int check_autonegotiation_completion(const struct device *dev)
+{
+	const struct phy_mii_dev_config *const cfg = dev->config;
+	struct phy_mii_dev_data *const data = dev->data;
 
-		/* On some PHY chips, the BMSR bits are latched, so the first read may
-		 * show incorrect status. A second read ensures correct values.
-		 */
-		if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
-			return -EIO;
-		}
+	uint16_t anar_reg = 0;
+	uint16_t bmsr_reg = 0;
+	uint16_t anlpar_reg = 0;
+	uint16_t c1kt_reg = 0;
+	uint16_t s1kt_reg = 0;
 
-		/* Second read, clears the latched bits and gives the correct status */
-		if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
-			return -EIO;
+	/* On some PHY chips, the BMSR bits are latched, so the first read may
+	 * show incorrect status. A second read ensures correct values.
+	 */
+	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
+		return -EIO;
+	}
+
+	/* Second read, clears the latched bits and gives the correct status */
+	if (phy_mii_reg_read(dev, MII_BMSR, &bmsr_reg) < 0) {
+		return -EIO;
+	}
+
+	if (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE)) {
+		if (data->autoneg_timeout-- == 0U) {
+			LOG_DBG("PHY (%d) auto-negotiate timedout",
+				    cfg->phy_addr);
+			return -ETIMEDOUT;
 		}
-	} while (!(bmsr_reg & MII_BMSR_AUTONEG_COMPLETE));
+		return -EINPROGRESS;
+	}
 
 	LOG_DBG("PHY (%d) auto-negotiate sequence completed",
 		cfg->phy_addr);
 
+	/* Read PHY default advertising parameters */
+	if (phy_mii_reg_read(dev, MII_ANAR, &anar_reg) < 0) {
+		return -EIO;
+	}
+
 	/** Read peer device capability */
 	if (phy_mii_reg_read(dev, MII_ANLPAR, &anlpar_reg) < 0) {
 		return -EIO;
@@ -283,7 +297,12 @@ static void monitor_work_handler(struct k_work *work)
 	const struct device *dev = data->dev;
 	int rc;
 
-	k_sem_take(&data->sem, K_FOREVER);
+	if (k_sem_take(&data->sem, K_NO_WAIT) != 0) {
+		/* Try again soon */
+		k_work_reschedule(&data->monitor_work,
+				  K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+		return;
+	}
 
 	rc = update_link_state(dev);
 
@@ -294,9 +313,50 @@ static void monitor_work_handler(struct k_work *work)
 		invoke_link_cb(dev);
 	}
 
-	/* Submit delayed work */
-	k_work_reschedule(&data->monitor_work,
-			  K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	if (rc == -EINPROGRESS) {
+		/* Check for autonegotiation completion */
+		k_work_reschedule(&data->autoneg_work,
+				  K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+	} else {
+		/* Submit delayed work */
+		k_work_reschedule(&data->monitor_work,
+				  K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	}
+}
+
+static void autoneg_work_handler(struct k_work *work)
+{
+	struct k_work_delayable *dwork = k_work_delayable_from_work(work);
+	struct phy_mii_dev_data *const data =
+		CONTAINER_OF(dwork, struct phy_mii_dev_data, autoneg_work);
+	const struct device *dev = data->dev;
+	int rc;
+
+	if (k_sem_take(&data->sem, K_NO_WAIT) != 0) {
+		/* Try again soon */
+		k_work_reschedule(&data->autoneg_work,
+				  K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+		return;
+	}
+
+	rc = check_autonegotiation_completion(dev);
+
+	k_sem_give(&data->sem);
+
+	/* If link state has changed and a callback is set, invoke callback */
+	if (rc == 0) {
+		invoke_link_cb(dev);
+	}
+
+	if (rc == -EINPROGRESS) {
+		/* Check again soon */
+		k_work_reschedule(&data->autoneg_work,
+				  K_MSEC(MII_AUTONEG_POLL_INTERVAL_MS));
+	} else {
+		/* Schedule the next monitoring call */
+		k_work_reschedule(&data->monitor_work,
+				  K_MSEC(CONFIG_PHY_MONITOR_PERIOD));
+	}
 }
 
 static int phy_mii_read(const struct device *dev, uint16_t reg_addr,
@@ -479,6 +539,8 @@ static int phy_mii_initialize(const struct device *dev)
 
 		k_work_init_delayable(&data->monitor_work,
 					monitor_work_handler);
+		k_work_init_delayable(&data->autoneg_work,
+					autoneg_work_handler);
 
 		monitor_work_handler(&data->monitor_work.work);
 	}