Message ID | 20200217233518.3159-3-hauke@hauke-m.de |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [1/3] ag71xx: Handle allocation errors in ag71xx_rings_init() | expand |
From: Hauke Mehrtens <hauke@hauke-m.de> Date: Tue, 18 Feb 2020 00:35:18 +0100 > + if (ag->duplex != phydev->duplex || > + ag->speed != phydev->speed) { > + status_change = 1; > + } A single statement basic block should not be enclosed in curly braces. Thank you.
On 18.02.2020 00:35, Hauke Mehrtens wrote: > My system printed this line every second: > ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off > The function ag71xx_phy_link_adjust() was called by the PHY layer every > second even when nothing changed. > With current phylib code this shouldn't happen, see phy_check_link_status(). On which kernel version do you observe this behavior? > With this patch the old status is stored and the real the > ag71xx_link_adjust() function is only called when something really > changed. This way the update and also this print is only done once any > more. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") > --- > drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > index 7d3fec009030..12eaf6d2518d 100644 > --- a/drivers/net/ethernet/atheros/ag71xx.c > +++ b/drivers/net/ethernet/atheros/ag71xx.c > @@ -307,6 +307,10 @@ struct ag71xx { > u32 msg_enable; > const struct ag71xx_dcfg *dcfg; > > + unsigned int link; > + unsigned int speed; > + int duplex; > + > /* From this point onwards we're not looking at per-packet fields. */ > void __iomem *mac_base; > > @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) > > if (!phydev->link && update) { > ag71xx_hw_stop(ag); > + phy_print_status(phydev); > return; > } > > @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) > static void ag71xx_phy_link_adjust(struct net_device *ndev) > { > struct ag71xx *ag = netdev_priv(ndev); > + struct phy_device *phydev = ndev->phydev; > + int status_change = 0; > + > + if (phydev->link) { > + if (ag->duplex != phydev->duplex || > + ag->speed != phydev->speed) { > + status_change = 1; > + } > + } > + > + if (phydev->link != ag->link) > + status_change = 1; > + > + ag->link = phydev->link; > + ag->duplex = phydev->duplex; > + ag->speed = phydev->speed; > > - ag71xx_link_adjust(ag, true); > + if (status_change) > + ag71xx_link_adjust(ag, true); > } > > static int ag71xx_phy_connect(struct ag71xx *ag) >
Hello! On 18.02.2020 2:35, Hauke Mehrtens wrote: > My system printed this line every second: > ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off > The function ag71xx_phy_link_adjust() was called by the PHY layer every > second even when nothing changed. > > With this patch the old status is stored and the real the > ag71xx_link_adjust() function is only called when something really > changed. This way the update and also this print is only done once any > more. > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> > Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") > --- > drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c > index 7d3fec009030..12eaf6d2518d 100644 > --- a/drivers/net/ethernet/atheros/ag71xx.c > +++ b/drivers/net/ethernet/atheros/ag71xx.c > @@ -307,6 +307,10 @@ struct ag71xx { > u32 msg_enable; > const struct ag71xx_dcfg *dcfg; > > + unsigned int link; > + unsigned int speed; > + int duplex; > + > /* From this point onwards we're not looking at per-packet fields. */ > void __iomem *mac_base; > > @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) > > if (!phydev->link && update) { > ag71xx_hw_stop(ag); > + phy_print_status(phydev); > return; > } > > @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) > static void ag71xx_phy_link_adjust(struct net_device *ndev) > { > struct ag71xx *ag = netdev_priv(ndev); > + struct phy_device *phydev = ndev->phydev; > + int status_change = 0; > + > + if (phydev->link) { > + if (ag->duplex != phydev->duplex || > + ag->speed != phydev->speed) { > + status_change = 1; > + } Do we really need {} here? There's only 1 statement enclosed... > + } > + > + if (phydev->link != ag->link) > + status_change = 1; > + > + ag->link = phydev->link; > + ag->duplex = phydev->duplex; > + ag->speed = phydev->speed; > > - ag71xx_link_adjust(ag, true); > + if (status_change) > + ag71xx_link_adjust(ag, true); > } > > static int ag71xx_phy_connect(struct ag71xx *ag) MBR, Sergei
Hi, current kernel still missing following patch: https://github.com/olerem/linux-2.6/commit/9a9a531bbad6d00c6221f393fd85628475e1f121 @Hauke, can you please test, rebase your changes (if needed) on top of this patch? Am 18.02.20 um 08:02 schrieb Heiner Kallweit: > On 18.02.2020 00:35, Hauke Mehrtens wrote: >> My system printed this line every second: >> ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off >> The function ag71xx_phy_link_adjust() was called by the PHY layer every >> second even when nothing changed. >> > With current phylib code this shouldn't happen, see phy_check_link_status(). > On which kernel version do you observe this behavior? > >> With this patch the old status is stored and the real the >> ag71xx_link_adjust() function is only called when something really >> changed. This way the update and also this print is only done once any >> more. >> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") >> --- >> drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c >> index 7d3fec009030..12eaf6d2518d 100644 >> --- a/drivers/net/ethernet/atheros/ag71xx.c >> +++ b/drivers/net/ethernet/atheros/ag71xx.c >> @@ -307,6 +307,10 @@ struct ag71xx { >> u32 msg_enable; >> const struct ag71xx_dcfg *dcfg; >> >> + unsigned int link; >> + unsigned int speed; >> + int duplex; >> + >> /* From this point onwards we're not looking at per-packet fields. */ >> void __iomem *mac_base; >> >> @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >> >> if (!phydev->link && update) { >> ag71xx_hw_stop(ag); >> + phy_print_status(phydev); >> return; >> } >> >> @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >> static void ag71xx_phy_link_adjust(struct net_device *ndev) >> { >> struct ag71xx *ag = netdev_priv(ndev); >> + struct phy_device *phydev = ndev->phydev; >> + int status_change = 0; >> + >> + if (phydev->link) { >> + if (ag->duplex != phydev->duplex || >> + ag->speed != phydev->speed) { >> + status_change = 1; >> + } >> + } >> + >> + if (phydev->link != ag->link) >> + status_change = 1; >> + >> + ag->link = phydev->link; >> + ag->duplex = phydev->duplex; >> + ag->speed = phydev->speed; >> >> - ag71xx_link_adjust(ag, true); >> + if (status_change) >> + ag71xx_link_adjust(ag, true); >> } >> >> static int ag71xx_phy_connect(struct ag71xx *ag) >> > -- Regards, Oleksij
On 2/18/20 11:30 AM, Oleksij Rempel wrote: > Hi, > > current kernel still missing following patch: > https://github.com/olerem/linux-2.6/commit/9a9a531bbad6d00c6221f393fd85628475e1f121 > > @Hauke, can you please test, rebase your changes (if needed) on top of this patch? I rebased my changes on top of this: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=892e09153fa3564fcbf9f422760b61eba48c123e and my patch is not needed any more, I will send a V2 only containing the first patch of this series, which should fix a potential bug. You missed adding RGMII support in your patch, but the MAC supports RGMII, I will also send a patch for that. @Oleksij: Are you planning to add support for the GMAC configuration like RGMII delays and so on? Hauke > > Am 18.02.20 um 08:02 schrieb Heiner Kallweit: >> On 18.02.2020 00:35, Hauke Mehrtens wrote: >>> My system printed this line every second: >>> ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off >>> The function ag71xx_phy_link_adjust() was called by the PHY layer every >>> second even when nothing changed. >>> >> With current phylib code this shouldn't happen, see phy_check_link_status(). >> On which kernel version do you observe this behavior? >> >>> With this patch the old status is stored and the real the >>> ag71xx_link_adjust() function is only called when something really >>> changed. This way the update and also this print is only done once any >>> more. >>> >>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") >>> --- >>> drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- >>> 1 file changed, 23 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c >>> index 7d3fec009030..12eaf6d2518d 100644 >>> --- a/drivers/net/ethernet/atheros/ag71xx.c >>> +++ b/drivers/net/ethernet/atheros/ag71xx.c >>> @@ -307,6 +307,10 @@ struct ag71xx { >>> u32 msg_enable; >>> const struct ag71xx_dcfg *dcfg; >>> >>> + unsigned int link; >>> + unsigned int speed; >>> + int duplex; >>> + >>> /* From this point onwards we're not looking at per-packet fields. */ >>> void __iomem *mac_base; >>> >>> @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >>> >>> if (!phydev->link && update) { >>> ag71xx_hw_stop(ag); >>> + phy_print_status(phydev); >>> return; >>> } >>> >>> @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >>> static void ag71xx_phy_link_adjust(struct net_device *ndev) >>> { >>> struct ag71xx *ag = netdev_priv(ndev); >>> + struct phy_device *phydev = ndev->phydev; >>> + int status_change = 0; >>> + >>> + if (phydev->link) { >>> + if (ag->duplex != phydev->duplex || >>> + ag->speed != phydev->speed) { >>> + status_change = 1; >>> + } >>> + } >>> + >>> + if (phydev->link != ag->link) >>> + status_change = 1; >>> + >>> + ag->link = phydev->link; >>> + ag->duplex = phydev->duplex; >>> + ag->speed = phydev->speed; >>> >>> - ag71xx_link_adjust(ag, true); >>> + if (status_change) >>> + ag71xx_link_adjust(ag, true); >>> } >>> >>> static int ag71xx_phy_connect(struct ag71xx *ag) >>> >> > > > -- > Regards, > Oleksij >
Am 01.03.20 um 13:19 schrieb Hauke Mehrtens: > On 2/18/20 11:30 AM, Oleksij Rempel wrote: >> Hi, >> >> current kernel still missing following patch: >> https://github.com/olerem/linux-2.6/commit/9a9a531bbad6d00c6221f393fd85628475e1f121 >> >> @Hauke, can you please test, rebase your changes (if needed) on top of this patch? > > I rebased my changes on top of this: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=892e09153fa3564fcbf9f422760b61eba48c123e > and my patch is not needed any more, I will send a V2 only containing > the first patch of this series, which should fix a potential bug. Thank you! > You missed adding RGMII support in your patch, but the MAC supports > RGMII, I will also send a patch for that. > > @Oleksij: Are you planning to add support for the GMAC configuration > like RGMII delays and so on? Not now. Currently I work on AR9331 which has no external RGMII interface. If you have HW capable to do RGMII, patches are welcome :) > Hauke > > >> >> Am 18.02.20 um 08:02 schrieb Heiner Kallweit: >>> On 18.02.2020 00:35, Hauke Mehrtens wrote: >>>> My system printed this line every second: >>>> ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off >>>> The function ag71xx_phy_link_adjust() was called by the PHY layer every >>>> second even when nothing changed. >>>> >>> With current phylib code this shouldn't happen, see phy_check_link_status(). >>> On which kernel version do you observe this behavior? >>> >>>> With this patch the old status is stored and the real the >>>> ag71xx_link_adjust() function is only called when something really >>>> changed. This way the update and also this print is only done once any >>>> more. >>>> >>>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> >>>> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") >>>> --- >>>> drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- >>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c >>>> index 7d3fec009030..12eaf6d2518d 100644 >>>> --- a/drivers/net/ethernet/atheros/ag71xx.c >>>> +++ b/drivers/net/ethernet/atheros/ag71xx.c >>>> @@ -307,6 +307,10 @@ struct ag71xx { >>>> u32 msg_enable; >>>> const struct ag71xx_dcfg *dcfg; >>>> >>>> + unsigned int link; >>>> + unsigned int speed; >>>> + int duplex; >>>> + >>>> /* From this point onwards we're not looking at per-packet fields. */ >>>> void __iomem *mac_base; >>>> >>>> @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >>>> >>>> if (!phydev->link && update) { >>>> ag71xx_hw_stop(ag); >>>> + phy_print_status(phydev); >>>> return; >>>> } >>>> >>>> @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) >>>> static void ag71xx_phy_link_adjust(struct net_device *ndev) >>>> { >>>> struct ag71xx *ag = netdev_priv(ndev); >>>> + struct phy_device *phydev = ndev->phydev; >>>> + int status_change = 0; >>>> + >>>> + if (phydev->link) { >>>> + if (ag->duplex != phydev->duplex || >>>> + ag->speed != phydev->speed) { >>>> + status_change = 1; >>>> + } >>>> + } >>>> + >>>> + if (phydev->link != ag->link) >>>> + status_change = 1; >>>> + >>>> + ag->link = phydev->link; >>>> + ag->duplex = phydev->duplex; >>>> + ag->speed = phydev->speed; >>>> >>>> - ag71xx_link_adjust(ag, true); >>>> + if (status_change) >>>> + ag71xx_link_adjust(ag, true); >>>> } >>>> >>>> static int ag71xx_phy_connect(struct ag71xx *ag) >>>> >>> >> >> >> -- >> Regards, >> Oleksij >> > > -- Regards, Oleksij
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c index 7d3fec009030..12eaf6d2518d 100644 --- a/drivers/net/ethernet/atheros/ag71xx.c +++ b/drivers/net/ethernet/atheros/ag71xx.c @@ -307,6 +307,10 @@ struct ag71xx { u32 msg_enable; const struct ag71xx_dcfg *dcfg; + unsigned int link; + unsigned int speed; + int duplex; + /* From this point onwards we're not looking at per-packet fields. */ void __iomem *mac_base; @@ -854,6 +858,7 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) if (!phydev->link && update) { ag71xx_hw_stop(ag); + phy_print_status(phydev); return; } @@ -907,8 +912,25 @@ static void ag71xx_link_adjust(struct ag71xx *ag, bool update) static void ag71xx_phy_link_adjust(struct net_device *ndev) { struct ag71xx *ag = netdev_priv(ndev); + struct phy_device *phydev = ndev->phydev; + int status_change = 0; + + if (phydev->link) { + if (ag->duplex != phydev->duplex || + ag->speed != phydev->speed) { + status_change = 1; + } + } + + if (phydev->link != ag->link) + status_change = 1; + + ag->link = phydev->link; + ag->duplex = phydev->duplex; + ag->speed = phydev->speed; - ag71xx_link_adjust(ag, true); + if (status_change) + ag71xx_link_adjust(ag, true); } static int ag71xx_phy_connect(struct ag71xx *ag)
My system printed this line every second: ag71xx 19000000.eth eth0: Link is Up - 1Gbps/Full - flow control off The function ag71xx_phy_link_adjust() was called by the PHY layer every second even when nothing changed. With this patch the old status is stored and the real the ag71xx_link_adjust() function is only called when something really changed. This way the update and also this print is only done once any more. Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver") --- drivers/net/ethernet/atheros/ag71xx.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)