| Message ID | 20210318142356.30702-1-michael@walle.cc (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: at803x: remove at803x_aneg_done() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 18.03.2021 15:23, Michael Walle wrote: > at803x_aneg_done() is pretty much dead code since the patch series > "net: phy: improve and simplify phylib state machine" [1]. Remove it. > Well, it's not dead, it's resting .. There are few places where phy_aneg_done() is used. So you would need to explain: - why these users can't be used with this PHY driver - or why the aneg_done callback isn't needed here and the genphy_aneg_done() fallback is sufficient > [1] https://lore.kernel.org/netdev/922c223b-7bc0-e0ec-345d-2034b796af91@gmail.com/ > > Suggested-by: Vladimir Oltean <olteanv@gmail.com> > Signed-off-by: Michael Walle <michael@walle.cc> > --- > drivers/net/phy/at803x.c | 31 ------------------------------- > 1 file changed, 31 deletions(-) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index c2aa4c92edde..d7799beb811c 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -751,36 +751,6 @@ static void at803x_link_change_notify(struct phy_device *phydev) > } > } > > -static int at803x_aneg_done(struct phy_device *phydev) > -{ > - int ccr; > - > - int aneg_done = genphy_aneg_done(phydev); > - if (aneg_done != BMSR_ANEGCOMPLETE) > - return aneg_done; > - > - /* > - * in SGMII mode, if copper side autoneg is successful, > - * also check SGMII side autoneg result > - */ > - ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); > - if ((ccr & AT803X_MODE_CFG_MASK) != AT803X_MODE_CFG_SGMII) > - return aneg_done; > - > - /* switch to SGMII/fiber page */ > - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); > - > - /* check if the SGMII link is OK. */ > - if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) { > - phydev_warn(phydev, "803x_aneg_done: SGMII link is not ok\n"); > - aneg_done = 0; > - } > - /* switch back to copper page */ > - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL); > - > - return aneg_done; > -} > - > static int at803x_read_status(struct phy_device *phydev) > { > int ss, err, old_link = phydev->link; > @@ -1198,7 +1168,6 @@ static struct phy_driver at803x_driver[] = { > .resume = at803x_resume, > /* PHY_GBIT_FEATURES */ > .read_status = at803x_read_status, > - .aneg_done = at803x_aneg_done, > .config_intr = &at803x_config_intr, > .handle_interrupt = at803x_handle_interrupt, > .get_tunable = at803x_get_tunable, >
On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: > On 18.03.2021 15:23, Michael Walle wrote: > > at803x_aneg_done() is pretty much dead code since the patch series > > "net: phy: improve and simplify phylib state machine" [1]. Remove it. > > > > Well, it's not dead, it's resting .. There are few places where > phy_aneg_done() is used. So you would need to explain: > - why these users can't be used with this PHY driver > - or why the aneg_done callback isn't needed here and the > genphy_aneg_done() fallback is sufficient The piece of code that Michael is removing keeps the aneg reporting as "not done" even when the copper-side link was reported as up, but the in-band autoneg has not finished. That was the _intended_ behavior when that code was introduced, and you have said about it: https://www.spinics.net/lists/stable/msg389193.html | That's not nice from the PHY: | It signals "link up", and if the system asks the PHY for link details, | then it sheepishly says "well, link is *almost* up". If the specification of phy_aneg_done behavior does not include in-band autoneg (and it doesn't), then this piece of code does not belong here. The fact that we can no longer trigger this code from phylib is yet another reason why it fails at its intended (and wrong) purpose and should be removed.
On 18.03.2021 16:17, Vladimir Oltean wrote: > On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: >> On 18.03.2021 15:23, Michael Walle wrote: >>> at803x_aneg_done() is pretty much dead code since the patch series >>> "net: phy: improve and simplify phylib state machine" [1]. Remove it. >>> >> >> Well, it's not dead, it's resting .. There are few places where >> phy_aneg_done() is used. So you would need to explain: >> - why these users can't be used with this PHY driver >> - or why the aneg_done callback isn't needed here and the >> genphy_aneg_done() fallback is sufficient > > The piece of code that Michael is removing keeps the aneg reporting as > "not done" even when the copper-side link was reported as up, but the > in-band autoneg has not finished. > > That was the _intended_ behavior when that code was introduced, and you > have said about it: > https://www.spinics.net/lists/stable/msg389193.html > > | That's not nice from the PHY: > | It signals "link up", and if the system asks the PHY for link details, > | then it sheepishly says "well, link is *almost* up". > > If the specification of phy_aneg_done behavior does not include in-band > autoneg (and it doesn't), then this piece of code does not belong here. > > The fact that we can no longer trigger this code from phylib is yet > another reason why it fails at its intended (and wrong) purpose and > should be removed. > I don't argue against the change, I just think that the current commit description isn't sufficient. What you just said I would have expected in the commit description.
Am 2021-03-18 17:21, schrieb Heiner Kallweit: > On 18.03.2021 16:17, Vladimir Oltean wrote: >> On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: >>> On 18.03.2021 15:23, Michael Walle wrote: >>>> at803x_aneg_done() is pretty much dead code since the patch series >>>> "net: phy: improve and simplify phylib state machine" [1]. Remove >>>> it. >>>> >>> >>> Well, it's not dead, it's resting .. There are few places where >>> phy_aneg_done() is used. So you would need to explain: >>> - why these users can't be used with this PHY driver >>> - or why the aneg_done callback isn't needed here and the >>> genphy_aneg_done() fallback is sufficient >> >> The piece of code that Michael is removing keeps the aneg reporting as >> "not done" even when the copper-side link was reported as up, but the >> in-band autoneg has not finished. >> >> That was the _intended_ behavior when that code was introduced, and >> you >> have said about it: >> https://www.spinics.net/lists/stable/msg389193.html >> >> | That's not nice from the PHY: >> | It signals "link up", and if the system asks the PHY for link >> details, >> | then it sheepishly says "well, link is *almost* up". >> >> If the specification of phy_aneg_done behavior does not include >> in-band >> autoneg (and it doesn't), then this piece of code does not belong >> here. >> >> The fact that we can no longer trigger this code from phylib is yet >> another reason why it fails at its intended (and wrong) purpose and >> should be removed. >> > I don't argue against the change, I just think that the current commit > description isn't sufficient. What you just said I would have expected > in the commit description. I'll come up with a better one, Vladimir, may I use parts of the text above? -michael
On Thu, Mar 18, 2021 at 05:38:13PM +0100, Michael Walle wrote: > Am 2021-03-18 17:21, schrieb Heiner Kallweit: > > On 18.03.2021 16:17, Vladimir Oltean wrote: > > > On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: > > > > On 18.03.2021 15:23, Michael Walle wrote: > > > > > at803x_aneg_done() is pretty much dead code since the patch series > > > > > "net: phy: improve and simplify phylib state machine" [1]. > > > > > Remove it. > > > > > > > > > > > > > Well, it's not dead, it's resting .. There are few places where > > > > phy_aneg_done() is used. So you would need to explain: > > > > - why these users can't be used with this PHY driver > > > > - or why the aneg_done callback isn't needed here and the > > > > genphy_aneg_done() fallback is sufficient > > > > > > The piece of code that Michael is removing keeps the aneg reporting as > > > "not done" even when the copper-side link was reported as up, but the > > > in-band autoneg has not finished. > > > > > > That was the _intended_ behavior when that code was introduced, and > > > you > > > have said about it: > > > https://www.spinics.net/lists/stable/msg389193.html > > > > > > | That's not nice from the PHY: > > > | It signals "link up", and if the system asks the PHY for link details, > > > | then it sheepishly says "well, link is *almost* up". > > > > > > If the specification of phy_aneg_done behavior does not include > > > in-band > > > autoneg (and it doesn't), then this piece of code does not belong > > > here. > > > > > > The fact that we can no longer trigger this code from phylib is yet > > > another reason why it fails at its intended (and wrong) purpose and > > > should be removed. > > > > > I don't argue against the change, I just think that the current commit > > description isn't sufficient. What you just said I would have expected > > in the commit description. > > I'll come up with a better one, Vladimir, may I use parts of the text > above? My words aren't copyrighted, so feel free, however you might want to check with Heiner too for his part, you never know.
On 18.03.2021 18:04, Vladimir Oltean wrote: > On Thu, Mar 18, 2021 at 05:38:13PM +0100, Michael Walle wrote: >> Am 2021-03-18 17:21, schrieb Heiner Kallweit: >>> On 18.03.2021 16:17, Vladimir Oltean wrote: >>>> On Thu, Mar 18, 2021 at 03:54:00PM +0100, Heiner Kallweit wrote: >>>>> On 18.03.2021 15:23, Michael Walle wrote: >>>>>> at803x_aneg_done() is pretty much dead code since the patch series >>>>>> "net: phy: improve and simplify phylib state machine" [1]. >>>>>> Remove it. >>>>>> >>>>> >>>>> Well, it's not dead, it's resting .. There are few places where >>>>> phy_aneg_done() is used. So you would need to explain: >>>>> - why these users can't be used with this PHY driver >>>>> - or why the aneg_done callback isn't needed here and the >>>>> genphy_aneg_done() fallback is sufficient >>>> >>>> The piece of code that Michael is removing keeps the aneg reporting as >>>> "not done" even when the copper-side link was reported as up, but the >>>> in-band autoneg has not finished. >>>> >>>> That was the _intended_ behavior when that code was introduced, and >>>> you >>>> have said about it: >>>> https://www.spinics.net/lists/stable/msg389193.html >>>> >>>> | That's not nice from the PHY: >>>> | It signals "link up", and if the system asks the PHY for link details, >>>> | then it sheepishly says "well, link is *almost* up". >>>> >>>> If the specification of phy_aneg_done behavior does not include >>>> in-band >>>> autoneg (and it doesn't), then this piece of code does not belong >>>> here. >>>> >>>> The fact that we can no longer trigger this code from phylib is yet >>>> another reason why it fails at its intended (and wrong) purpose and >>>> should be removed. >>>> >>> I don't argue against the change, I just think that the current commit >>> description isn't sufficient. What you just said I would have expected >>> in the commit description. >> >> I'll come up with a better one, Vladimir, may I use parts of the text >> above? > > My words aren't copyrighted, so feel free, however you might want to > check with Heiner too for his part, you never know. > I'm not paid for the content of my mails, so feel free to quote.
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index c2aa4c92edde..d7799beb811c 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -751,36 +751,6 @@ static void at803x_link_change_notify(struct phy_device *phydev) } } -static int at803x_aneg_done(struct phy_device *phydev) -{ - int ccr; - - int aneg_done = genphy_aneg_done(phydev); - if (aneg_done != BMSR_ANEGCOMPLETE) - return aneg_done; - - /* - * in SGMII mode, if copper side autoneg is successful, - * also check SGMII side autoneg result - */ - ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG); - if ((ccr & AT803X_MODE_CFG_MASK) != AT803X_MODE_CFG_SGMII) - return aneg_done; - - /* switch to SGMII/fiber page */ - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr & ~AT803X_BT_BX_REG_SEL); - - /* check if the SGMII link is OK. */ - if (!(phy_read(phydev, AT803X_PSSR) & AT803X_PSSR_MR_AN_COMPLETE)) { - phydev_warn(phydev, "803x_aneg_done: SGMII link is not ok\n"); - aneg_done = 0; - } - /* switch back to copper page */ - phy_write(phydev, AT803X_REG_CHIP_CONFIG, ccr | AT803X_BT_BX_REG_SEL); - - return aneg_done; -} - static int at803x_read_status(struct phy_device *phydev) { int ss, err, old_link = phydev->link; @@ -1198,7 +1168,6 @@ static struct phy_driver at803x_driver[] = { .resume = at803x_resume, /* PHY_GBIT_FEATURES */ .read_status = at803x_read_status, - .aneg_done = at803x_aneg_done, .config_intr = &at803x_config_intr, .handle_interrupt = at803x_handle_interrupt, .get_tunable = at803x_get_tunable,
at803x_aneg_done() is pretty much dead code since the patch series "net: phy: improve and simplify phylib state machine" [1]. Remove it. [1] https://lore.kernel.org/netdev/922c223b-7bc0-e0ec-345d-2034b796af91@gmail.com/ Suggested-by: Vladimir Oltean <olteanv@gmail.com> Signed-off-by: Michael Walle <michael@walle.cc> --- drivers/net/phy/at803x.c | 31 ------------------------------- 1 file changed, 31 deletions(-)