netdev - Re: [PATCH net-next v2 1/2] net: stmmac: Return early if invalid in loongson_dwmac_fix

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5ef8ae99-256e-8ff7-861f-025e7b7cfb6f@loongson.cn>
Date: Thu, 24 Jul 2025 17:05:57 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Andrew Lunn <andrew@...n.ch>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
 "David S . Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/2] net: stmmac: Return early if invalid in
 loongson_dwmac_fix_reset()

On 2025/7/24 上午10:26, Tiezhu Yang wrote:
> On 2025/7/23 下午10:53, Andrew Lunn wrote:
>> On Wed, Jul 23, 2025 at 06:00:55PM +0800, Tiezhu Yang wrote:
>>> If the MAC controller does not connect to any PHY interface, there is a
>>> missing clock, then the DMA reset fails.

...

>>> +    if (value & DMA_BUS_MODE_SFT_RESET)
>>> +        return -EINVAL;
>>
>> What happens with this return value? Do you get an error message which
>> gives a hint the PHY clock is missing? Would a netdev_err() make sense
>> here?
> 
> Yes, I will use dev_err() rather than netdev_err() (because there is no
> net_device member here) to do something like this:
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index 6d10077666c7..4a7b2b11ecce 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -513,8 +513,11 @@ static int loongson_dwmac_fix_reset(void *priv, 
> void __iomem *ioaddr)
>   {
>          u32 value = readl(ioaddr + DMA_BUS_MODE);
> 
> -       if (value & DMA_BUS_MODE_SFT_RESET)
> +       if (value & DMA_BUS_MODE_SFT_RESET) {
> +               struct plat_stmmacenet_data *plat = priv;
> +               dev_err(&plat->pdev->dev, "the PHY clock is missing\n");
>                  return -EINVAL;
> +       }
> 
>          value |= DMA_BUS_MODE_SFT_RESET;
>          writel(value, ioaddr + DMA_BUS_MODE);

Oops, the above changes can not work well.

It can not use netdev_err() or dev_err() to print message with device info
in loongson_dwmac_fix_reset() directly, this is because the type of "priv"
argument is struct plat_stmmacenet_data and the "pdev" member of "priv" is
NULL here, it will lead to the fatal error "Unable to handle kernel paging
request at virtual address" when printing message.

Based on the above analysis, in order to show an error message which gives
a hint the PHY clock is missing, it is proper to check the return value of
stmmac_reset() which calls loongson_dwmac_fix_reset().

With this patch, for the normal end user, the computer start faster with
reducing boot time for 2 seconds on the specified mainboard.

The final changes look something like this:

----->8-----
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
index e1591e6217d4..6d10077666c7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
@@ -513,6 +513,9 @@ static int loongson_dwmac_fix_reset(void *priv, void 
__iomem *ioaddr)
  {
         u32 value = readl(ioaddr + DMA_BUS_MODE);

+       if (value & DMA_BUS_MODE_SFT_RESET)
+               return -EINVAL;
+
         value |= DMA_BUS_MODE_SFT_RESET;
         writel(value, ioaddr + DMA_BUS_MODE);

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c 
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b948df1bff9a..1a2610815847 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3133,6 +3133,9 @@ static int stmmac_init_dma_engine(struct 
stmmac_priv *priv)

         ret = stmmac_reset(priv, priv->ioaddr);
         if (ret) {
+               if (ret == -EINVAL)
+                       netdev_err(priv->dev, "the PHY clock is missing\n");
+
                 netdev_err(priv->dev, "Failed to reset the dma\n");
                 return ret;
         }

I will wait for more comments and send v3 after the merge window.

Thanks,
Tiezhu