| Message ID | 90f63d2a-e357-c1cb-0b11-09f57b44e0fb@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: improve starting PHY | expand |
On January 19, 2019 3:30:05 AM PST, Heiner Kallweit <hkallweit1@gmail.com> wrote: >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. > >Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >--- >+ * 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); > phydev->irq = PHY_POLL; >- return 0; > } We should propagate the return code here and/or indicate we are falling back to polling since may not be desired. An use case that should be considered is probe deferral for instance.
On 20.01.2019 00:43, Florian Fainelli wrote: > > > On January 19, 2019 3:30:05 AM PST, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> 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. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- > >> + * 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); >> phydev->irq = PHY_POLL; >> - return 0; >> } > > We should propagate the return code here and/or indicate we are falling back to polling since may not be desired. An use case that should be considered is probe deferral for instance. > I kept the current behavior to basically ignore the error and just warn and fall back to polling. Seems that phylib has behaved this way forever. If we come to the conclusion that changing this behavior makes sense, shouldn't we do it separately? Also we may break systems relying on the fallback to polling. But something we can easily agree on is to extend the warning to state that we fall back to polling.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 30ba650bb..7bbe8bf8a 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -790,28 +790,23 @@ 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); 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);
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. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy.c | 11 +++-------- drivers/net/phy/phy_device.c | 4 ++-- drivers/net/phy/phylink.c | 4 ++-- include/linux/phy.h | 2 +- 4 files changed, 8 insertions(+), 13 deletions(-)