ksz9893 with FEC driver on 4.19.35 kernel

[cedricje]

Hello,

We have a board with an iMX7 connected to a ksz9893. i2c is used for register access. I've integrated the files under KSZ/linux-drivers/ksz9897/linux-4.19 in our 4.19.35 kernel (taken from the linux-imx). When compiling the i2c_ksz9897 driver as a module it worked. Now i'm compiling it statically in the kernel and there are issues:

• we don't receive packets anymore. (i see the FEC doesn't receive interrupts anymore)
• when bringing eth0 down with eg, ifconfig, this hangs.

There are differences between loading the module statically or dynamically.

• when loading it dynamically, the fec driver will have attached itself to the fixed generic phy driver before the ksz9897 driver is modprobed. In case of the static loading, it attaches itself to the ksz phy driver. See the dmesg output: [ 19.795300] Microchip KSZ9893 Switch sw.0:01: attached PHY driver [Microchip KSZ9893 Switch] (mii_bus:phy_addr=sw.0:01, irq=81)
• when dynamically loading the module fep->port.sw will be NULL, with static compilation this is filled in. The code takes different paths based on this.

Is it normal for the ksz phy driver to be used for the cpu port? I'd expect this to be a fixed-link (as we have configure in our dts file). And have the ksz phy driver used for the switch ports. But we're not using multi_dev, so that's not applicable.

thanks, Cedric

[triha2work]

There is an order in driver loading. The I2C drivers should be loaded first. The subsys_initcall in the driver should give the driver higher priority in loading. Please check the switch driver is run first before the MAC driver.

[cedricje]

Yes, the switch driver is loaded first. But is it normal the ksz9897 phy driver is used for eth0 when this is a fixed RGMII link, and we're not using multi_dev?

I've done some more investigation, and found out fec_enet_adjust_link is being called from link_update_work, but before that call has returned fec_enet_adjust_link is also called from phy_state_machine. I think phydev->lock should be taken in link_update_work, but i'm not sure a driver is supposed to call phydev->adjust_link directly.

[triha2work]

Then why there is a difference when the driver is loaded statically instead of dynamically? When the switch driver is used the regular PHY device mechanism will not be used. The PHY device is connected through phy_attach instead of phy_connect, and the phy_start function will not be called. The switch driver detects link change through interrupt or polling and notifies the MAC driver through adjust_link. Newer kernels will notify the MAC driver from phy_state_machine no matter what; the switch driver should override the netdevice link status after that. The "multi_dev=0" setting should be used first to eliminate all issues related to MAC driver porting before more complex settings are used.

[cedricje]

I wasn't clear. When loading dynamically, the switch driver is loaded after the mac driver (the fec driver was still compiled statically). As this isn't correct, i stopped looking at this and continued looking at what's wrong when compiling statically.

I've added a lock in link_update_work to avoid the race condition with phy_state_machine:

diff --git a/drivers/net/ethernet/micrel/ksz_sw_9897.c b/drivers/net/ethernet/micrel/ksz_sw_9897.c                                                                                                                
index d2ea1ffa1e9d..2b33484c4a8b 100644
--- a/drivers/net/ethernet/micrel/ksz_sw_9897.c
+++ b/drivers/net/ethernet/micrel/ksz_sw_9897.c
@@ -15818,6 +15818,7 @@ static void link_update_work(struct work_struct *work)

                /* phydev settings may be changed by ethtool. */
                info = get_port_info(sw, sw->HOST_PORT);
+               mutex_lock(&phydev->lock);
                phydev->link = (info->state == media_connected);
                phydev->speed = info->tx_rate / TX_RATE_UNIT;
                phydev->duplex = (info->duplex == 2);
@@ -15837,6 +15838,7 @@ static void link_update_work(struct work_struct *work)
                        phydev->adjust_link(phydev->attached_dev);
                        info->report = false;
                }
+               mutex_unlock(&phydev->lock);
 #endif

                /* Carrier may be turned on in the adjust_link function. */

Then it hangs in napi_disable called from fec_enet_adjust_link. I think this happens when napi_enable hasn't been called before. And in fec_enet_open, the call to napi_enable isn't done when it's a switch:

#ifdef HAVE_KSZ_SWITCH
     if (!sw_is_switch(sw))
#endif
        napi_enable(&fep->napi);

[triha2work]

The MAC driver needs to be modified/patched to use the switch driver. If not then the switch driver has no effect other than allowing register access. The switch driver creates a virtual PHY device for the MAC driver to use. The regular PHY operation then will not be used. I am not sure the PHY state machine will be invoked continually unless the PHY interrupt is not specified. I am not sure the patched MAC driver is correct. Please check the linux-drivers/imx directory which contains the original fec_main.c and the patch to create a new one.

[cedricje]

The fec_main.c and fec.h have been patched with the patch files in KSZ/linux-drivers/ksz9897/linux-4.19/drivers/net/ethernet/freescale Do you know a kernel version (maybe 4.14?) the fec patch is working on?

[triha2work]

As I mentioned there is an imx directory containing patches for fec_main.c in different kernels. There are 2 versions of linux 4.14, one from imx and the other from generic kernel.org. The generic 4.14 version is quite old though: 4.14.39. It seems the newer 4.14.150 and so added so much changes that the patch will not apply completely. Anyway all patches for different kernels are about the same. If you compare the original file and the new one visually you can see the switch driver modifications and can easily add them to any fec_main.c code.