Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

udev: net_id: Use devicetree aliases when available #24265

Merged
merged 1 commit into from Aug 12, 2022

Conversation

sean-anderson-seco
Copy link
Contributor

@sean-anderson-seco sean-anderson-seco commented Aug 9, 2022

Devicetree firmware contains an "aliases" node, containing various
aliases for devices described by the firmware. For ethernet devices,
these are named "ethernet0", "ethernet1", etc. They provide a convenient
means of numbering ethernet devices, especially on systems with no other
stable number other than the address. In particular, U-Boot already uses
these aliases to name its ethernet devices.

Previously, there have been attempts (such as [1]) to add support for
these aliases to Linux. However, these patches have been rejected
because it is the maintainers' view that naming policy be left to
userspace. Well, systemd is userspace, so here we are.

In terms of implementation, apparently there can be multiple device
trees at once. I have decided to dodge this problem for now, and just
use /proc/device-tree. If it is desired to support multiple device trees
later, then the scheme can be modified to something like enb2d3.

For the moment we only support "ethernetX" aliases. Future patches might
want to also handle "canX" and "wifiX".

It is common on boards with only one ethernet device to use an alias of
just "ethernet". In this case, the index is an implicit 0. In case the
author of the firmware made a mistake, we check to ensure that aliases
of "ethernet" and "ethernet0" do not both exist.

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/1399390594-1409-1-git-send-email-boris.brezillon@free-electrons.com/

Signed-off-by: Sean Anderson sean.anderson@seco.com

Closes #17625.

@github-actions github-actions bot added the udev label Aug 9, 2022
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
@yuwata yuwata added new-feature reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Aug 9, 2022
@yuwata
Copy link
Member

yuwata commented Aug 9, 2022

BTW, is this for RFE #17625? If so, please add Closes #17625. in the commit message.
Also, please drop signed-off-by tag in the commit message. We do not use it.

@yuwata
Copy link
Member

yuwata commented Aug 9, 2022

In terms of implementation, apparently there can be multiple device
trees at once. I have decided to dodge this problem for now, and just
use /proc/device-tree. If it is desired to support multiple device trees
later, then the scheme can be modified to something like enb2d3.

What is enb2d3??

@sean-anderson-seco
Copy link
Contributor Author

sean-anderson-seco commented Aug 9, 2022

Thank you for the quick review.

BTW, is this for RFE #17625? If so, please add Closes #17625. in the commit message.

I did not write this specifically for that issue, but it looks like this might close that (depending on whether there is an appropriate alias). I will add that tag.

Also, please drop signed-off-by tag in the commit message. We do not use it.

OK (as it happened I quickly went through the history of this file and saw it used in d6eda67).

In terms of implementation, apparently there can be multiple device
trees at once. I have decided to dodge this problem for now, and just
use /proc/device-tree. If it is desired to support multiple device trees
later, then the scheme can be modified to something like enb2d3.

What is enb2d3??

According to the sysfs ABI documentation, there can be multiple devicetrees, but it does not document the naming scheme used. The actual attachment is done by __of_attach_node_sysfs, which uses safe_name to create the new top-level tree. I believe this results in something like base, base1, ..., base15. So the idea is if we needed to support this we could use b for "base".

@sean-anderson-seco sean-anderson-seco force-pushed the netdev_aliases branch 3 times, most recently from a34339e to 57f39cf Compare August 9, 2022 22:42
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

Looks much better now. One more round please.

src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. Only several coding style comments.

src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
src/udev/udev-builtin-net_id.c Outdated Show resolved Hide resolved
Devicetree firmware contains an "aliases" node, containing various
aliases for devices described by the firmware. For ethernet devices,
these are named "ethernet0", "ethernet1", etc. They provide a convenient
means of numbering ethernet devices, especially on systems with no other
stable number other than the address. In particular, U-Boot already uses
these aliases to name its ethernet devices.

Previously, there have been attempts (such as [1]) to add support for
these aliases to Linux. However, these patches have been rejected
because it is the maintainers' view that naming policy be left to
userspace. Well, systemd is userspace, so here we are.

In terms of implementation, apparently there can be multiple device
trees at once. I have decided to dodge this problem for now, and just
use /proc/device-tree. If it is desired to support multiple device trees
later, then the scheme can be modified to include the device tree's
index. For example, /sys/firmware/devicetree/base2/aliases/ethernet3
might be named enb2d3.

For the moment we only support "ethernetX" aliases. Future patches might
want to also handle "canX" and "wifiX".

It is common on boards with only one ethernet device to use an alias of
just "ethernet". In this case, the index is an implicit 0. In case the
author of the firmware made a mistake, we check to ensure that aliases
of "ethernet" and "ethernet0" do not both exist.

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/1399390594-1409-1-git-send-email-boris.brezillon@free-electrons.com/

Closes: systemd#17625
@yuwata yuwata removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 11, 2022
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@yuwata yuwata added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Aug 11, 2022
@poettering poettering merged commit 65c2ad9 into systemd:main Aug 12, 2022
@yuwata yuwata removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

udevd net_id builtin does not identify platform eth on Raspberry Pi 4b
3 participants