[PATCH net-next 0/2] net: phy: remove parameter new_link from phy_mac_interrupt()

[PATCH net-next 0/2] net: phy: remove parameter new_link from phy_mac_interrupt()

[Heiner Kallweit]

I see two issues with parameter new_link:

1. It's not needed. See also phy_interrupt(), works w/o this parameter.
   phy_mac_interrupt sets the state to PHY_CHANGELINK and triggers the
   state machine which then calls phy_read_status. And phy_read_status
   updates the link state.

2. phy_mac_interrupt is used in interrupt context and getting the link
   state may sleep (at least when having to access the PHY registers
   via MDIO bus).

bcmgenet driver so far is the only user, therefore changing the API
has minimal impact.

Heiner Kallweit (2):
  net: phy: remove parameter new_link from phy_mac_interrupt()
  net: bcmgenet: remove parameter new_link from phy_mac_interrupt()

 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  7 ++++---
 drivers/net/phy/phy.c                          | 10 +++-------
 include/linux/phy.h                            |  2 +-
 3 files changed, 8 insertions(+), 11 deletions(-)

-- 
2.15.1

[PATCH net-next 1/2] net: phy: remove parameter new_link from phy_mac_interrupt()

[Heiner Kallweit]

I see two issues with parameter new_link:

1. It's not needed. See also phy_interrupt(), works w/o this parameter.
   phy_mac_interrupt sets the state to PHY_CHANGELINK and triggers the
   state machine which then calls phy_read_status. And phy_read_status
   updates the link state.

2. phy_mac_interrupt is used in interrupt context and getting the link
   state may sleep (at least when having to access the PHY registers
   via MDIO bus).

So let's remove it.

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

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 0c165ad1d..f3313a129 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1057,16 +1057,12 @@ void phy_state_machine(struct work_struct *work)
 /**
  * phy_mac_interrupt - MAC says the link has changed
  * @phydev: phy_device struct with changed link
- * @new_link: Link is Up/Down.
  *
- * Description: The MAC layer is able indicate there has been a change
- *   in the PHY link status. Set the new link status, and trigger the
- *   state machine, work a work queue.
+ * The MAC layer is able to indicate there has been a change in the PHY link
+ * status. Trigger the state machine and work a work queue.
  */
-void phy_mac_interrupt(struct phy_device *phydev, int new_link)
+void phy_mac_interrupt(struct phy_device *phydev)
 {
-	phydev->link = new_link;
-
 	/* Trigger a state machine change */
 	queue_work(system_power_efficient_wq, &phydev->phy_queue);
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 135aba5c3..47715a311 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -964,7 +964,7 @@ int phy_drivers_register(struct phy_driver *new_driver, int n,
 void phy_state_machine(struct work_struct *work);
 void phy_change(struct phy_device *phydev);
 void phy_change_work(struct work_struct *work);
-void phy_mac_interrupt(struct phy_device *phydev, int new_link);
+void phy_mac_interrupt(struct phy_device *phydev);
 void phy_start_machine(struct phy_device *phydev);
 void phy_stop_machine(struct phy_device *phydev);
 void phy_trigger_machine(struct phy_device *phydev, bool sync);
-- 
2.15.1

Re: [PATCH net-next 1/2] net: phy: remove parameter new_link from phy_mac_interrupt()

[Florian Fainelli]

On 01/10/2018 12:11 PM, Heiner Kallweit wrote:
> I see two issues with parameter new_link:
> 
> 1. It's not needed. See also phy_interrupt(), works w/o this parameter.
>    phy_mac_interrupt sets the state to PHY_CHANGELINK and triggers the
>    state machine which then calls phy_read_status. And phy_read_status
>    updates the link state.
> 
> 2. phy_mac_interrupt is used in interrupt context and getting the link
>    state may sleep (at least when having to access the PHY registers
>    via MDIO bus).
> 
> So let's remove it.

This looks fine in premise, but you really need to combine patch 1 and
to 2 to avoid a build failure.
-- 
Florian

[PATCH net-next 2/2] net: bcmgenet: remove parameter new_link from phy_mac_interrupt()

[Heiner Kallweit]

Reflect a phylib API change and remove second parameter from call to
phy_mac_interrupt(). Keep the current logic and set phydev->link,
although this may not be needed. However I don't have the hardware
and can't test this.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 77154f147..db97873cd 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2527,9 +2527,10 @@ static void bcmgenet_irq_task(struct work_struct *work)
 	spin_unlock_irq(&priv->lock);
 
 	/* Link UP/DOWN event */
-	if (status & UMAC_IRQ_LINK_EVENT)
-		phy_mac_interrupt(priv->dev->phydev,
-				  !!(status & UMAC_IRQ_LINK_UP));
+	if (status & UMAC_IRQ_LINK_EVENT) {
+		priv->dev->phydev->link = !!(status & UMAC_IRQ_LINK_UP);
+		phy_mac_interrupt(priv->dev->phydev);
+	}
 }
 
 /* bcmgenet_isr1: handle Rx and Tx priority queues */
-- 
2.15.1