* [PATCH 1/3] ag71xx: Handle allocation errors in ag71xx_rings_init()
@ 2020-02-17 23:35 Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 2/3] ag71xx: Call ag71xx_hw_disable() in case phy_conenct fails Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
0 siblings, 2 replies; 9+ messages in thread
From: Hauke Mehrtens @ 2020-02-17 23:35 UTC (permalink / raw)
To: davem, linux; +Cc: netdev, jcliburn, chris.snook, Hauke Mehrtens
Free the allocated resources in ag71xx_rings_init() in case
ag71xx_ring_rx_init() returns an error.
This is only a potential problem, I did not ran into this one.
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
---
drivers/net/ethernet/atheros/ag71xx.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index e95687a780fb..6f7281f38d5a 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1133,6 +1133,7 @@ static int ag71xx_rings_init(struct ag71xx *ag)
struct ag71xx_ring *tx = &ag->tx_ring;
struct ag71xx_ring *rx = &ag->rx_ring;
int ring_size, tx_size;
+ int ret;
ring_size = BIT(tx->order) + BIT(rx->order);
tx_size = BIT(tx->order);
@@ -1145,9 +1146,8 @@ static int ag71xx_rings_init(struct ag71xx *ag)
ring_size * AG71XX_DESC_SIZE,
&tx->descs_dma, GFP_KERNEL);
if (!tx->descs_cpu) {
- kfree(tx->buf);
- tx->buf = NULL;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto err_free_buf;
}
rx->buf = &tx->buf[tx_size];
@@ -1155,7 +1155,21 @@ static int ag71xx_rings_init(struct ag71xx *ag)
rx->descs_dma = tx->descs_dma + tx_size * AG71XX_DESC_SIZE;
ag71xx_ring_tx_init(ag);
- return ag71xx_ring_rx_init(ag);
+ ret = ag71xx_ring_rx_init(ag);
+ if (ret)
+ goto err_free_dma;
+
+ return 0;
+
+err_free_dma:
+ dma_free_coherent(&ag->pdev->dev, ring_size * AG71XX_DESC_SIZE,
+ tx->descs_cpu, tx->descs_dma);
+ rx->buf = NULL;
+err_free_buf:
+ kfree(tx->buf);
+ tx->buf = NULL;
+
+ return ret;
}
static void ag71xx_rings_free(struct ag71xx *ag)
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] ag71xx: Call ag71xx_hw_disable() in case phy_conenct fails
2020-02-17 23:35 [PATCH 1/3] ag71xx: Handle allocation errors in ag71xx_rings_init() Hauke Mehrtens
@ 2020-02-17 23:35 ` Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
1 sibling, 0 replies; 9+ messages in thread
From: Hauke Mehrtens @ 2020-02-17 23:35 UTC (permalink / raw)
To: davem, linux; +Cc: netdev, jcliburn, chris.snook, Hauke Mehrtens
When the ag71xx_phy_connect() failed only parts of the actions done
previously in this function were reverted, because only
ag71xx_rings_cleanup() was called. My system crashed the next time
open() was called because napi_disable() was not called again and this
resulted in two calls to napi_enable(), which is not allowed.
Fix this by disabling the device again.
Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
Fixes: d51b6ce441d3 ("net: ethernet: add ag71xx driver")
---
drivers/net/ethernet/atheros/ag71xx.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/atheros/ag71xx.c b/drivers/net/ethernet/atheros/ag71xx.c
index 6f7281f38d5a..7d3fec009030 100644
--- a/drivers/net/ethernet/atheros/ag71xx.c
+++ b/drivers/net/ethernet/atheros/ag71xx.c
@@ -1263,18 +1263,18 @@ static int ag71xx_open(struct net_device *ndev)
ret = ag71xx_hw_enable(ag);
if (ret)
- goto err;
+ return ret;
ret = ag71xx_phy_connect(ag);
if (ret)
- goto err;
+ goto err_hw_disable;
phy_start(ndev->phydev);
return 0;
-err:
- ag71xx_rings_cleanup(ag);
+err_hw_disable:
+ ag71xx_hw_disable(ag);
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-17 23:35 [PATCH 1/3] ag71xx: Handle allocation errors in ag71xx_rings_init() Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 2/3] ag71xx: Call ag71xx_hw_disable() in case phy_conenct fails Hauke Mehrtens
@ 2020-02-17 23:35 ` Hauke Mehrtens
2020-02-18 5:51 ` David Miller
` (2 more replies)
1 sibling, 3 replies; 9+ messages in thread
From: Hauke Mehrtens @ 2020-02-17 23:35 UTC (permalink / raw)
To: davem, linux; +Cc: netdev, jcliburn, chris.snook, Hauke Mehrtens
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;
+ }
+ }
+
+ 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)
--
2.20.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
@ 2020-02-18 5:51 ` David Miller
2020-02-18 7:02 ` Heiner Kallweit
2020-02-18 9:43 ` Sergei Shtylyov
2 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2020-02-18 5:51 UTC (permalink / raw)
To: hauke; +Cc: linux, netdev, jcliburn, chris.snook
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
2020-02-18 5:51 ` David Miller
@ 2020-02-18 7:02 ` Heiner Kallweit
2020-02-18 10:30 ` Oleksij Rempel
2020-02-18 9:43 ` Sergei Shtylyov
2 siblings, 1 reply; 9+ messages in thread
From: Heiner Kallweit @ 2020-02-18 7:02 UTC (permalink / raw)
To: Hauke Mehrtens, davem, linux; +Cc: netdev, jcliburn, chris.snook
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)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
2020-02-18 5:51 ` David Miller
2020-02-18 7:02 ` Heiner Kallweit
@ 2020-02-18 9:43 ` Sergei Shtylyov
2 siblings, 0 replies; 9+ messages in thread
From: Sergei Shtylyov @ 2020-02-18 9:43 UTC (permalink / raw)
To: Hauke Mehrtens, davem, linux; +Cc: netdev, jcliburn, chris.snook
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-18 7:02 ` Heiner Kallweit
@ 2020-02-18 10:30 ` Oleksij Rempel
2020-03-01 12:19 ` Hauke Mehrtens
0 siblings, 1 reply; 9+ messages in thread
From: Oleksij Rempel @ 2020-02-18 10:30 UTC (permalink / raw)
To: Heiner Kallweit, Hauke Mehrtens, davem; +Cc: netdev, jcliburn, chris.snook
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-02-18 10:30 ` Oleksij Rempel
@ 2020-03-01 12:19 ` Hauke Mehrtens
2020-03-01 13:24 ` Oleksij Rempel
0 siblings, 1 reply; 9+ messages in thread
From: Hauke Mehrtens @ 2020-03-01 12:19 UTC (permalink / raw)
To: Oleksij Rempel, Heiner Kallweit, davem; +Cc: netdev, jcliburn, chris.snook
[-- Attachment #1.1: Type: text/plain, Size: 3411 bytes --]
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
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed
2020-03-01 12:19 ` Hauke Mehrtens
@ 2020-03-01 13:24 ` Oleksij Rempel
0 siblings, 0 replies; 9+ messages in thread
From: Oleksij Rempel @ 2020-03-01 13:24 UTC (permalink / raw)
To: Hauke Mehrtens, Heiner Kallweit, davem; +Cc: netdev, jcliburn, chris.snook
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-01 13:24 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 23:35 [PATCH 1/3] ag71xx: Handle allocation errors in ag71xx_rings_init() Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 2/3] ag71xx: Call ag71xx_hw_disable() in case phy_conenct fails Hauke Mehrtens
2020-02-17 23:35 ` [PATCH 3/3] ag71xx: Run ag71xx_link_adjust() only when needed Hauke Mehrtens
2020-02-18 5:51 ` David Miller
2020-02-18 7:02 ` Heiner Kallweit
2020-02-18 10:30 ` Oleksij Rempel
2020-03-01 12:19 ` Hauke Mehrtens
2020-03-01 13:24 ` Oleksij Rempel
2020-02-18 9:43 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.