Skip to content

Conversation

Maihuanyi
Copy link
Contributor

@Maihuanyi Maihuanyi commented Oct 14, 2025

拉取/合并请求描述:(PR description)

[

你的解决方案是什么 (what is your solution)

  • ETH驱动层初始化失败不影响lwip的功能;在ETH驱动层改变link的状态前请检查ETH的device是否初始化成功,否则需要再次初始化一次;
       if (!(xxx_eth_device.parent.parent.flag & RT_DEVICE_FLAG_ACTIVATED))
            {
                if (RT_EOK != xxx_eth_device.parent.parent.init(&xxx_eth_device.parent.parent))
                    return;
            }
            eth_device_linkchange(&xxx_eth_device.parent, RT_TRUE);

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/ALL_BSP_COMPILE.json 详细请参考链接BSP自查

of the lwip layer was incomplete due to the failure of the eth driver layer
initialization;
@github-actions
Copy link

👋 感谢您对 RT-Thread 的贡献!Thank you for your contribution to RT-Thread!

为确保代码符合 RT-Thread 的编码规范,请在你的仓库中执行以下步骤运行代码格式化工作流(如果格式化CI运行失败)。
To ensure your code complies with RT-Thread's coding style, please run the code formatting workflow by following the steps below (If the formatting of CI fails to run).


🛠 操作步骤 | Steps

  1. 前往 Actions 页面 | Go to the Actions page
    点击进入工作流 → | Click to open workflow →

  2. 点击 Run workflow | Click Run workflow

  • 设置需排除的文件/目录(目录请以"/"结尾)
    Set files/directories to exclude (directories should end with "/")
  • 将目标分支设置为 \ Set the target branch to:dev_lwip
  • 设置PR number为 \ Set the PR number to:10810
  1. 等待工作流完成 | Wait for the workflow to complete
    格式化后的代码将自动推送至你的分支。
    The formatted code will be automatically pushed to your branch.

完成后,提交将自动更新至 dev_lwip 分支,关联的 Pull Request 也会同步更新。
Once completed, commits will be pushed to the dev_lwip branch automatically, and the related Pull Request will be updated.

如有问题欢迎联系我们,再次感谢您的贡献!💐
If you have any questions, feel free to reach out. Thanks again for your contribution!

@github-actions
Copy link

📌 Code Review Assignment

🏷️ Tag: components

Reviewers: @Maihuanyi

Changed Files (Click to expand)
  • components/net/lwip/port/ethernetif.c

📊 Current Review Status (Last Updated: 2025-10-14 13:42 CST)


📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

@Maihuanyi
Copy link
Contributor Author

LGTM

@Maihuanyi
Copy link
Contributor Author

@Rbb666

@Rbb666
Copy link
Member

Rbb666 commented Oct 16, 2025

@Ryan-CW-Code 大佬可以协助审核下吗?谢谢

@Rbb666 Rbb666 requested a review from Copilot October 16, 2025 06:07
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an issue where ETH driver layer initialization failures prevent complete lwIP initialization. The solution moves device initialization and opening to occur after network interface setup (DHCP start, netif_set_up), allowing lwIP functionality to proceed even if the ETH driver fails initially.

Key Changes:

  • Device initialization (rt_device_init and rt_device_open) is moved from before netdev registration to after network interface setup
  • Netif flags assignment is changed from direct assignment (=) to bitwise OR (|=) to preserve flags set during setup
  • These changes are applied consistently to both eth_netif_device_init and af_unix_eth_netif_device_init functions

Comment on lines +539 to +543
{
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Returning ERR_IF after network interface is already set up (DHCP started, netif_set_up called) leaves the network stack in an inconsistent state. If device initialization fails at this point, the netif should be cleaned up or the error should be logged without terminating initialization.

错误:在网络接口已经设置完成(DHCP已启动,netif_set_up已调用)后返回ERR_IF会使网络栈处于不一致状态。如果此时设备初始化失败,应该清理netif或记录错误而不是终止初始化。

Suggested change
{
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
{
/* Clean up netif if device init fails */
#if LWIP_DHCP
dhcp_stop(netif);
#endif
netif_set_down(netif);
LWIP_DEBUGF(NETIF_DEBUG, ("ethernetif_init: device init failed, cleaned up netif\n"));
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
/* Clean up netif if device open fails */
#if LWIP_DHCP
dhcp_stop(netif);
#endif
netif_set_down(netif);
LWIP_DEBUGF(NETIF_DEBUG, ("ethernetif_init: device open failed, cleaned up netif\n"));

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这样的目的是为网络协议栈和硬件分开初始化,当硬件的link状态改变后保证lwip功能完整;

Comment on lines +722 to +726
{
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Same issue as Comment 1 - returning ERR_IF after netif_set_up leaves the network interface in an inconsistent state. This contradicts the PR's stated goal of allowing lwIP to function even when ETH driver initialization fails.

错误:与评论1相同的问题 - 在netif_set_up之后返回ERR_IF会使网络接口处于不一致状态。这与PR声明的"ETH驱动层初始化失败不影响lwip的功能"目标相矛盾。

Suggested change
{
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
{
netif_set_down(netif);
return ERR_IF;
}
if (rt_device_open(device, RT_DEVICE_FLAG_RDWR) != RT_EOK)
{
netif_set_down(netif);

Copilot uses AI. Check for mistakes.

{
return ERR_IF;
}
netif->flags |= (ethif->flags & 0xff);
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Using |= operator after device initialization may incorrectly combine flags. If rt_device_init or rt_device_open modifies ethif->flags, this could set unintended flag bits. The original assignment at line 494 (netif->flags = (ethif->flags & 0xff)) should be removed to avoid setting flags twice with different operators.

错误:在设备初始化后使用|=运算符可能会错误地组合标志。如果rt_device_initrt_device_open修改了ethif->flags,这可能会设置意外的标志位。应该删除第494行的原始赋值(netif->flags = (ethif->flags & 0xff))以避免使用不同运算符两次设置标志。

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

状态已改变了,需要再次同步一次

return ERR_IF;
}

netif->flags |= (ethif->flags & 0xff);
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Same issue as Comment 3 - duplicate flag assignment with different operators. Line 682 sets netif->flags = (ethif->flags & 0xff) and line 730 uses |=, which may produce incorrect flag combinations.

错误:与评论3相同的问题 - 使用不同运算符重复赋值标志。第682行设置netif->flags = (ethif->flags & 0xff),第730行使用|=,这可能产生错误的标志组合。

Suggested change
netif->flags |= (ethif->flags & 0xff);
/* netif->flags |= (ethif->flags & 0xff); -- removed duplicate assignment */

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

同上

@Maihuanyi
Copy link
Contributor Author

@Ryan-CW-Code 大佬可以看看这个PR吗?

}

/* copy device flags to netif flags */
netif->flags = (ethif->flags & 0xff);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

佬,这部分可以延续你的思路,稍微更改一下,我没用过gd32,提供一个参考。
考虑到可能有其他 BSP 代码依赖 init 后的 flags 来配置 lwIP 的参数,我们尽量不要去改 lwIP port 层的逻辑。
在 GD32 上,可以模拟实现类似 STM32 ETH 驱动的效果,rt_device_init 阶段不直接调用 enet_init,而是只做 PHY、MAC 等不会触发链路状态检查的基础配置。
在 PHY 链路检测函数里,再判断 enet_init 是否已经执行过,如果还没有,就在链路 up 的时候补一次初始化。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

几天前review的,忘记提交了

@Maihuanyi Maihuanyi closed this Oct 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants