[PATCH] miiphy: define mii_devs with LIST_HEAD()

[PATCH] miiphy: define mii_devs with LIST_HEAD()

[Weijie Gao]

When enabling net console and console multiplexing, a boot crash was
observed using mtk_eth driver with stdin/stdout set to "serial,nc"
in persistent environment:

> CPU:   MediaTek MT7981
> Model: OpenWrt One
> DRAM:  1 GiB
> Core:  35 devices, 15 uclasses, devicetree: separate
> spi-nand: spi_nand spi_nand@0: Winbond SPI NAND was found.
> spi-nand: spi_nand spi_nand@0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
> Loading Environment from UBI... SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> mtd: partition "ubi" extends beyond the end of device "spi-nand0" -- size truncated to 0x7f00000
> Read 126976 bytes from volume ubootenv to 000000007f7bf0c0
> Read 126976 bytes from volume ubootenv2 to 000000007f7de100
> OK
> "Synchronous Abort" handler, esr 0x96000004, far 0xeafffffeea000018
> elr: 0000000041e63cd4 lr : 0000000041e1b844 (reloc)
> elr: 000000007ff9ecd4 lr : 000000007ff56844
> x0 : eafffffeea000018 x1 : 000000007fb552e0
> x2 : 00000000000000fe x3 : 0000000000000000

The cause is that "serial,nc" forced the console subsystem to
initialize the ethernet driver before ethernet subsystem
initialization (console_init_r() is called before initr_net()).

During the mtk_eth driver initialization, mdio_register() will be
called, and miiphy_get_dev_by_name() will then be called.

The miiphy_get_dev_by_name() will check the list "mii_devs" to see
if the passed device name exists. However the mii_devs is defined
without initialization:
> static struct list_head mii_devs;
and the actual initialization is done in the following chain:
initr_net -> eth_initialize -> eth_common_init -> miiphy_init
Since initr_net() hasn't be called, iterating over the mii_devs
will access to physical address 0 (mii_devs.next == NULL) and will
cause the crash.

The fix is to define mii_devs using:
> static LIST_HEAD(mii_devs);

As the "current_mii" is defined as a static variable, it will
always be NULL in board_r stage and initializing it will NULL is
unnecessary. So the entire miiphy_init() can be remove.

Signed-off-by: Weijie Gao <hackpascal@gmail.com>
---
 common/miiphyutil.c | 12 +-----------
 include/miiphy.h    |  2 --
 net/eth_common.c    |  5 -----
 3 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/common/miiphyutil.c b/common/miiphyutil.c
index 9b8744e5d8..6169ea884a 100644
--- a/common/miiphyutil.c
+++ b/common/miiphyutil.c
@@ -30,7 +30,7 @@
 #define debug(fmt, args...)
 #endif /* MII_DEBUG */
 
