Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530084 > unrolled thread

[PATCH] net: fec: turn on device when extracting statistics

Back to article view | Back to linux.kernel

Contents

  [PATCH] net: fec: turn on device when extracting statistics Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-11-25 11:10 +0100
    RE: [PATCH] net: fec: turn on device when extracting statistics Andy Duan <fugang.duan@nxp.com> - 2016-11-27 07:50 +0100
    Re: [PATCH] net: fec: turn on device when extracting statistics David Miller <davem@davemloft.net> - 2016-11-28 02:40 +0100
      Re: [PATCH] net: fec: turn on device when extracting statistics Nikita Yushchenko <nikita.yoush@cogentembedded.com> - 2016-11-28 08:10 +0100
        Re: [PATCH] net: fec: turn on device when extracting statistics David Miller <davem@davemloft.net> - 2016-11-28 15:20 +0100

#1530084 — [PATCH] net: fec: turn on device when extracting statistics

Execution 'ethtool -S' on fec device that is down causes OOPS on Vybrid
board:

Unhandled fault: external abort on non-linefetch (0x1008) at 0xe0898200
pgd = ddecc000
[e0898200] *pgd=9e406811, *pte=400d1653, *ppte=400d1453
Internal error: : 1008 [#1] SMP ARM
...

Reason of OOPS is that fec_enet_get_ethtool_stats() accesses fec
registers while IPG clock is stopped by PM.

Fix that by wrapping statistics extraction into pm_runtime_get_sync()
... pm_runtime_put_autosuspend() braces.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 5aa9d4ded214..9c7592b80ce8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2317,10 +2317,19 @@ static void fec_enet_get_ethtool_stats(struct net_device *dev,
 	struct ethtool_stats *stats, u64 *data)
 {
 	struct fec_enet_private *fep = netdev_priv(dev);
-	int i;
+	int i, ret;
+
+	ret = pm_runtime_get_sync(&fep->pdev->dev);
+	if (IS_ERR_VALUE(ret)) {
+		memset(data, 0, sizeof(*data) * ARRAY_SIZE(fec_stats));
+		return;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
 		data[i] = readl(fep->hwp + fec_stats[i].offset);
+
+	pm_runtime_mark_last_busy(&fep->pdev->dev);
+	pm_runtime_put_autosuspend(&fep->pdev->dev);
 }
 
 static void fec_enet_get_strings(struct net_device *netdev,
-- 
2.1.4

[toc] | [next] | [standalone]

#1530842

From: Nikita Yushchenko <nikita.yoush@cogentembedded.com> Sent: Friday, November 25, 2016 6:02 PM
 >To: Andy Duan <fugang.duan@nxp.com>; David S. Miller
 ><davem@davemloft.net>; Troy Kisky <troy.kisky@boundarydevices.com>;
 >Andrew Lunn <andrew@lunn.ch>; Eric Nelson <eric@nelint.com>; Philippe
 >Reynes <tremyfr@gmail.com>; Johannes Berg <johannes@sipsolutions.net>;
 >netdev@vger.kernel.org; linux-kernel@vger.kernel.org
 >Cc: Chris Healy <cphealy@gmail.com>; Nikita Yushchenko
 ><nikita.yoush@cogentembedded.com>
 >Subject: [PATCH] net: fec: turn on device when extracting statistics
 >
 >Execution 'ethtool -S' on fec device that is down causes OOPS on Vybrid
 >board:
 >
 >Unhandled fault: external abort on non-linefetch (0x1008) at 0xe0898200 pgd
 >= ddecc000 [e0898200] *pgd=9e406811, *pte=400d1653, *ppte=400d1453
 >Internal error: : 1008 [#1] SMP ARM ...
 >
 >Reason of OOPS is that fec_enet_get_ethtool_stats() accesses fec registers
 >while IPG clock is stopped by PM.
 >
 >Fix that by wrapping statistics extraction into pm_runtime_get_sync() ...
 >pm_runtime_put_autosuspend() braces.
 >
 >Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
 >---

Acked-by: Fugang Duan <fugang.duan@nxp.com>

 > drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++++-
 > 1 file changed, 10 insertions(+), 1 deletion(-)
 >
 >diff --git a/drivers/net/ethernet/freescale/fec_main.c
 >b/drivers/net/ethernet/freescale/fec_main.c
 >index 5aa9d4ded214..9c7592b80ce8 100644
 >--- a/drivers/net/ethernet/freescale/fec_main.c
 >+++ b/drivers/net/ethernet/freescale/fec_main.c
 >@@ -2317,10 +2317,19 @@ static void fec_enet_get_ethtool_stats(struct
 >net_device *dev,
 > 	struct ethtool_stats *stats, u64 *data)  {
 > 	struct fec_enet_private *fep = netdev_priv(dev);
 >-	int i;
 >+	int i, ret;
 >+
 >+	ret = pm_runtime_get_sync(&fep->pdev->dev);
 >+	if (IS_ERR_VALUE(ret)) {
 >+		memset(data, 0, sizeof(*data) * ARRAY_SIZE(fec_stats));
 >+		return;
 >+	}
 >
 > 	for (i = 0; i < ARRAY_SIZE(fec_stats); i++)
 > 		data[i] = readl(fep->hwp + fec_stats[i].offset);
 >+
 >+	pm_runtime_mark_last_busy(&fep->pdev->dev);
 >+	pm_runtime_put_autosuspend(&fep->pdev->dev);
 > }
 >
 > static void fec_enet_get_strings(struct net_device *netdev,
 >--
 >2.1.4

[toc] | [prev] | [next] | [standalone]

#1530982

From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Date: Fri, 25 Nov 2016 13:02:00 +0300

> +	int i, ret;
> +
> +	ret = pm_runtime_get_sync(&fep->pdev->dev);
> +	if (IS_ERR_VALUE(ret)) {
> +		memset(data, 0, sizeof(*data) * ARRAY_SIZE(fec_stats));
> +		return;
> +	}

This really isn't the way to do this.

When the device is suspended and the clocks are going to be stopped,
you must fetch the statistic values into a software copy and provide
those if the device is suspended when statistics are requested.

[toc] | [prev] | [next] | [standalone]

#1531069

28.11.2016 04:29, David Miller пишет:
> From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Date: Fri, 25 Nov 2016 13:02:00 +0300
> 
>> +	int i, ret;
>> +
>> +	ret = pm_runtime_get_sync(&fep->pdev->dev);
>> +	if (IS_ERR_VALUE(ret)) {
>> +		memset(data, 0, sizeof(*data) * ARRAY_SIZE(fec_stats));
>> +		return;
>> +	}
> 
> This really isn't the way to do this.
> 
> When the device is suspended and the clocks are going to be stopped,
> you must fetch the statistic values into a software copy and provide
> those if the device is suspended when statistics are requested.

Ok, can do that, although can't see what's wrong with waking device
here. The situation of requesting stats on down device isn't something
widely used, thus keeping handling of that as local as possible looks
better for me.

[toc] | [prev] | [next] | [standalone]

#1531338

From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Date: Mon, 28 Nov 2016 10:06:31 +0300

> 
> 
> 28.11.2016 04:29, David Miller пишет:
>> From: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
>> Date: Fri, 25 Nov 2016 13:02:00 +0300
>> 
>>> +	int i, ret;
>>> +
>>> +	ret = pm_runtime_get_sync(&fep->pdev->dev);
>>> +	if (IS_ERR_VALUE(ret)) {
>>> +		memset(data, 0, sizeof(*data) * ARRAY_SIZE(fec_stats));
>>> +		return;
>>> +	}
>> 
>> This really isn't the way to do this.
>> 
>> When the device is suspended and the clocks are going to be stopped,
>> you must fetch the statistic values into a software copy and provide
>> those if the device is suspended when statistics are requested.
> 
> Ok, can do that, although can't see what's wrong with waking device
> here. The situation of requesting stats on down device isn't something
> widely used, thus keeping handling of that as local as possible looks
> better for me.

The issue is the fact that you need error handling at all and might
therefore provide a set of zero stats to the user when that entire
possibility could have been avoided in the first place by recording
the stats at suspend time.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web