* [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
@ 2020-01-02 14:33 Andrew Lunn
2020-01-03 0:38 ` David Miller
2020-01-03 0:51 ` Florian Fainelli
0 siblings, 2 replies; 6+ messages in thread
From: Andrew Lunn @ 2020-01-02 14:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev, fugang.duan, Chris Healy, Andrew Lunn
In order to dump the FECs registers the clocks have to be ticking,
otherwise a data abort occurs. Add calls to runtime PM so they are
enabled and later disabled.
Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
Reported-by: Chris Healy <Chris.Healy@zii.aero>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/ethernet/freescale/fec_main.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 05c1899f6628..9294027e9d90 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2199,8 +2199,14 @@ static void fec_enet_get_regs(struct net_device *ndev,
{
struct fec_enet_private *fep = netdev_priv(ndev);
u32 __iomem *theregs = (u32 __iomem *)fep->hwp;
+ struct device *dev = &fep->pdev->dev;
u32 *buf = (u32 *)regbuf;
u32 i, off;
+ int ret;
+
+ ret = pm_runtime_get_sync(dev);
+ if (ret < 0)
+ return;
regs->version = fec_enet_register_version;
@@ -2216,6 +2222,9 @@ static void fec_enet_get_regs(struct net_device *ndev,
off >>= 2;
buf[off] = readl(&theregs[off]);
}
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
}
static int fec_enet_get_ts_info(struct net_device *ndev,
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
2020-01-02 14:33 [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM Andrew Lunn
@ 2020-01-03 0:38 ` David Miller
2020-01-03 0:51 ` Florian Fainelli
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2020-01-03 0:38 UTC (permalink / raw)
To: andrew; +Cc: netdev, fugang.duan, Chris.Healy
From: Andrew Lunn <andrew@lunn.ch>
Date: Thu, 2 Jan 2020 15:33:34 +0100
> In order to dump the FECs registers the clocks have to be ticking,
> otherwise a data abort occurs. Add calls to runtime PM so they are
> enabled and later disabled.
>
> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> Reported-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
2020-01-02 14:33 [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM Andrew Lunn
2020-01-03 0:38 ` David Miller
@ 2020-01-03 0:51 ` Florian Fainelli
2020-01-03 1:01 ` Andrew Lunn
1 sibling, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2020-01-03 0:51 UTC (permalink / raw)
To: Andrew Lunn, David Miller; +Cc: netdev, fugang.duan, Chris Healy
On 1/2/20 6:33 AM, Andrew Lunn wrote:
> In order to dump the FECs registers the clocks have to be ticking,
> otherwise a data abort occurs. Add calls to runtime PM so they are
> enabled and later disabled.
>
> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> Reported-by: Chris Healy <Chris.Healy@zii.aero>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
> drivers/net/ethernet/freescale/fec_main.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 05c1899f6628..9294027e9d90 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -2199,8 +2199,14 @@ static void fec_enet_get_regs(struct net_device *ndev,
> {
> struct fec_enet_private *fep = netdev_priv(ndev);
> u32 __iomem *theregs = (u32 __iomem *)fep->hwp;
> + struct device *dev = &fep->pdev->dev;
> u32 *buf = (u32 *)regbuf;
> u32 i, off;
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + return;
>
> regs->version = fec_enet_register_version;
>
> @@ -2216,6 +2222,9 @@ static void fec_enet_get_regs(struct net_device *ndev,
> off >>= 2;
> buf[off] = readl(&theregs[off]);
> }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
This fix will do, but you should consider implementing
ethtool_ops::begin and ethtool_ops::end to make sure this condition is
resolved for all ethtool operations.
For instance the following looks possibly problematic too:
fec_enet_set_coalesce -> fec_enet_itr_coal_set
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
2020-01-03 0:51 ` Florian Fainelli
@ 2020-01-03 1:01 ` Andrew Lunn
2020-01-03 2:02 ` [EXT] " Andy Duan
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2020-01-03 1:01 UTC (permalink / raw)
To: Florian Fainelli; +Cc: David Miller, netdev, fugang.duan, Chris Healy
> This fix will do, but you should consider implementing
> ethtool_ops::begin and ethtool_ops::end to make sure this condition is
> resolved for all ethtool operations.
>
> For instance the following looks possibly problematic too:
> fec_enet_set_coalesce -> fec_enet_itr_coal_set
Hi Florian
I did a quick test of all the ethtool operations which the driver
supports, including setting coalescing. I did not exhaustively try all
possible coalescing settings, but the ones i did try did not provoke a
data abort.
Still, it would make sense to implement begin and end, but only for
net-next.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [EXT] Re: [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
2020-01-03 1:01 ` Andrew Lunn
@ 2020-01-03 2:02 ` Andy Duan
2020-01-03 2:38 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Andy Duan @ 2020-01-03 2:02 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: David Miller, netdev, Chris Healy
From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, January 3, 2020 9:02 AM
> > This fix will do, but you should consider implementing
> > ethtool_ops::begin and ethtool_ops::end to make sure this condition is
> > resolved for all ethtool operations.
> >
> > For instance the following looks possibly problematic too:
> > fec_enet_set_coalesce -> fec_enet_itr_coal_set
>
> Hi Florian
>
> I did a quick test of all the ethtool operations which the driver supports,
> including setting coalescing. I did not exhaustively try all possible coalescing
> settings, but the ones i did try did not provoke a data abort.
The original design is that driver power off clocks when net interface is down,
use ethtool to dump registers are not allowed. Only .get_regs/ .get/set_coalesce
are allowed when the net interface is up by checking netif_running(ndev).
If there have requirement to access registers when net interface is down status,
It is better to change the common code to enhance ethtool_ops callbacks usage
to support ethtool_ops::begin and ethtool_ops::end for all net drivers.
>
> Still, it would make sense to implement begin and end, but only for net-next.
>
> Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [EXT] Re: [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM
2020-01-03 2:02 ` [EXT] " Andy Duan
@ 2020-01-03 2:38 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2020-01-03 2:38 UTC (permalink / raw)
To: Andy Duan; +Cc: Florian Fainelli, David Miller, netdev, Chris Healy
On Fri, Jan 03, 2020 at 02:02:50AM +0000, Andy Duan wrote:
> From: Andrew Lunn <andrew@lunn.ch> Sent: Friday, January 3, 2020 9:02 AM
> > > This fix will do, but you should consider implementing
> > > ethtool_ops::begin and ethtool_ops::end to make sure this condition is
> > > resolved for all ethtool operations.
> > >
> > > For instance the following looks possibly problematic too:
> > > fec_enet_set_coalesce -> fec_enet_itr_coal_set
> >
> > Hi Florian
> >
> > I did a quick test of all the ethtool operations which the driver supports,
> > including setting coalescing. I did not exhaustively try all possible coalescing
> > settings, but the ones i did try did not provoke a data abort.
>
> The original design is that driver power off clocks when net interface is down,
> use ethtool to dump registers are not allowed. Only .get_regs/ .get/set_coalesce
> are allowed when the net interface is up by checking netif_running(ndev).
Hi Andy
The function fec_enet_get_regs() does not perform a
netif_running(ndev) check. So it seems like it was intended to work
with the interface down. It could be a difference between variants of
the FEC. I was seeing the data abort on the vf610. Maybe its clocks
are different to other FEC implementations?
I've no strong preference about seeing the registers when the
interface is down, or return maybe -ENETDOWN. What i don't want is a
data abort, and locks left in a bad state.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-03 2:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-02 14:33 [PATCH net] net: freescale: fec: Fix ethtool -d runtime PM Andrew Lunn
2020-01-03 0:38 ` David Miller
2020-01-03 0:51 ` Florian Fainelli
2020-01-03 1:01 ` Andrew Lunn
2020-01-03 2:02 ` [EXT] " Andy Duan
2020-01-03 2:38 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).