From: Russell King - ARM Linux <hidden> Date: 2014-08-04 18:11:06
On Mon, Aug 04, 2014 at 02:53:42PM -0300, Fabio Estevam wrote:
Unable to handle kernel NULL pointer dereference at virtual address 0000002c
pgd = bcd14000
[0000002c] *pgd=4d9e0831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] SMP ARM
Modules linked in:
CPU: 0 PID: 617 Comm: sh Not tainted 3.16.0 #17
task: bc0c4e00 ti: bceb6000 task.ti: bceb6000
PC is at fec_suspend+0x10/0x70
LR is at dpm_run_callback.isra.7+0x34/0x6c
pc : [<803f8a98>] lr : [<80361f44>] psr: 600f0013
sp : bceb7d70 ip : bceb7d88 fp : bceb7d84
r10: 8091523c r9 : 00000000 r8 : bd88f478
r7 : 803f8a88 r6 : 81165988 r5 : 00000000 r4 : 00000000
r3 : 00000000 r2 : 00000000 r1 : bd88f478 r0 : bd88f478
Right, so the code which oopsed was:
0: e1a0c00d mov ip, sp
4: e92dd818 push {r3, r4, fp, ip, lr, pc}
8: e24cb004 sub fp, ip, #4
c: e5904088 ldr r4, [r0, #136] ; 0x88
10: e594302c ldr r3, [r4, #44] ; 0x2c
and from the above, we can see that r4 was NULL.
fec_suspend(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
which would make ndev NULL. I don't see how that could happen though,
as we set the driver data in the probe function. Any patches on top of
3.16?
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
Hi Russell,
On Mon, Aug 4, 2014 at 3:11 PM, Russell King - ARM Linux
[off-list ref] wrote:
Right, so the code which oopsed was:
0: e1a0c00d mov ip, sp
4: e92dd818 push {r3, r4, fp, ip, lr, pc}
8: e24cb004 sub fp, ip, #4
c: e5904088 ldr r4, [r0, #136] ; 0x88
10: e594302c ldr r3, [r4, #44] ; 0x2c
and from the above, we can see that r4 was NULL.
fec_suspend(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
which would make ndev NULL. I don't see how that could happen though,
as we set the driver data in the probe function. Any patches on top of
3.16?
Yes, I still do not understand why we are getting this NULL pointer
inside fec_suspend().
I am using the original 3.16 with no other patches on top.
Thanks
On Mon, Aug 4, 2014 at 3:21 PM, Fabio Estevam [off-list ref] wrote:
Hi Russell,
On Mon, Aug 4, 2014 at 3:11 PM, Russell King - ARM Linux
[off-list ref] wrote:
quoted
Right, so the code which oopsed was:
0: e1a0c00d mov ip, sp
4: e92dd818 push {r3, r4, fp, ip, lr, pc}
8: e24cb004 sub fp, ip, #4
c: e5904088 ldr r4, [r0, #136] ; 0x88
10: e594302c ldr r3, [r4, #44] ; 0x2c
and from the above, we can see that r4 was NULL.
fec_suspend(struct device *dev)
{
struct net_device *ndev = dev_get_drvdata(dev);
which would make ndev NULL. I don't see how that could happen though,
as we set the driver data in the probe function. Any patches on top of
3.16?
Yes, I still do not understand why we are getting this NULL pointer
inside fec_suspend().
I am using the original 3.16 with no other patches on top.
I managed to bisect it. Will start a thread at netdev about it.
On Tue, Aug 05, 2014 at 02:06:18PM +0800, Duan Fugang-B38611 wrote:
I use linux net tree cannot reproduce the issue like your log, but show another issue as below log:
(imx6dl sabresd board, Nfs mount rootfs)
root at freescale ~$ uname -r
3.16.0-rc5-01146-gda388973d
Fabio already bisect the issue down to commit a71e3c37960c (net: phy:
Set the driver when registering an MDIO bus device). This commit
landed on mainline after v3.16-rc7, and that may be the reason you
do not see it on net tree (3.16.0-rc5-01146-gda388973d).
Shawn
To: Duan Fugang-B38611
Cc: Fabio Estevam; Sascha Hauer; linux-arm-kernel at lists.infradead.org;
Russell King; Marek Va?ut
Subject: Re: Suspend/resume broken on mx5/mx6 running 4.16
On Tue, Aug 05, 2014 at 02:06:18PM +0800, Duan Fugang-B38611 wrote:
quoted
I use linux net tree cannot reproduce the issue like your log, but show
another issue as below log:
quoted
(imx6dl sabresd board, Nfs mount rootfs)
root at freescale ~$ uname -r
3.16.0-rc5-01146-gda388973d
Fabio already bisect the issue down to commit a71e3c37960c (net: phy:
Set the driver when registering an MDIO bus device). This commit landed
on mainline after v3.16-rc7, and that may be the reason you do not see it
on net tree (3.16.0-rc5-01146-gda388973d).
Shawn
From: Russell King - ARM Linux <hidden> Date: 2014-08-05 09:21:07
On Tue, Aug 05, 2014 at 02:16:59PM +0800, Shawn Guo wrote:
On Tue, Aug 05, 2014 at 02:06:18PM +0800, Duan Fugang-B38611 wrote:
quoted
I use linux net tree cannot reproduce the issue like your log, but show another issue as below log:
(imx6dl sabresd board, Nfs mount rootfs)
root at freescale ~$ uname -r
3.16.0-rc5-01146-gda388973d
Fabio already bisect the issue down to commit a71e3c37960c (net: phy:
Set the driver when registering an MDIO bus device). This commit
landed on mainline after v3.16-rc7, and that may be the reason you
do not see it on net tree (3.16.0-rc5-01146-gda388973d).
Oh my... that commit looks totally bogus.
It has the effect (as can be seen from the oops) of attaching the MDIO bus
device (itself is a bus-less device) to the platform driver, which means
that if the platform driver supports power management, it will be called
to power manage the MDIO bus device.
Moreover, drivers do not expect to be called for power management
operations for devices which they haven't probed, and certainly not for
devices which aren't part of the same bus that the driver is registered
against.
The commit text says:
net: phy: Set the driver when registering an MDIO bus device
mdiobus_register() registers a device which is already bound to a driver.
Hence, the driver pointer should be set properly in order to track down
the driver associated to the MDIO bus.
This will be used to allow ethernet driver to pin down a MDIO bus driver,
preventing it from being unloaded while the PHY device is running.
which misses the implications of adding an unknown parent driver to that
class device - and the argument that it's just to track down the parent
driver is totally bogus. That can already be done - it's the parent
device's driver pointer. Let's take the example of FEC.
/sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/
lrwxrwxrwx 1 root root 0 Aug 5 10:07 device -> ../../../2188000.ethernet
drwxr-xr-x 2 root root 0 Aug 5 10:07 power
lrwxrwxrwx 1 root root 0 Aug 5 10:07 subsystem -> ../../../../../../../class/mdio_bus
-rw-r--r-- 1 root root 4096 Aug 5 10:07 uevent
(note that the "%s-%x" format for this device in fec_main.c has been
truncated - that's another bug!)
Remembering that this is a class device, these devices have a "device"
symlink which point at the parent device. Normal devices which are bound
to a driver have a "driver" symlink.
So, the driver can be reached by following the "device" pointer, and then
following the "driver" symlink:
/sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/device/
...
lrwxrwxrwx 1 root root 0 Aug 5 10:08 driver -> ../../../../../bus/platform/drivers/fec
..
So, given that there are already perfectly good ways to discover the
information stated in the commit message, and that this commit causes
regression, I think this commit should be reverted. Greg, do you
concur?
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
On Tue, Aug 05, 2014 at 10:21:07AM +0100, Russell King - ARM Linux wrote:
On Tue, Aug 05, 2014 at 02:16:59PM +0800, Shawn Guo wrote:
quoted
On Tue, Aug 05, 2014 at 02:06:18PM +0800, Duan Fugang-B38611 wrote:
quoted
I use linux net tree cannot reproduce the issue like your log, but show another issue as below log:
(imx6dl sabresd board, Nfs mount rootfs)
root at freescale ~$ uname -r
3.16.0-rc5-01146-gda388973d
Fabio already bisect the issue down to commit a71e3c37960c (net: phy:
Set the driver when registering an MDIO bus device). This commit
landed on mainline after v3.16-rc7, and that may be the reason you
do not see it on net tree (3.16.0-rc5-01146-gda388973d).
Oh my... that commit looks totally bogus.
It has the effect (as can be seen from the oops) of attaching the MDIO bus
device (itself is a bus-less device) to the platform driver, which means
that if the platform driver supports power management, it will be called
to power manage the MDIO bus device.
Moreover, drivers do not expect to be called for power management
operations for devices which they haven't probed, and certainly not for
devices which aren't part of the same bus that the driver is registered
against.
The commit text says:
net: phy: Set the driver when registering an MDIO bus device
mdiobus_register() registers a device which is already bound to a driver.
Hence, the driver pointer should be set properly in order to track down
the driver associated to the MDIO bus.
This will be used to allow ethernet driver to pin down a MDIO bus driver,
preventing it from being unloaded while the PHY device is running.
which misses the implications of adding an unknown parent driver to that
class device - and the argument that it's just to track down the parent
driver is totally bogus. That can already be done - it's the parent
device's driver pointer. Let's take the example of FEC.
/sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/
lrwxrwxrwx 1 root root 0 Aug 5 10:07 device -> ../../../2188000.ethernet
drwxr-xr-x 2 root root 0 Aug 5 10:07 power
lrwxrwxrwx 1 root root 0 Aug 5 10:07 subsystem -> ../../../../../../../class/mdio_bus
-rw-r--r-- 1 root root 4096 Aug 5 10:07 uevent
(note that the "%s-%x" format for this device in fec_main.c has been
truncated - that's another bug!)
Remembering that this is a class device, these devices have a "device"
symlink which point at the parent device. Normal devices which are bound
to a driver have a "driver" symlink.
So, the driver can be reached by following the "device" pointer, and then
following the "driver" symlink:
/sys/devices/soc0/soc/2100000.aips-bus/2188000.ethernet/mdio_bus/2188000.ethernet/device/
...
lrwxrwxrwx 1 root root 0 Aug 5 10:08 driver -> ../../../../../bus/platform/drivers/fec
..
So, given that there are already perfectly good ways to discover the
information stated in the commit message, and that this commit causes
regression, I think this commit should be reverted. Greg, do you
concur?