| Message ID | 20240902104433.2561797-4-niklas.cassel@wdc.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | rock5b additional improvements | expand |
On Mon, 2 Sep 2024 12:44:31 +0200 Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: > From: Niklas Cassel <cassel@kernel.org> > > The arm64 defconfig enables both the R8169 Ethernet driver and the > PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. > > Enable udev so that these modules will get automatically loaded by udev > after the rootfs is mounted from the SD card. > > This way, we do not need to build these modules as built-in, and we also > get the benefit that any device plugged in to the PCIe slot will get > automatically loaded. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> I'm not sure what we want to do here, especially when this ends up needing net.ifnames=0 that you add in PATCH 4/5. We do have already a small number of defconfigs that enable eudev. So far we only use net.ifnames=0 in 4 configurations. Basically, I'm trying to make sure we remain consistent and have a policy that applies to all defconfigs rather than ad-hoc solutions that are different for each and every defconfig. That's why I'm a bit reluctant with applying your patches as-is because they only solve the specific case of this defconfig, in a specific way, without creating a clear rule that all defconfigs meeting a specific situation should follow. Thomas
On 03/09/2024 21:26, Thomas Petazzoni via buildroot wrote: > On Mon, 2 Sep 2024 12:44:31 +0200 > Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: > >> From: Niklas Cassel <cassel@kernel.org> >> >> The arm64 defconfig enables both the R8169 Ethernet driver and the >> PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. >> >> Enable udev so that these modules will get automatically loaded by udev >> after the rootfs is mounted from the SD card. >> >> This way, we do not need to build these modules as built-in, and we also >> get the benefit that any device plugged in to the PCIe slot will get >> automatically loaded. >> >> Signed-off-by: Niklas Cassel <cassel@kernel.org> > > I'm not sure what we want to do here, especially when this ends up > needing net.ifnames=0 that you add in PATCH 4/5. > > We do have already a small number of defconfigs that enable eudev. So > far we only use net.ifnames=0 in 4 configurations. > > Basically, I'm trying to make sure we remain consistent and have a > policy that applies to all defconfigs rather than ad-hoc solutions that > are different for each and every defconfig. That's why I'm a bit > reluctant with applying your patches as-is because they only solve the > specific case of this defconfig, in a specific way, without creating a > clear rule that all defconfigs meeting a specific situation should > follow. We don't really have a policy or rules at the moment. I've added this to the topics of the Buildroot Developer meeting in Vienna. For the time being, I think we generally prefer to have a kernel config that is as much as possible custom for the target board. Thus, we also generally prefer to not require loading modules. Certainly for ethernet - having ethernet as a module makes nfsroot impossible. So I've marked patches 3/5 and 4/5 as Rejected. Feel free to give arguments why we should prefer these drivers as module though! Regards, Arnout
On Tue, Sep 03, 2024 at 09:26:11PM +0200, Thomas Petazzoni wrote: > On Mon, 2 Sep 2024 12:44:31 +0200 > Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: > > > From: Niklas Cassel <cassel@kernel.org> > > > > The arm64 defconfig enables both the R8169 Ethernet driver and the > > PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. > > > > Enable udev so that these modules will get automatically loaded by udev > > after the rootfs is mounted from the SD card. > > > > This way, we do not need to build these modules as built-in, and we also > > get the benefit that any device plugged in to the PCIe slot will get > > automatically loaded. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > I'm not sure what we want to do here, especially when this ends up > needing net.ifnames=0 that you add in PATCH 4/5. > > We do have already a small number of defconfigs that enable eudev. So > far we only use net.ifnames=0 in 4 configurations. > > Basically, I'm trying to make sure we remain consistent and have a > policy that applies to all defconfigs rather than ad-hoc solutions that > are different for each and every defconfig. That's why I'm a bit > reluctant with applying your patches as-is because they only solve the > specific case of this defconfig, in a specific way, without creating a > clear rule that all defconfigs meeting a specific situation should > follow. (Replying with the email that I have subscribed to the list this time.) An alternative to specify net.ifnames=0 on the kernel command line (to disable udev predictable NIC names), is to create an empty file: /etc/udev/rules.d/80-net-name-slot.rules See: https://wiki.gentoo.org/wiki/Udev#Optional:_Disable_or_override_predictable_network_interface_naming I guess we could modify package/eudev/eudev.mk and package/eudev/Config.in to create a new BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES Kconfig which we default to "not set". If BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES is "not set", we install/touch /etc/udev/rules.d/80-net-name-slot.rules in the target rootfs. That would avoid the need for net.ifnames=0. We could also remove it for the existing boards that have it set. Any board that actually wants predictable NIC names can set the new BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES Kconfig. Thoughts? Kind regards, Niklas
On Tue, Sep 03, 2024 at 09:50:53PM +0200, Arnout Vandecappelle wrote: > > > On 03/09/2024 21:26, Thomas Petazzoni via buildroot wrote: > > On Mon, 2 Sep 2024 12:44:31 +0200 > > Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: > > > > > From: Niklas Cassel <cassel@kernel.org> > > > > > > The arm64 defconfig enables both the R8169 Ethernet driver and the > > > PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. > > > > > > Enable udev so that these modules will get automatically loaded by udev > > > after the rootfs is mounted from the SD card. > > > > > > This way, we do not need to build these modules as built-in, and we also > > > get the benefit that any device plugged in to the PCIe slot will get > > > automatically loaded. > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > > I'm not sure what we want to do here, especially when this ends up > > needing net.ifnames=0 that you add in PATCH 4/5. > > > > We do have already a small number of defconfigs that enable eudev. So > > far we only use net.ifnames=0 in 4 configurations. > > > > Basically, I'm trying to make sure we remain consistent and have a > > policy that applies to all defconfigs rather than ad-hoc solutions that > > are different for each and every defconfig. That's why I'm a bit > > reluctant with applying your patches as-is because they only solve the > > specific case of this defconfig, in a specific way, without creating a > > clear rule that all defconfigs meeting a specific situation should > > follow. > > We don't really have a policy or rules at the moment. I've added this to > the topics of the Buildroot Developer meeting in Vienna. See my reply to Thomas where I suggest to create a BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES Kconfig which is default not set. I think that would be a nice policy / solution. > > For the time being, I think we generally prefer to have a kernel config > that is as much as possible custom for the target board. Thus, we also > generally prefer to not require loading modules. Certainly for ethernet - > having ethernet as a module makes nfsroot impossible. > > So I've marked patches 3/5 and 4/5 as Rejected. > > Feel free to give arguments why we should prefer these drivers as module though! Sure, we could keep the Ethernet driver as built-in. I still think it is a really good idea to enable udev though. rock5b is currently using arm64 defconfig as kernel config. Just enabling udev shows this in lsmod: # lsmod Module Size Used by Not tainted hantro_vpu 270336 0 rockchipdrm 167936 0 v4l2_jpeg 16384 1 hantro_vpu v4l2_vp9 20480 1 hantro_vpu v4l2_h264 12288 1 hantro_vpu analogix_dp 36864 1 rockchipdrm v4l2_mem2mem 36864 1 hantro_vpu dw_mipi_dsi 20480 1 rockchipdrm panthor 118784 0 videobuf2_dma_contig 16384 1 hantro_vpu dw_hdmi 40960 1 rockchipdrm videobuf2_memops 12288 1 videobuf2_dma_contig cec 53248 1 dw_hdmi drm_gpuvm 32768 1 panthor videobuf2_v4l2 28672 2 hantro_vpu,v4l2_mem2mem drm_exec 12288 2 panthor,drm_gpuvm drm_display_helper 167936 3 rockchipdrm,analogix_dp,dw_hdmi drm_shmem_helper 24576 1 panthor drm_dma_helper 20480 1 rockchipdrm crct10dif_ce 12288 1 videodev 253952 3 hantro_vpu,v4l2_mem2mem,videobuf2_v4l2 rockchip_saradc 16384 0 gpu_sched 40960 1 panthor videobuf2_common 57344 5 hantro_vpu,v4l2_mem2mem,videobuf2_dma_contig,videobuf2_memops,videobuf2_v4l2 drm_kms_helper 188416 7 rockchipdrm,analogix_dp,dw_mipi_dsi,dw_hdmi,drm_display_helper,drm_shmem_helper,drm_dma_helper mc 57344 5 hantro_vpu,v4l2_mem2mem,videobuf2_v4l2,videodev,videobuf2_common drm 552960 12 rockchipdrm,analogix_dp,dw_mipi_dsi,panthor,dw_hdmi,drm_gpuvm,drm_exec,drm_display_helper,drm_shmem_helper,drm_dma_helper,gpu_sched,drm_kms_helper industrialio_triggered_buffer 12288 1 rockchip_saradc backlight 20480 2 drm_display_helper,drm phy_rockchip_usbdp 20480 2 phy_rockchip_naneng_combphy 16384 5 kfifo_buf 12288 1 industrialio_triggered_buffer pwm_fan 16384 0 rtc_hym8563 12288 1 snd_soc_audio_graph_card 16384 0 typec 77824 1 phy_rockchip_usbdp snd_soc_rockchip_i2s_tdm 20480 2 snd_soc_simple_card_utils 28672 1 snd_soc_audio_graph_card snd_soc_es8316 40960 1 rockchip_thermal 24576 0 spi_rockchip_sfc 12288 0 rk805_pwrkey 12288 0 Without udev (i.e. with the current buildroot rock5b_defconfig) none of these modules will be loaded. Kind regards, Niklas
On 03/09/2024 21:56, Niklas Cassel wrote: > On Tue, Sep 03, 2024 at 09:26:11PM +0200, Thomas Petazzoni wrote: >> On Mon, 2 Sep 2024 12:44:31 +0200 >> Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: >> >>> From: Niklas Cassel <cassel@kernel.org> >>> >>> The arm64 defconfig enables both the R8169 Ethernet driver and the >>> PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. >>> >>> Enable udev so that these modules will get automatically loaded by udev >>> after the rootfs is mounted from the SD card. >>> >>> This way, we do not need to build these modules as built-in, and we also >>> get the benefit that any device plugged in to the PCIe slot will get >>> automatically loaded. >>> >>> Signed-off-by: Niklas Cassel <cassel@kernel.org> >> >> I'm not sure what we want to do here, especially when this ends up >> needing net.ifnames=0 that you add in PATCH 4/5. >> >> We do have already a small number of defconfigs that enable eudev. So >> far we only use net.ifnames=0 in 4 configurations. >> >> Basically, I'm trying to make sure we remain consistent and have a >> policy that applies to all defconfigs rather than ad-hoc solutions that >> are different for each and every defconfig. That's why I'm a bit >> reluctant with applying your patches as-is because they only solve the >> specific case of this defconfig, in a specific way, without creating a >> clear rule that all defconfigs meeting a specific situation should >> follow. > > (Replying with the email that I have subscribed to the list this time.) > > > An alternative to specify net.ifnames=0 on the kernel command line > (to disable udev predictable NIC names), is to create an empty file: > /etc/udev/rules.d/80-net-name-slot.rules Well, I think Thomas' comment was primarily about whether we should enable eudev and dynamically load modules rather than using builtins. Passing net.ifnames=0 is probably the easiest way to disable predictable NIC names. Only, it's surprising that we have something like 25 defconfigs that enable eudev, but only 4 of them enable that option... We should preferably have one "normal" way that this is done. What is BTW not clear to me is _why_ you don't want predictable names. What's wrong with BR2_SYSTEM_DHCP="enp0s31f6"? Regards, Arnout > > See: > https://wiki.gentoo.org/wiki/Udev#Optional:_Disable_or_override_predictable_network_interface_naming > > I guess we could modify package/eudev/eudev.mk and package/eudev/Config.in > to create a new BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES Kconfig which we > default to "not set". > > If BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES is "not set", we install/touch > /etc/udev/rules.d/80-net-name-slot.rules in the target rootfs. > > That would avoid the need for net.ifnames=0. We could also remove it for > the existing boards that have it set. > > Any board that actually wants predictable NIC names can set the new > BR2_PACKAGE_EUDEV_PREDICTABLE_NIC_NAMES Kconfig. > > Thoughts? > > > Kind regards, > Niklas
On 03/09/2024 22:06, Niklas Cassel wrote: > On Tue, Sep 03, 2024 at 09:50:53PM +0200, Arnout Vandecappelle wrote: >> >> [snip] >> For the time being, I think we generally prefer to have a kernel config >> that is as much as possible custom for the target board. Thus, we also >> generally prefer to not require loading modules. Certainly for ethernet - >> having ethernet as a module makes nfsroot impossible. >> >> So I've marked patches 3/5 and 4/5 as Rejected. >> >> Feel free to give arguments why we should prefer these drivers as module though! > > Sure, we could keep the Ethernet driver as built-in. > > I still think it is a really good idea to enable udev though. > > rock5b is currently using arm64 defconfig as kernel config. > > > Just enabling udev shows this in lsmod: > > # lsmod > Module Size Used by Not tainted > hantro_vpu 270336 0 > rockchipdrm 167936 0 > v4l2_jpeg 16384 1 hantro_vpu > v4l2_vp9 20480 1 hantro_vpu > v4l2_h264 12288 1 hantro_vpu > analogix_dp 36864 1 rockchipdrm > v4l2_mem2mem 36864 1 hantro_vpu > dw_mipi_dsi 20480 1 rockchipdrm > panthor 118784 0 > videobuf2_dma_contig 16384 1 hantro_vpu > dw_hdmi 40960 1 rockchipdrm > videobuf2_memops 12288 1 videobuf2_dma_contig > cec 53248 1 dw_hdmi > drm_gpuvm 32768 1 panthor > videobuf2_v4l2 28672 2 hantro_vpu,v4l2_mem2mem > drm_exec 12288 2 panthor,drm_gpuvm > drm_display_helper 167936 3 rockchipdrm,analogix_dp,dw_hdmi > drm_shmem_helper 24576 1 panthor > drm_dma_helper 20480 1 rockchipdrm > crct10dif_ce 12288 1 > videodev 253952 3 hantro_vpu,v4l2_mem2mem,videobuf2_v4l2 > rockchip_saradc 16384 0 > gpu_sched 40960 1 panthor > videobuf2_common 57344 5 hantro_vpu,v4l2_mem2mem,videobuf2_dma_contig,videobuf2_memops,videobuf2_v4l2 > drm_kms_helper 188416 7 rockchipdrm,analogix_dp,dw_mipi_dsi,dw_hdmi,drm_display_helper,drm_shmem_helper,drm_dma_helper > mc 57344 5 hantro_vpu,v4l2_mem2mem,videobuf2_v4l2,videodev,videobuf2_common > drm 552960 12 rockchipdrm,analogix_dp,dw_mipi_dsi,panthor,dw_hdmi,drm_gpuvm,drm_exec,drm_display_helper,drm_shmem_helper,drm_dma_helper,gpu_sched,drm_kms_helper > industrialio_triggered_buffer 12288 1 rockchip_saradc > backlight 20480 2 drm_display_helper,drm > phy_rockchip_usbdp 20480 2 > phy_rockchip_naneng_combphy 16384 5 > kfifo_buf 12288 1 industrialio_triggered_buffer > pwm_fan 16384 0 > rtc_hym8563 12288 1 > snd_soc_audio_graph_card 16384 0 > typec 77824 1 phy_rockchip_usbdp > snd_soc_rockchip_i2s_tdm 20480 2 > snd_soc_simple_card_utils 28672 1 snd_soc_audio_graph_card > snd_soc_es8316 40960 1 > rockchip_thermal 24576 0 > spi_rockchip_sfc 12288 0 > rk805_pwrkey 12288 0 > > Without udev (i.e. with the current buildroot rock5b_defconfig) > none of these modules will be loaded. That is a very good reaosn to want eudev, indeed! The alternative is to extend the linux config fragment to include all of these as builtins, but I guess that's not even possible for hantro_vpu... Then another question is whether we should use eudev or mdev. We currently have 25 confgs that enable mdev and 30 that enable eudev. Some consistency there would be nice as well! Regards, Arnout
On Tue, Sep 03, 2024 at 11:06:13PM +0200, Arnout Vandecappelle wrote: > > > On 03/09/2024 21:56, Niklas Cassel wrote: > > On Tue, Sep 03, 2024 at 09:26:11PM +0200, Thomas Petazzoni wrote: > > > On Mon, 2 Sep 2024 12:44:31 +0200 > > > Niklas Cassel via buildroot <buildroot@buildroot.org> wrote: > > > > > > > From: Niklas Cassel <cassel@kernel.org> > > > > > > > > The arm64 defconfig enables both the R8169 Ethernet driver and the > > > > PHY_ROCKCHIP_NANENG_COMBO_PHY PHY driver as modules. > > > > > > > > Enable udev so that these modules will get automatically loaded by udev > > > > after the rootfs is mounted from the SD card. > > > > > > > > This way, we do not need to build these modules as built-in, and we also > > > > get the benefit that any device plugged in to the PCIe slot will get > > > > automatically loaded. > > > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > > > > I'm not sure what we want to do here, especially when this ends up > > > needing net.ifnames=0 that you add in PATCH 4/5. > > > > > > We do have already a small number of defconfigs that enable eudev. So > > > far we only use net.ifnames=0 in 4 configurations. > > > > > > Basically, I'm trying to make sure we remain consistent and have a > > > policy that applies to all defconfigs rather than ad-hoc solutions that > > > are different for each and every defconfig. That's why I'm a bit > > > reluctant with applying your patches as-is because they only solve the > > > specific case of this defconfig, in a specific way, without creating a > > > clear rule that all defconfigs meeting a specific situation should > > > follow. > > > > (Replying with the email that I have subscribed to the list this time.) > > > > > > An alternative to specify net.ifnames=0 on the kernel command line > > (to disable udev predictable NIC names), is to create an empty file: > > /etc/udev/rules.d/80-net-name-slot.rules > > Well, I think Thomas' comment was primarily about whether we should enable > eudev and dynamically load modules rather than using builtins. For boards that support USB, PCIe and HDMI, I think it is a good default to enable udev (or mdev), as you could see from my other email, enabling udev will load 45 different kernel modules. If we don't enable udev, lsmod will show 0 loaded kernel modules. I don't think we can enable everything as built-in, especially since a user can plug in basically anything in the PCIe slot or USB slot. (But I agree that keeping e.g. Ethernet as built-in is good for NFS root.) E.g. I can plug in a USB to Ethernet adapter, and with udev enabled, the correct kernel module gets loaded automatically. > > Passing net.ifnames=0 is probably the easiest way to disable predictable > NIC names. Only, it's surprising that we have something like 25 defconfigs > that enable eudev, but only 4 of them enable that option... We should > preferably have one "normal" way that this is done. > > > What is BTW not clear to me is _why_ you don't want predictable names. > What's wrong with BR2_SYSTEM_DHCP="enp0s31f6"? Well, if you do a: $ git grep BR2_SYSTEM_DHCP you see that everyone except: configs/ls1028ardb_defconfig:BR2_SYSTEM_DHCP="eno0" is using eth0. But sure, it would be possible to specify the predictable ifname. I guess that it is just a bit unintuitive that you would need to change BR2_SYSTEM_DHCP="eth0" to something else if you happen to enable eudev. (While if you have mdev or neither mdev or udev, eth0 works fine.) Kind regards, Niklas
On Tue, Sep 03, 2024 at 11:10:44PM +0200, Arnout Vandecappelle wrote: > > > > Without udev (i.e. with the current buildroot rock5b_defconfig) > > none of these modules will be loaded. > > That is a very good reaosn to want eudev, indeed! The alternative is to > extend the linux config fragment to include all of these as builtins, but I > guess that's not even possible for hantro_vpu... > > > Then another question is whether we should use eudev or mdev. We currently > have 25 confgs that enable mdev and 30 that enable eudev. Some consistency > there would be nice as well! I remember that long ago, mdev did not support automatically loading kernel modules when hotplugging devices. See e.g.: https://wiki.gentoo.org/wiki/Mdev#Miscellaneous that still says: "mdev unlike udev does not support auto-modules loading." However, it appears that since: https://github.com/buildroot/buildroot/commit/07f46c2b6daec44a6176039c90be67e66c4c2e42 mdev does support automatic module loading. I changed from: BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y to BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_MDEV=y in my defconfig, and did a: make clean && make rock5b_defconfig && make and: $ lsmod | wc -l 45 shows the same amout of loaded kernel modules both with udev and mdev. Plugging in a USB to Ethernet also loads the correct kernel module with mdev. Using mdev will also avoid "udev predictable NIC names", so I the patch in this series that adds net.ifnames=0 is no longer needed. I will resubmit a patch that enables MDEV and will simply drop the patch that adds net.ifnames=0 to the kernel command line. Kind regards, Niklas
diff --git a/board/radxa/rock5b/linux.fragment b/board/radxa/rock5b/linux.fragment deleted file mode 100644 index 90f2f291ba..0000000000 --- a/board/radxa/rock5b/linux.fragment +++ /dev/null @@ -1,2 +0,0 @@ -CONFIG_R8169=y -CONFIG_PHY_ROCKCHIP_NANENG_COMBO_PHY=y diff --git a/configs/rock5b_defconfig b/configs/rock5b_defconfig index 899537c001..bcc2b68f6c 100644 --- a/configs/rock5b_defconfig +++ b/configs/rock5b_defconfig @@ -34,7 +34,6 @@ BR2_LINUX_KERNEL_DTS_SUPPORT=y BR2_LINUX_KERNEL_INSTALL_TARGET=y BR2_LINUX_KERNEL_INTREE_DTS_NAME="rockchip/rk3588-rock-5b" BR2_LINUX_KERNEL_DTB_OVERLAY_SUPPORT=y -BR2_LINUX_KERNEL_CONFIG_FRAGMENT_FILES="board/radxa/rock5b/linux.fragment" # Filesystem BR2_TARGET_GENERIC_HOSTNAME="rock5b" @@ -46,6 +45,7 @@ BR2_PACKAGE_HOST_DOSFSTOOLS=y BR2_PACKAGE_HOST_DTC=y BR2_PACKAGE_HOST_GENIMAGE=y BR2_PACKAGE_HOST_MTOOLS=y +BR2_ROOTFS_DEVICE_CREATION_DYNAMIC_EUDEV=y BR2_ROOTFS_POST_BUILD_SCRIPT="board/radxa/rock5b/post-build.sh" BR2_ROOTFS_POST_IMAGE_SCRIPT="support/scripts/genimage.sh" BR2_ROOTFS_POST_SCRIPT_ARGS="-c board/radxa/rock5b/genimage.cfg"