-static struct list_head mii_devs;
+static LIST_HEAD(mii_devs);
 static struct mii_dev *current_mii;
 
 /*
@@ -55,16 +55,6 @@ struct mii_dev *miiphy_get_dev_by_name(const char *devname)
 	return NULL;
 }
 
-/*****************************************************************************
- *
- * Initialize global data. Need to be called before any other miiphy routine.
- */
-void miiphy_init(void)
-{
-	INIT_LIST_HEAD(&mii_devs);
-	current_mii = NULL;
-}
-
 struct mii_dev *mdio_alloc(void)
 {
 	struct mii_dev *bus;
diff --git a/include/miiphy.h b/include/miiphy.h
index 5abffd8fb6..8e4b58c734 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -33,8 +33,6 @@ int miiphy_is_1000base_x(const char *devname, unsigned char addr);
 int miiphy_link(const char *devname, unsigned char addr);
 #endif
 
-void miiphy_init(void);
-
 int miiphy_set_current_dev(const char *devname);
 const char *miiphy_get_current_dev(void);
 struct mii_dev *mdio_get_current_dev(void);
diff --git a/net/eth_common.c b/net/eth_common.c
index 89b5bb3718..ba57d836b0 100644
--- a/net/eth_common.c
+++ b/net/eth_common.c
@@ -31,11 +31,6 @@ int eth_env_set_enetaddr_by_index(const char *base_name, int index,
 void eth_common_init(void)
 {
 	bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
-#if CONFIG_IS_ENABLED(ETH)
-#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
-	miiphy_init();
-#endif
-#endif
 }
 
 int eth_mac_skip(int index)
-- 
2.34.1

Re: [PATCH] miiphy: define mii_devs with LIST_HEAD()

[Weijie Gao]

Hi,

Is there any update for this patch?

Thanks

Weijie Gao <hackpascal@gmail.com> 于2025年1月25日周六 23:27写道：
>
> When enabling net console and console multiplexing, a boot crash was
> observed using mtk_eth driver with stdin/stdout set to "serial,nc"
> in persistent environment:
>
> > CPU:   MediaTek MT7981
> > Model: OpenWrt One
> > DRAM:  1 GiB
> > Core:  35 devices, 15 uclasses, devicetree: separate
> > spi-nand: spi_nand spi_nand@0: Winbond SPI NAND was found.
> > spi-nand: spi_nand spi_nand@0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
> > Loading Environment from UBI... SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> > mtd: partition "ubi" extends beyond the end of device "spi-nand0" -- size truncated to 0x7f00000
> > Read 126976 bytes from volume ubootenv to 000000007f7bf0c0
> > Read 126976 bytes from volume ubootenv2 to 000000007f7de100
> > OK
> > "Synchronous Abort" handler, esr 0x96000004, far 0xeafffffeea000018
> > elr: 0000000041e63cd4 lr : 0000000041e1b844 (reloc)
> > elr: 000000007ff9ecd4 lr : 000000007ff56844
> > x0 : eafffffeea000018 x1 : 000000007fb552e0
> > x2 : 00000000000000fe x3 : 0000000000000000
>
> The cause is that "serial,nc" forced the console subsystem to
> initialize the ethernet driver before ethernet subsystem
> initialization (console_init_r() is called before initr_net()).
>
> During the mtk_eth driver initialization, mdio_register() will be
> called, and miiphy_get_dev_by_name() will then be called.
>
> The miiphy_get_dev_by_name() will check the list "mii_devs" to see
> if the passed device name exists. However the mii_devs is defined
> without initialization:
> > static struct list_head mii_devs;
> and the actual initialization is done in the following chain:
> initr_net -> eth_initialize -> eth_common_init -> miiphy_init
> Since initr_net() hasn't be called, iterating over the mii_devs
> will access to physical address 0 (mii_devs.next == NULL) and will
> cause the crash.
>
> The fix is to define mii_devs using:
> > static LIST_HEAD(mii_devs);
>
> As the "current_mii" is defined as a static variable, it will
> always be NULL in board_r stage and initializing it will NULL is
> unnecessary. So the entire miiphy_init() can be remove.
>
> Signed-off-by: Weijie Gao <hackpascal@gmail.com>
> ---
>  common/miiphyutil.c | 12 +-----------
>  include/miiphy.h    |  2 --
>  net/eth_common.c    |  5 -----
>  3 files changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/common/miiphyutil.c b/common/miiphyutil.c
> index 9b8744e5d8..6169ea884a 100644
> --- a/common/miiphyutil.c
> +++ b/common/miiphyutil.c
> @@ -30,7 +30,7 @@
>  #define debug(fmt, args...)
>  #endif /* MII_DEBUG */
>
> -static struct list_head mii_devs;
> +static LIST_HEAD(mii_devs);
>  static struct mii_dev *current_mii;
>
>  /*
> @@ -55,16 +55,6 @@ struct mii_dev *miiphy_get_dev_by_name(const char *devname)
>         return NULL;
>  }
>
> -/*****************************************************************************
> - *
> - * Initialize global data. Need to be called before any other miiphy routine.
> - */
> -void miiphy_init(void)
> -{
> -       INIT_LIST_HEAD(&mii_devs);
> -       current_mii = NULL;
> -}
> -
>  struct mii_dev *mdio_alloc(void)
>  {
>         struct mii_dev *bus;
> diff --git a/include/miiphy.h b/include/miiphy.h
> index 5abffd8fb6..8e4b58c734 100644
> --- a/include/miiphy.h
> +++ b/include/miiphy.h
> @@ -33,8 +33,6 @@ int miiphy_is_1000base_x(const char *devname, unsigned char addr);
>  int miiphy_link(const char *devname, unsigned char addr);
>  #endif
>
> -void miiphy_init(void);
> -
>  int miiphy_set_current_dev(const char *devname);
>  const char *miiphy_get_current_dev(void);
>  struct mii_dev *mdio_get_current_dev(void);
> diff --git a/net/eth_common.c b/net/eth_common.c
> index 89b5bb3718..ba57d836b0 100644
> --- a/net/eth_common.c
> +++ b/net/eth_common.c
> @@ -31,11 +31,6 @@ int eth_env_set_enetaddr_by_index(const char *base_name, int index,
>  void eth_common_init(void)
>  {
>         bootstage_mark(BOOTSTAGE_ID_NET_ETH_START);
> -#if CONFIG_IS_ENABLED(ETH)
> -#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII) || defined(CONFIG_PHYLIB)
> -       miiphy_init();
> -#endif
> -#endif
>  }
>
>  int eth_mac_skip(int index)
> --
> 2.34.1
>

Re: [PATCH] miiphy: define mii_devs with LIST_HEAD()

[Tom Rini]

On Sat, 25 Jan 2025 23:26:32 +0800, Weijie Gao wrote:

> When enabling net console and console multiplexing, a boot crash was
> observed using mtk_eth driver with stdin/stdout set to "serial,nc"
> in persistent environment:
> 
> > CPU:   MediaTek MT7981
> > Model: OpenWrt One
> > DRAM:  1 GiB
> > Core:  35 devices, 15 uclasses, devicetree: separate
> > spi-nand: spi_nand spi_nand@0: Winbond SPI NAND was found.
> > spi-nand: spi_nand spi_nand@0: 128 MiB, block size: 128 KiB, page size: 2048, OOB size: 64
> > Loading Environment from UBI... SF: Detected w25q128 with page size 256 Bytes, erase size 4 KiB, total 16 MiB
> > mtd: partition "ubi" extends beyond the end of device "spi-nand0" -- size truncated to 0x7f00000
> > Read 126976 bytes from volume ubootenv to 000000007f7bf0c0
> > Read 126976 bytes from volume ubootenv2 to 000000007f7de100
> > OK
> > "Synchronous Abort" handler, esr 0x96000004, far 0xeafffffeea000018
> > elr: 0000000041e63cd4 lr : 0000000041e1b844 (reloc)
> > elr: 000000007ff9ecd4 lr : 000000007ff56844
> > x0 : eafffffeea000018 x1 : 000000007fb552e0
> > x2 : 00000000000000fe x3 : 0000000000000000
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom