| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150803150258.GD9999@pengutronix.de>
Date: Mon, 3 Aug 2015 17:02:58 +0200
From: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
To: Andrew Lunn <andrew@...n.ch>
Cc: fabio.estevam@...escale.com, netdev <netdev@...r.kernel.org>,
tyler.baker@...aro.org, kernel@...gutronix.de,
shawn.guo@...aro.org, Lucas Stach <l.stach@...gutronix.de>
Subject: Re: [PATCHv6 RFT] net: fec: Ensure clocks are enabled while using
mdio bus
Hello,
On Mon, Aug 03, 2015 at 03:50:23PM +0200, Andrew Lunn wrote:
> > The problem is that on i.MX27 there are two clocks involved that both
> > must be on to send a packet, while on i.MX6 it's only a single one
> > (abstracted by having ipg-clock = ahb-clock). With the suggested patch
> > only a single one is asserted to be on. This is enough for i.MX6 but
> > it's not for i.MX27 (and from looking at the device trees also i.MX25,
> > i.MX28, and i.MX35 are affected).
>
> I don't think it is as simple as this. If you are sending a packet,
> fec_enet_open() must of been called. This does a pm_runtime_get_sync()
> to ensure the ipg-clock is ticking, and fec_enet_clk_enable() to
> enable all other clocks.
>
> Can you debug this further to find out which clock is off, and where
> it gets turned off?
I added a call to pm_runtime_get_sync to fec_enet_start_xmit and put it
back at its end. This way I was able to boot with NFS-root which
resulted in an oops before. But this is wrong because if I set
FEC_MDIO_PM_TIMEOUT to 0 I get an oops anyhow.
I added the following patch:
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 32e3807c650e..508c4af4fde8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -64,6 +64,30 @@
#include "fec.h"
+#define fec_pm_runtime_get_sync(_dev) ({ \
+ dev_info((_dev), "%s: get_sync (%d)", __func__, (_dev)->power.usage_count.counter); \
+ pm_runtime_get_sync((_dev)); \
+})
+#define pm_runtime_get_sync(_dev) fec_pm_runtime_get_sync(_dev)
+
+#define fec_pm_runtime_put_autosuspend(_dev) ({ \
+ dev_info((_dev), "%s: put_autosuspend (%d)", __func__, (_dev)->power.usage_count.counter); \
+ pm_runtime_put_autosuspend((_dev)); \
+})
+#define pm_runtime_put_autosuspend(_dev) fec_pm_runtime_put_autosuspend(_dev)
+
+#define fec_clk_prepare_enable(_clk) ({ \
+ pr_info("%s: enable " #_clk "\n", __func__); \
+ clk_prepare_enable((_clk)); \
+})
+#define clk_prepare_enable(_clk) fec_clk_prepare_enable(_clk)
+
+#define fec_clk_disable_unprepare(_clk) ({ \
+ pr_info("%s: disable " #_clk "\n", __func__); \
+ clk_disable_unprepare((_clk)); \
+})
+#define clk_disable_unprepare(_clk) fec_clk_disable_unprepare(_clk)
+
static void set_multicast_list(struct net_device *ndev);
static void fec_enet_itr_coal_init(struct net_device *ndev);
@@ -784,6 +808,7 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
struct netdev_queue *nq;
int ret;
+ pr_info("%s\n", __func__);
queue = skb_get_queue_mapping(skb);
txq = fep->tx_queue[queue];
nq = netdev_get_tx_queue(ndev, queue);
@@ -2864,6 +2889,7 @@ fec_enet_open(struct net_device *ndev)
struct fec_enet_private *fep = netdev_priv(ndev);
int ret;
+ pr_info("%s\n", __func__);
ret = pm_runtime_get_sync(&fep->pdev->dev);
if (IS_ERR_VALUE(ret))
return ret;
@@ -2932,6 +2958,8 @@ fec_enet_close(struct net_device *ndev)
fec_enet_free_buffers(ndev);
+ pr_info("%s\n", __func__);
+
return 0;
}
@@ -3416,6 +3444,7 @@ fec_probe(struct platform_device *pdev)
goto failed_clk;
ret = clk_prepare_enable(fep->clk_ipg);
+ clk_prepare_enable(fep->clk_ipg);
if (ret)
goto failed_clk_ipg;
Which produced the following output (piped through nl for better
reference):
1 fec_enet_open
2 fec 1002b000.ethernet: fec_enet_open: get_sync (-1)
3 fec_runtime_resume: enable fep->clk_ipg
4 fec_enet_clk_enable: enable fep->clk_ahb
5 fec 1002b000.ethernet: fec_enet_mdio_write: get_sync (0)
6 fec 1002b000.ethernet: fec_enet_mdio_write: put_autosuspend (1)
7 mmc0: new SD card at address 0007
8 mmcblk0: mmc0:0007 SD01G 972 MiB (ro)
9 mmcblk0: p1
10 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
11 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
12 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
13 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
14 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
15 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
16 fec 1002b000.ethernet: fec_enet_mdio_write: get_sync (0)
17 fec 1002b000.ethernet: fec_enet_mdio_write: put_autosuspend (1)
18 fec 1002b000.ethernet eth0: Freescale FEC PHY driver [Generic PHY] (mii_bus:phy_addr=1002b000.etherne:00, irq=-1)
19 fec_runtime_suspend: disable fep->clk_ipg
20 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
21 fec_runtime_resume: enable fep->clk_ipg
22 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
23 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
24 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
25 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
26 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
27 fec_runtime_suspend: disable fep->clk_ipg
28 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
29 fec_runtime_resume: enable fep->clk_ipg
30 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
31 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
32 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
33 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
34 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
35 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
36 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
37 fec_runtime_suspend: disable fep->clk_ipg
38 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
39 fec_runtime_resume: enable fep->clk_ipg
40 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
41 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
42 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
43 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
44 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
45 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
46 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
47 fec 1002b000.ethernet: fec_enet_mdio_read: get_sync (0)
48 fec 1002b000.ethernet: fec_enet_mdio_read: put_autosuspend (1)
49 fec 1002b000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
50 Sending DHCP requests .
51 fec_enet_start_xmit
52 ,
53 fec_enet_start_xmit
54 OK
55 IP-Config: Got DHCP answer from 192.168.23.4, my address is 192.168.24.64
56 IP-Config: Complete:
57 device=eth0, hwaddr=4a:5c:f5:cb:22:15, ipaddr=192.168.24.64, mask=255.255.0.0, gw=192.168.23.254
58 host=192.168.24.64, domain=lab.pengutronix.de, nis-domain=(none)
59 fec_runtime_suspend: disable fep->clk_ipg
60 bootserver=192.168.23.4, rootserver=192.168.23.4, rootpath=
61 nameserver0=192.168.23.254
62 fec_enet_start_xmit
You see, fec_runtime_suspend is called even though _open was called. (And even
though get_sync was called once more than put_autosuspend).
Without the additional clk_prepare_enable in probe line 62 is where the
machine barfs.
I don't understand it starring at the code for a while. But the -1 in
line 2 looks fishy.
> > As I think it would be bold to fix this commit once more for 4.2, I
> > suggest to revert 8fff755e9f8d0f70a595e79f248695ce6aef5cc3.
> > (To be honest I wonder why v5 of this commit was even taken for -rc3.)
> >
> > The straight forward fix for the MDIO issue that was intended to be
> > addressed by 8fff755e9f8d is to add clk_prepare_enable and matching
> > _unprepare_disable calls to the mdio callbacks, right?
>
> You mean v1 of the patch?
>
> https://patchwork.ozlabs.org/patch/483668/
>
> Or at least something similar. That idea got NACK because it is
> thought to consume too much power.
I didn't follow the discussion. But yes, that's what I'd do. I'm with
Florian here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists