[BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Michal Vokáč]

Hi,

just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
with QCA8334 switch and got this:

[    6.957822] 8<--- cut here ---
[    6.963550] Unable to handle kernel NULL pointer dereference at virtual address 00000264
[    6.974342] pgd = (ptrval)
[    6.979751] [00000264] *pgd=00000000
[    6.986005] Internal error: Oops: 5 [#1] SMP ARM
[    6.993318] CPU: 0 PID: 21 Comm: kworker/0:1 Not tainted 5.3.0-rc5-00985-g0394a63acfe2 #7
[    7.004196] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[    7.013467] Workqueue: events deferred_probe_work_func
[    7.021339] PC is at phy_support_asym_pause+0x14/0x44
[    7.029149] LR is at qca8k_port_enable+0x40/0x48
[    7.036485] pc : [<806840f4>]    lr : [<80686724>]    psr: 60000013
[    7.045489] sp : e821bca0  ip : e821bcb0  fp : e821bcac
[    7.053456] r10: 00000000  r9 : e8241f00  r8 : e812d040
[    7.061431] r7 : 00000000  r6 : 00000000  r5 : e8931640  r4 : 00000002
[    7.070702] r3 : 00000001  r2 : 00000000  r1 : 00000000  r0 : 00000000
[    7.079947] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    7.089825] Control: 10c5387d  Table: 1000404a  DAC: 00000051
[    7.098382] Process kworker/0:1 (pid: 21, stack limit = 0x(ptrval))
[    7.107431] Stack: (0xe821bca0 to 0xe821c000)
[    7.114607] bca0: e821bccc e821bcb0 80686724 806840ec e812d090 e812d090 e88a02cc 00000000
[    7.125637] bcc0: e821bce4 e821bcd0 80a74134 806866f0 e812d090 e812d0cc e821bd7c e821bce8
[    7.136666] bce0: 80a730bc 80a74104 00000000 e88a02cc 00000004 e821bd00 e88a02e0 80cc95f8
[    7.147725] bd00: e88a02e0 e88a02c0 e812d090 e812d090 81006548 e88a02e4 e812d040 e88a02c0
[    7.158791] bd20: 00000003 e812d040 e8830c00 00000000 e821bd5c e821bd40 806243b8 80623abc
[    7.169887] bd40: e8931640 00000000 00000000 5303fd08 e821bd7c e8931640 e8830c00 e8931680
[    7.181000] bd60: 00000000 81077adc 00000004 00000000 e821bd9c e821bd80 806864f8 80a72930
[    7.192107] bd80: 81077adc e8830c00 81123b24 00000000 e821bdb4 e821bda0 806856c8 80686354
[    7.203216] bda0: 81123a18 e8830c00 e821bde4 e821bdb8 8061f944 80685694 00000000 e8830c00
[    7.214318] bdc0: 81077adc e8830c00 81006548 00000001 810c07f0 00000000 e821be1c e821bde8
[    7.225434] bde0: 8061ff68 8061f850 e821be04 e821bdf8 81077adc e821be74 81077adc e821be74
[    7.236589] be00: e8830c00 81006548 00000001 810c07f0 e821be3c e821be20 80620178 8061ff04
[    7.247788] be20: 00000000 e821be74 806200dc 81006548 e821be6c e821be40 8061e160 806200e8
[    7.258982] be40: e821be6c e836d96c e86acdb8 5303fd08 e8830c00 81006548 e8830c44 81071194
[    7.270217] be60: e821bea4 e821be70 8061fd80 8061e104 8061b58c e8830c00 00000001 5303fd08
[    7.281470] be80: 00000000 e8830c00 81077998 e8830c00 81071194 00000000 e821beb4 e821bea8
[    7.292770] bea0: 80620220 8061fcac e821bed4 e821beb8 8061e360 80620210 e8830c00 81071180
[    7.304105] bec0: 81071180 81071194 e821bef4 e821bed8 8061f144 8061e2d8 810711b8 e81f5080
[    7.315494] bee0: eada5f40 eada7100 e821bf34 e821bef8 80142e2c 8061f0dc e8200d00 ffffe000
[    7.326890] bf00: e821bf1c e821bf10 8014234c e81f5080 eada5f40 e81f5094 eada5f58 ffffe000
[    7.338312] bf20: 00000008 81003d00 e821bf74 e821bf38 801433f4 80142c14 ffffe000 80e1264c
[    7.349778] bf40: 810c00c7 eada5f40 80148afc e81f76c0 e81f7680 00000000 e821a000 e81f5080
[    7.361246] bf60: 80143130 e8143e74 e821bfac e821bf78 80148e2c 8014313c e81f76dc e81f76dc
[    7.372758] bf80: e821bfac e81f7680 80148cc4 00000000 00000000 00000000 00000000 00000000
[    7.384313] bfa0: 00000000 e821bfb0 801010e8 80148cd0 00000000 00000000 00000000 00000000
[    7.395858] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    7.407363] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    7.418856] Backtrace:
[    7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
[    7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
[    7.448629]  r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
[    7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
[    7.469833]  r5:e812d0cc r4:e812d090
[    7.476909] [<80a72924>] (dsa_register_switch) from [<806864f8>] (qca8k_sw_probe+0x1b0/0x1c4)
[    7.489008]  r10:00000000 r9:00000004 r8:81077adc r7:00000000 r6:e8931680 r5:e8830c00
[    7.500419]  r4:e8931640
[    7.506528] [<80686348>] (qca8k_sw_probe) from [<806856c8>] (mdio_probe+0x40/0x64)
[    7.517774]  r7:00000000 r6:81123b24 r5:e8830c00 r4:81077adc
[    7.527048] [<80685688>] (mdio_probe) from [<8061f944>] (really_probe+0x100/0x2d8)
[    7.538284]  r5:e8830c00 r4:81123a18
[    7.545528] [<8061f844>] (really_probe) from [<8061ff68>] (driver_probe_device+0x70/0x180)
[    7.557539]  r10:00000000 r9:810c07f0 r8:00000001 r7:81006548 r6:e8830c00 r5:81077adc
[    7.569129]  r4:e8830c00 r3:00000000
[    7.576439] [<8061fef8>] (driver_probe_device) from [<80620178>] (__device_attach_driver+0x9c/0xc8)
[    7.589338]  r9:810c07f0 r8:00000001 r7:81006548 r6:e8830c00 r5:e821be74 r4:81077adc
[    7.600949] [<806200dc>] (__device_attach_driver) from [<8061e160>] (bus_for_each_drv+0x68/0xc8)
[    7.613617]  r7:81006548 r6:806200dc r5:e821be74 r4:00000000
[    7.623099] [<8061e0f8>] (bus_for_each_drv) from [<8061fd80>] (__device_attach+0xe0/0x14c)
[    7.635235]  r7:81071194 r6:e8830c44 r5:81006548 r4:e8830c00
[    7.644681] [<8061fca0>] (__device_attach) from [<80620220>] (device_initial_probe+0x1c/0x20)
[    7.657036]  r8:00000000 r7:81071194 r6:e8830c00 r5:81077998 r4:e8830c00
[    7.667551] [<80620204>] (device_initial_probe) from [<8061e360>] (bus_probe_device+0x94/0x9c)
[    7.680054] [<8061e2cc>] (bus_probe_device) from [<8061f144>] (deferred_probe_work_func+0x74/0xa0)
[    7.692914]  r7:81071194 r6:81071180 r5:81071180 r4:e8830c00
[    7.702440] [<8061f0d0>] (deferred_probe_work_func) from [<80142e2c>] (process_one_work+0x224/0x528)
[    7.715483]  r7:eada7100 r6:eada5f40 r5:e81f5080 r4:810711b8
[    7.725046] [<80142c08>] (process_one_work) from [<801433f4>] (worker_thread+0x2c4/0x5e4)
[    7.737175]  r10:81003d00 r9:00000008 r8:ffffe000 r7:eada5f58 r6:e81f5094 r5:eada5f40
[    7.748949]  r4:e81f5080
[    7.755402] [<80143130>] (worker_thread) from [<80148e2c>] (kthread+0x168/0x170)
[    7.766823]  r10:e8143e74 r9:80143130 r8:e81f5080 r7:e821a000 r6:00000000 r5:e81f7680
[    7.778636]  r4:e81f76c0
[    7.785159] [<80148cc4>] (kthread) from [<801010e8>] (ret_from_fork+0x14/0x2c)
[    7.796457] Exception stack(0xe821bfb0 to 0xe821bff8)
[    7.805543] bfa0:                                     00000000 00000000 00000000 00000000
[    7.817805] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    7.830047] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    7.840688]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80148cc4
[    7.852535]  r4:e81f7680
[    7.859061] Code: e92dd800 e24cb004 e52de004 e8bd4000 (e5902264)
[    7.869239] ---[ end trace 1b5559d3dc3f80a4 ]---

Bisecting the dsa code changes since v5.3 brought me to this commit:

0394a63acfe2a6e1c08af0eb1a9133ee8650d7bd is the first bad commit
commit 0394a63acfe2a6e1c08af0eb1a9133ee8650d7bd
Author: Vivien Didelot <vivien.didelot@gmail.com>
Date:   Mon Aug 19 16:00:50 2019 -0400

     net: dsa: enable and disable all ports
     
     Call the .port_enable and .port_disable functions for all ports,
     not only the user ports, so that drivers may optimize the power
     consumption of all ports after a successful setup.
     
     Unused ports are now disabled on setup. CPU and DSA ports are now
     enabled on setup and disabled on teardown. User ports were already
     enabled at slave creation and disabled at slave destruction.
     
     Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
     Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
     Signed-off-by: David S. Miller <davem@davemloft.net>

:040000 040000 0462b53f03ece23b4af955c3b8a48dce05d90970 34e5df083585e6cce35600698a757508b539e508 M	net

Any ideas what might be wrong?
Thank you!

Michal

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Andrew Lunn]

On Tue, Sep 24, 2019 at 01:27:24PM +0200, Michal Vokáč wrote:
> Hi,
> 
> just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
> with QCA8334 switch and got this:
> 
> [    7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
> [    7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
> [    7.448629]  r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
> [    7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
> [    7.469833]  r5:e812d0cc r4:e812d090

Hi Michal

Please could you add a printk to verify it is the CPU port, and that
in qca8k_port_enable() phy is a NULL pointer.

I think the fix is going to look something like:

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 16f15c93a102..86c80a873e30 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -939,7 +939,8 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
        qca8k_port_set_status(priv, port, 1);
        priv->port_sts[port].enabled = 1;
 
-       phy_support_asym_pause(phy);
+       if (phy)
+               phy_support_asym_pause(phy);
 
        return 0;
 }

But i want to take a closer look at what priv->port_sts[port].enabled
= 1; does. Also, if there are any other port_enable() functions which
always assume a valid phy device.

       Andrew

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Michal Vokáč]

On 24. 09. 19 14:31, Andrew Lunn wrote:
> On Tue, Sep 24, 2019 at 01:27:24PM +0200, Michal Vokáč wrote:
>> Hi,
>>
>> just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
>> with QCA8334 switch and got this:
>>
>> [    7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
>> [    7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
>> [    7.448629]  r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
>> [    7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
>> [    7.469833]  r5:e812d0cc r4:e812d090
> 
> Hi Michal
> 
> Please could you add a printk to verify it is the CPU port, and that
> in qca8k_port_enable() phy is a NULL pointer.
> 
> I think the fix is going to look something like:
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 16f15c93a102..86c80a873e30 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -939,7 +939,8 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>          qca8k_port_set_status(priv, port, 1);
>          priv->port_sts[port].enabled = 1;

  	dev_info(priv->dev, "port: %d, phy: %d", port, (u32)phy);

>   
> -       phy_support_asym_pause(phy);
> +       if (phy)
> +               phy_support_asym_pause(phy);
>   
>          return 0;
>   }
> 

I added the printk and the above fix and can confirm that it is the CPU
port and the phy is NULL pointer:

[    6.976366] qca8k 2188000.ethernet-1:0a: Using legacy PHYLIB callbacks. Please migrate to PHYLINK!
[    6.992021] qca8k 2188000.ethernet-1:0a: port: 0, phy: 0
[    7.001323] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): PHY [2188000.ethernet-1:01] driver [Generic PHY]
[    7.014221] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
[    7.030598] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): PHY [2188000.ethernet-1:02] driver [Generic PHY]
[    7.043500] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
[    7.063335] DSA: tree 0 setup

Now the device boots but there is a problem with the CPU port configuration:

root@hydraco:~# ifconfig eth0 up
[  255.256047] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
[  255.272449] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
[  255.286539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
root@hydraco:~# ifconfig eth1 up
[  268.350078] qca8k 2188000.ethernet-1:0a: port: 3, phy: -393143296
[  268.364442] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
[  268.375400] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
[  268.393901] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
[  268.404849] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
[  268.420740] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
[  268.432995] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
root@hydraco:~# udhcpc -i eth1
udhcpc (v1.23.2) started
Sending discover...
Sending discover...
Sending discover...

On v5.3.1 it works OK:

root@hydraco:~# ifconfig eth0 up
[   29.665997] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
[   29.682075] Generic PHY fixed-0:00: PHY advertising (00,00000200,000022e0) more modes than genphy supports, some modes not advertised.
[   29.698284] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
[   29.709568] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
root@hydraco:~# ifconfig eth1 up
[   33.699923] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
[   33.714466] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
[   33.735356] 8021q: adding VLAN 0 to HW filter on device eth1
[   33.755671] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
[   33.766071] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
[   33.781987] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
[   33.794290] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
oot@hydraco:~# udhcpc -i eth1
udhcpc (v1.23.2) started
Sending discover...
Sending select for 10.1.8.103...

> But i want to take a closer look at what priv->port_sts[port].enabled
> = 1; does. Also, if there are any other port_enable() functions which
> always assume a valid phy device.

Anything else I can do to help with this?

Thank you!
Michal

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Andrew Lunn]

On Tue, Sep 24, 2019 at 03:10:44PM +0200, Michal Vokáč wrote:
> On 24. 09. 19 14:31, Andrew Lunn wrote:
> I added the printk and the above fix and can confirm that it is the CPU
> port and the phy is NULL pointer:
> 
> [    6.976366] qca8k 2188000.ethernet-1:0a: Using legacy PHYLIB callbacks. Please migrate to PHYLINK!
> [    6.992021] qca8k 2188000.ethernet-1:0a: port: 0, phy: 0
> [    7.001323] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): PHY [2188000.ethernet-1:01] driver [Generic PHY]
> [    7.014221] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [    7.030598] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): PHY [2188000.ethernet-1:02] driver [Generic PHY]
> [    7.043500] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [    7.063335] DSA: tree 0 setup
> 
> Now the device boots but there is a problem with the CPU port configuration:

Hi Michal

Thanks for testing. I will post a different fix very soon.

> root@hydraco:~# ifconfig eth0 up
> [  255.256047] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
> [  255.272449] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> [  255.286539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> root@hydraco:~# ifconfig eth1 up
> [  268.350078] qca8k 2188000.ethernet-1:0a: port: 3, phy: -393143296
> [  268.364442] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
> [  268.375400] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
> [  268.393901] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
> [  268.404849] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
> [  268.420740] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
> [  268.432995] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
> root@hydraco:~# udhcpc -i eth1
> udhcpc (v1.23.2) started
> Sending discover...
> Sending discover...
> Sending discover...

This i think is something different. What looks odd is
imx6dl-yapp4-common.dtsi

&fec {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_enet>;
        phy-mode = "rgmii-id";
        phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
        phy-reset-duration = <20>;
        phy-supply = <&sw2_reg>;
        phy-handle = <&ethphy0>;
        status = "okay";


	mdio {

               switch@10 {
                        compatible = "qca,qca8334";
                        reg = <10>;

                        switch_ports: ports {
                                #address-cells = <1>;
                                #size-cells = <0>;

                                ethphy0: port@0 {
                                        reg = <0>;
                                        label = "cpu";
                                        phy-mode = "rgmii-id";
                                        ethernet = <&fec>;

Both the FEC and the CPU port are set to use rgmii-id". So we are
getting double delays.

Try changing one of these to plain rgmii.

    Andrew

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Michal Vokáč]

On 25. 09. 19 2:44, Andrew Lunn wrote:
> On Tue, Sep 24, 2019 at 03:10:44PM +0200, Michal Vokáč wrote:
>> On 24. 09. 19 14:31, Andrew Lunn wrote:
>> I added the printk and the above fix and can confirm that it is the CPU
>> port and the phy is NULL pointer:
>>
>> [    6.976366] qca8k 2188000.ethernet-1:0a: Using legacy PHYLIB callbacks. Please migrate to PHYLINK!
>> [    6.992021] qca8k 2188000.ethernet-1:0a: port: 0, phy: 0
>> [    7.001323] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): PHY [2188000.ethernet-1:01] driver [Generic PHY]
>> [    7.014221] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
>> [    7.030598] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): PHY [2188000.ethernet-1:02] driver [Generic PHY]
>> [    7.043500] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
>> [    7.063335] DSA: tree 0 setup
>>
>> Now the device boots but there is a problem with the CPU port configuration:
> 
> Hi Michal
> 
> Thanks for testing. I will post a different fix very soon.

Thank you, I tested the patch it and it does the business.

>> root@hydraco:~# ifconfig eth0 up
>> [  255.256047] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
>> [  255.272449] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
>> [  255.286539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>> root@hydraco:~# ifconfig eth1 up
>> [  268.350078] qca8k 2188000.ethernet-1:0a: port: 3, phy: -393143296
>> [  268.364442] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
>> [  268.375400] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
>> [  268.393901] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
>> [  268.404849] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
>> [  268.420740] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
>> [  268.432995] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
>> root@hydraco:~# udhcpc -i eth1
>> udhcpc (v1.23.2) started
>> Sending discover...
>> Sending discover...
>> Sending discover...
> 
> This i think is something different. What looks odd is
> imx6dl-yapp4-common.dtsi
> 
> &fec {
>          pinctrl-names = "default";
>          pinctrl-0 = <&pinctrl_enet>;
>          phy-mode = "rgmii-id";
>          phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
>          phy-reset-duration = <20>;
>          phy-supply = <&sw2_reg>;
>          phy-handle = <&ethphy0>;
>          status = "okay";
> 
> 
> 	mdio {
> 
>                 switch@10 {
>                          compatible = "qca,qca8334";
>                          reg = <10>;
> 
>                          switch_ports: ports {
>                                  #address-cells = <1>;
>                                  #size-cells = <0>;
> 
>                                  ethphy0: port@0 {
>                                          reg = <0>;
>                                          label = "cpu";
>                                          phy-mode = "rgmii-id";
>                                          ethernet = <&fec>;
> 
> Both the FEC and the CPU port are set to use rgmii-id". So we are
> getting double delays.
> 
> Try changing one of these to plain rgmii.

Actually, when this file was introduced in [1] I had rgmii-id for the FEC
and rgmii for the CPU port.

Later on the RGMII mode was "fixed" in [2] and that broke the CPU port
for the first time. The problem was fixed by introducing RGMII_ID mode [3]
and using this mode for the CPU port [4].

So now I tried setting rgmii for the FEC but it does not help.
Any other ideas?

Michal

[1] 87489ec3a77f ("ARM: dts: imx: Add Y Soft IOTA Draco, Hydra and Ursa boards")
[2] 5ecdd77c61c8 ("net: dsa: qca8k: disable delay for RGMII mode")
[3] a968b5e9d587 ("net: dsa: qca8k: Enable delay for RGMII_ID mode")
[4] 1a7ee0efb26d ("ARM: dts: imx6dl-yapp4: Use rgmii-id phy mode on the cpu port")

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Heiner Kallweit]

On 24.09.2019 13:27, Michal Vokáč wrote:
> Hi,
> 
> just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
> with QCA8334 switch and got this:
> 
> [    6.957822] 8<--- cut here ---
> [    6.963550] Unable to handle kernel NULL pointer dereference at virtual address 00000264
> [    6.974342] pgd = (ptrval)
> [    6.979751] [00000264] *pgd=00000000
> [    6.986005] Internal error: Oops: 5 [#1] SMP ARM
> [    6.993318] CPU: 0 PID: 21 Comm: kworker/0:1 Not tainted 5.3.0-rc5-00985-g0394a63acfe2 #7
> [    7.004196] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [    7.013467] Workqueue: events deferred_probe_work_func
> [    7.021339] PC is at phy_support_asym_pause+0x14/0x44
> [    7.029149] LR is at qca8k_port_enable+0x40/0x48
> [    7.036485] pc : [<806840f4>]    lr : [<80686724>]    psr: 60000013
> [    7.045489] sp : e821bca0  ip : e821bcb0  fp : e821bcac
> [    7.053456] r10: 00000000  r9 : e8241f00  r8 : e812d040
> [    7.061431] r7 : 00000000  r6 : 00000000  r5 : e8931640  r4 : 00000002
> [    7.070702] r3 : 00000001  r2 : 00000000  r1 : 00000000  r0 : 00000000
> [    7.079947] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    7.089825] Control: 10c5387d  Table: 1000404a  DAC: 00000051
> [    7.098382] Process kworker/0:1 (pid: 21, stack limit = 0x(ptrval))
> [    7.107431] Stack: (0xe821bca0 to 0xe821c000)
> [    7.114607] bca0: e821bccc e821bcb0 80686724 806840ec e812d090 e812d090 e88a02cc 00000000
> [    7.125637] bcc0: e821bce4 e821bcd0 80a74134 806866f0 e812d090 e812d0cc e821bd7c e821bce8
> [    7.136666] bce0: 80a730bc 80a74104 00000000 e88a02cc 00000004 e821bd00 e88a02e0 80cc95f8
> [    7.147725] bd00: e88a02e0 e88a02c0 e812d090 e812d090 81006548 e88a02e4 e812d040 e88a02c0
> [    7.158791] bd20: 00000003 e812d040 e8830c00 00000000 e821bd5c e821bd40 806243b8 80623abc
> [    7.169887] bd40: e8931640 00000000 00000000 5303fd08 e821bd7c e8931640 e8830c00 e8931680
> [    7.181000] bd60: 00000000 81077adc 00000004 00000000 e821bd9c e821bd80 806864f8 80a72930
> [    7.192107] bd80: 81077adc e8830c00 81123b24 00000000 e821bdb4 e821bda0 806856c8 80686354
> [    7.203216] bda0: 81123a18 e8830c00 e821bde4 e821bdb8 8061f944 80685694 00000000 e8830c00
> [    7.214318] bdc0: 81077adc e8830c00 81006548 00000001 810c07f0 00000000 e821be1c e821bde8
> [    7.225434] bde0: 8061ff68 8061f850 e821be04 e821bdf8 81077adc e821be74 81077adc e821be74
> [    7.236589] be00: e8830c00 81006548 00000001 810c07f0 e821be3c e821be20 80620178 8061ff04
> [    7.247788] be20: 00000000 e821be74 806200dc 81006548 e821be6c e821be40 8061e160 806200e8
> [    7.258982] be40: e821be6c e836d96c e86acdb8 5303fd08 e8830c00 81006548 e8830c44 81071194
> [    7.270217] be60: e821bea4 e821be70 8061fd80 8061e104 8061b58c e8830c00 00000001 5303fd08
> [    7.281470] be80: 00000000 e8830c00 81077998 e8830c00 81071194 00000000 e821beb4 e821bea8
> [    7.292770] bea0: 80620220 8061fcac e821bed4 e821beb8 8061e360 80620210 e8830c00 81071180
> [    7.304105] bec0: 81071180 81071194 e821bef4 e821bed8 8061f144 8061e2d8 810711b8 e81f5080
> [    7.315494] bee0: eada5f40 eada7100 e821bf34 e821bef8 80142e2c 8061f0dc e8200d00 ffffe000
> [    7.326890] bf00: e821bf1c e821bf10 8014234c e81f5080 eada5f40 e81f5094 eada5f58 ffffe000
> [    7.338312] bf20: 00000008 81003d00 e821bf74 e821bf38 801433f4 80142c14 ffffe000 80e1264c
> [    7.349778] bf40: 810c00c7 eada5f40 80148afc e81f76c0 e81f7680 00000000 e821a000 e81f5080
> [    7.361246] bf60: 80143130 e8143e74 e821bfac e821bf78 80148e2c 8014313c e81f76dc e81f76dc
> [    7.372758] bf80: e821bfac e81f7680 80148cc4 00000000 00000000 00000000 00000000 00000000
> [    7.384313] bfa0: 00000000 e821bfb0 801010e8 80148cd0 00000000 00000000 00000000 00000000
> [    7.395858] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    7.407363] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [    7.418856] Backtrace:
> [    7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
> [    7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
> [    7.448629]  r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
> [    7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
> [    7.469833]  r5:e812d0cc r4:e812d090
> [    7.476909] [<80a72924>] (dsa_register_switch) from [<806864f8>] (qca8k_sw_probe+0x1b0/0x1c4)
> [    7.489008]  r10:00000000 r9:00000004 r8:81077adc r7:00000000 r6:e8931680 r5:e8830c00
> [    7.500419]  r4:e8931640
> [    7.506528] [<80686348>] (qca8k_sw_probe) from [<806856c8>] (mdio_probe+0x40/0x64)
> [    7.517774]  r7:00000000 r6:81123b24 r5:e8830c00 r4:81077adc
> [    7.527048] [<80685688>] (mdio_probe) from [<8061f944>] (really_probe+0x100/0x2d8)
> [    7.538284]  r5:e8830c00 r4:81123a18
> [    7.545528] [<8061f844>] (really_probe) from [<8061ff68>] (driver_probe_device+0x70/0x180)
> [    7.557539]  r10:00000000 r9:810c07f0 r8:00000001 r7:81006548 r6:e8830c00 r5:81077adc
> [    7.569129]  r4:e8830c00 r3:00000000
> [    7.576439] [<8061fef8>] (driver_probe_device) from [<80620178>] (__device_attach_driver+0x9c/0xc8)
> [    7.589338]  r9:810c07f0 r8:00000001 r7:81006548 r6:e8830c00 r5:e821be74 r4:81077adc
> [    7.600949] [<806200dc>] (__device_attach_driver) from [<8061e160>] (bus_for_each_drv+0x68/0xc8)
> [    7.613617]  r7:81006548 r6:806200dc r5:e821be74 r4:00000000
> [    7.623099] [<8061e0f8>] (bus_for_each_drv) from [<8061fd80>] (__device_attach+0xe0/0x14c)
> [    7.635235]  r7:81071194 r6:e8830c44 r5:81006548 r4:e8830c00
> [    7.644681] [<8061fca0>] (__device_attach) from [<80620220>] (device_initial_probe+0x1c/0x20)
> [    7.657036]  r8:00000000 r7:81071194 r6:e8830c00 r5:81077998 r4:e8830c00
> [    7.667551] [<80620204>] (device_initial_probe) from [<8061e360>] (bus_probe_device+0x94/0x9c)
> [    7.680054] [<8061e2cc>] (bus_probe_device) from [<8061f144>] (deferred_probe_work_func+0x74/0xa0)
> [    7.692914]  r7:81071194 r6:81071180 r5:81071180 r4:e8830c00
> [    7.702440] [<8061f0d0>] (deferred_probe_work_func) from [<80142e2c>] (process_one_work+0x224/0x528)
> [    7.715483]  r7:eada7100 r6:eada5f40 r5:e81f5080 r4:810711b8
> [    7.725046] [<80142c08>] (process_one_work) from [<801433f4>] (worker_thread+0x2c4/0x5e4)
> [    7.737175]  r10:81003d00 r9:00000008 r8:ffffe000 r7:eada5f58 r6:e81f5094 r5:eada5f40
> [    7.748949]  r4:e81f5080
> [    7.755402] [<80143130>] (worker_thread) from [<80148e2c>] (kthread+0x168/0x170)
> [    7.766823]  r10:e8143e74 r9:80143130 r8:e81f5080 r7:e821a000 r6:00000000 r5:e81f7680
> [    7.778636]  r4:e81f76c0
> [    7.785159] [<80148cc4>] (kthread) from [<801010e8>] (ret_from_fork+0x14/0x2c)
> [    7.796457] Exception stack(0xe821bfb0 to 0xe821bff8)
> [    7.805543] bfa0:                                     00000000 00000000 00000000 00000000
> [    7.817805] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    7.830047] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000
> [    7.840688]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:80148cc4
> [    7.852535]  r4:e81f7680
> [    7.859061] Code: e92dd800 e24cb004 e52de004 e8bd4000 (e5902264)
> [    7.869239] ---[ end trace 1b5559d3dc3f80a4 ]---
> 
> Bisecting the dsa code changes since v5.3 brought me to this commit:
> 
> 0394a63acfe2a6e1c08af0eb1a9133ee8650d7bd is the first bad commit
> commit 0394a63acfe2a6e1c08af0eb1a9133ee8650d7bd
> Author: Vivien Didelot <vivien.didelot@gmail.com>
> Date:   Mon Aug 19 16:00:50 2019 -0400
> 
>     net: dsa: enable and disable all ports
>         Call the .port_enable and .port_disable functions for all ports,
>     not only the user ports, so that drivers may optimize the power
>     consumption of all ports after a successful setup.
>         Unused ports are now disabled on setup. CPU and DSA ports are now
>     enabled on setup and disabled on teardown. User ports were already
>     enabled at slave creation and disabled at slave destruction.
>         Signed-off-by: Vivien Didelot <vivien.didelot@gmail.com>
>     Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> :040000 040000 0462b53f03ece23b4af955c3b8a48dce05d90970 34e5df083585e6cce35600698a757508b539e508 M    net
> 
> Any ideas what might be wrong?
> Thank you!
> 
This commit added a call to dsa_port_enable() with the phy_device parameter
being NULL what causes the NPE. Other port_enable callback implementations
may also not check this parameter and face a similar issue.
What I can't answer at a first glance is whether passing NULL is the bug,
or whether port_enable implementations not checking the parameter is the
bug.

> Michal
> 
> 
Heiner

Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause

[Michal Vokáč]

On 24. 09. 19 15:10, Michal Vokáč wrote:
> On 24. 09. 19 14:31, Andrew Lunn wrote:
>> On Tue, Sep 24, 2019 at 01:27:24PM +0200, Michal Vokáč wrote:
>>> Hi,
>>>
>>> just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
>>> with QCA8334 switch and got this:
>>>
>>> [    7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
>>> [    7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
>>> [    7.448629]  r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
>>> [    7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
>>> [    7.469833]  r5:e812d0cc r4:e812d090
>>
>> Hi Michal
>>
>> Please could you add a printk to verify it is the CPU port, and that
>> in qca8k_port_enable() phy is a NULL pointer.
>>
>> I think the fix is going to look something like:
>>
>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
>> index 16f15c93a102..86c80a873e30 100644
>> --- a/drivers/net/dsa/qca8k.c
>> +++ b/drivers/net/dsa/qca8k.c
>> @@ -939,7 +939,8 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>>          qca8k_port_set_status(priv, port, 1);
>>          priv->port_sts[port].enabled = 1;
> 
>       dev_info(priv->dev, "port: %d, phy: %d", port, (u32)phy);
> 
>> -       phy_support_asym_pause(phy);
>> +       if (phy)
>> +               phy_support_asym_pause(phy);
>>          return 0;
>>   }
>>
> 
> I added the printk and the above fix and can confirm that it is the CPU
> port and the phy is NULL pointer:
> 
> [    6.976366] qca8k 2188000.ethernet-1:0a: Using legacy PHYLIB callbacks. Please migrate to PHYLINK!
> [    6.992021] qca8k 2188000.ethernet-1:0a: port: 0, phy: 0
> [    7.001323] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): PHY [2188000.ethernet-1:01] driver [Generic PHY]
> [    7.014221] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [    7.030598] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): PHY [2188000.ethernet-1:02] driver [Generic PHY]
> [    7.043500] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [    7.063335] DSA: tree 0 setup
> 
> Now the device boots but there is a problem with the CPU port configuration:
> 
> root@hydraco:~# ifconfig eth0 up
> [  255.256047] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
> [  255.272449] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> [  255.286539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> root@hydraco:~# ifconfig eth1 up
> [  268.350078] qca8k 2188000.ethernet-1:0a: port: 3, phy: -393143296
> [  268.364442] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
> [  268.375400] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
> [  268.393901] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
> [  268.404849] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
> [  268.420740] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
> [  268.432995] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
> root@hydraco:~# udhcpc -i eth1
> udhcpc (v1.23.2) started
> Sending discover...
> Sending discover...
> Sending discover...

I found and fixed the problem with CPU and USER ports not working.

The issue was in the fact that the qca8k driver incorrectly allocates
the dsa_switch structure with DSA_MAX_PORTS (which is 12) but the QCA8K
switches support up 7 ports.

So the dsa_tree_setup_switches() looped with num_ports set to 12.
Since commit [1] disables all unused ports, ports 7-11 were disabled.
What happened is that some registers were incorrectly rewritten by
the qca8k_port_disable() calls.

I will submit a patch very soon.

Thank you,
Michal

[1] 0394a63acfe2 ("net: dsa: enable and disable all ports")