diff mbox series

Automatic module loading with mdev is broken?

Message ID CAFr9PX=M4VCMcakZsENMXBW8Xi3BcKkrVCTdARu=nCvK-A5ovg@mail.gmail.com
State Not Applicable
Headers show
Series Automatic module loading with mdev is broken? | expand

Commit Message

Daniel Palmer May 6, 2018, 9:46 a.m. UTC
Hi all,

I'm trying to get automatic module loading with mdev and it looks like
it was broken with this commit
b4fc5a180c81689a982d5c595844331684c14f51
(https://github.com/buildroot/buildroot/commit/b4fc5a180c81689a982d5c595844331684c14f51)
or has maybe regressed?

Anyhow the previous version of the line that the commit changed works
(the modules I expect to get loaded get loaded).
The changed version results in nothing being loaded.

I think it's to do with this part "xargs -0 sort -u -z". Sort is
taking in the big list of files correctly and sorting them but the
result
seems to still have new lines in it. So when that is passed to xargs
(which is expecting the items to be null terminated) again
to run modprobe the module aliases don't match and thus nothing gets loaded.

This *works* for me but I'm not sure if it's right:

       ;;

Cheers,

Daniel

Comments

Peter Korsgaard May 7, 2018, 6:45 a.m. UTC | #1
>>>>> "Daniel" == Daniel Palmer <daniel@0x0f.com> writes:

 > Hi all,
 > I'm trying to get automatic module loading with mdev and it looks like
 > it was broken with this commit
 > b4fc5a180c81689a982d5c595844331684c14f51
 > (https://github.com/buildroot/buildroot/commit/b4fc5a180c81689a982d5c595844331684c14f51)
 > or has maybe regressed?

 > Anyhow the previous version of the line that the commit changed works
 > (the modules I expect to get loaded get loaded).
 > The changed version results in nothing being loaded.

 > I think it's to do with this part "xargs -0 sort -u -z". Sort is
 > taking in the big list of files correctly and sorting them but the
 > result
 > seems to still have new lines in it. So when that is passed to xargs
 > (which is expecting the items to be null terminated) again
 > to run modprobe the module aliases don't match and thus nothing gets loaded.

 > This *works* for me but I'm not sure if it's right:

Hmm, you are right. sort -z means that the lines in the input files
should be zero terminated and not newline terminated in addition to
outputting zero terminated strings.

The lines in modalias files are newline terminated, and not zero, E.G.:

hexdump -C /sys/devices/LNXSYSTM:00/LNXCPU:00/modalias
00000000  61 63 70 69 3a 4c 4e 58  43 50 55 3a 0a           |acpi:LNXCPU:.|

The modalias entries themselves afaik never contains spaces, those can
only happen in the path name to the modalias file (as b4fc5a180c81
showed):

find /sys -name modalias | xargs cat | egrep '[[:space:]]'

So I guess we can just do:

find /sys/ -name modalias -print0 | xargs -0 sort u | xargs modprobe -abq

Can you give that a try? Thanks.


 > diff --git a/package/busybox/S10mdev b/package/busybox/S10mdev
 > index 63ca955b1c..71693e7a9d 100644
 > --- a/package/busybox/S10mdev
 > +++ b/package/busybox/S10mdev
 > @@ -9,7 +9,7 @@ case "$1" in
 >        echo /sbin/mdev >/proc/sys/kernel/hotplug
 >        /sbin/mdev -s
 >        # coldplug modules
 > -       find /sys/ -name modalias -print0 | xargs -0 sort -u -z |
 > xargs -0 modprobe -abq
 > +       find /sys/ -name modalias -print0 | xargs -0 sort -u -z | tr
 > -d '\n' | xargs -0 modprobe -abq
 >        ;;
 >   stop)
 >        ;;

 > Cheers,

 > Daniel
 > _______________________________________________
 > buildroot mailing list
 > buildroot@busybox.net
 > http://lists.busybox.net/mailman/listinfo/buildroot
Daniel Palmer May 7, 2018, 8:30 a.m. UTC | #2
Hi Peter,

Your change works but looking at the text of the original change that
caused the regression 'First alias in question is "platform:Fixed MDIO
bus".' it seems it would fail with that? The driver that creates that
alias still exists ->
https://github.com/torvalds/linux/blob/master/drivers/net/phy/fixed_phy.c#L261

Cheers,

Daniel
Peter Korsgaard May 7, 2018, 8:53 a.m. UTC | #3
>>>>> "Daniel" == Daniel Palmer <daniel@0x0f.com> writes:

 > Hi Peter,
 > Your change works but looking at the text of the original change that
 > caused the regression 'First alias in question is "platform:Fixed MDIO
 > bus".' it seems it would fail with that? The driver that creates that
 > alias still exists ->
 > https://github.com/torvalds/linux/blob/master/drivers/net/phy/fixed_phy.c#L261

Ok. Looking at the error message it was really about the sysfs entry:

sort: /sys/devices/platform/Fixed

So that was presumably '/sys/devices/platform/Fixed MDIO bus'. This is
fixed by the find .. -print0 | xargs -0 sort

There may or may not be situations where the modalias info contains
spaces. I have so far not seen such situation though. fixed_phy does not
provide it:

/sbin/modinfo build/linux-4.14.34/drivers/net/phy/fixed_phy.ko
icense:        GPL
author:         Vitaly Bordug
description:    Fixed MDIO bus (MDIO bus emulation with fixed PHYs)
depends:        libphy
intree:         Y
name:           fixed_phy
vermagic:       4.14.34 SMP

There unfortunately doesn't seem to be an obvious/easy way to handle
modalias entries containing spaces. The easy solution would be to use
xargs -d '\n' but that is not supported by the busybox applet, so I
guess we need to use tr to replace \n with \0 and then xargs -0.

But I would prefer to wait with that until we know it is needed.
Daniel Palmer May 7, 2018, 9:40 a.m. UTC | #4
Hi Peter,

I found one of my systems that actually has this module loaded:

daniel@espressobin2:/sys/devices/platform/Fixed MDIO bus.0$ cat modalias
platform:Fixed MDIO bus

So I think we do need the tr fix.
Peter Korsgaard May 7, 2018, 6:29 p.m. UTC | #5
>>>>> "Daniel" == Daniel Palmer <daniel@0x0f.com> writes:

 > Hi Peter,
 > I found one of my systems that actually has this module loaded:

 > daniel@espressobin2:/sys/devices/platform/Fixed MDIO bus.0$ cat modalias
 > platform:Fixed MDIO bus

 > So I think we do need the tr fix.

Ok :/ - Lets then add a tr '\n' '\0' between sort and xargs -0.
diff mbox series

Patch

diff --git a/package/busybox/S10mdev b/package/busybox/S10mdev
index 63ca955b1c..71693e7a9d 100644
--- a/package/busybox/S10mdev
+++ b/package/busybox/S10mdev
@@ -9,7 +9,7 @@  case "$1" in
       echo /sbin/mdev >/proc/sys/kernel/hotplug
       /sbin/mdev -s
       # coldplug modules
-       find /sys/ -name modalias -print0 | xargs -0 sort -u -z |
xargs -0 modprobe -abq
+       find /sys/ -name modalias -print0 | xargs -0 sort -u -z | tr
-d '\n' | xargs -0 modprobe -abq
       ;;
  stop)