Hi there.
I have come across an issue in the Linux kernel (6.8.7 and others) and
wondered whether I am doing something wrong?
In trying to test the dsa_loop module, I have found that the
dsa_loop_bdinfo module is not built when the following combination of
configuration items is set:
CONFIG_NET_DSA=y
CONFIG_NET_DSA_LOOP=m
CONFIG_FIXED_PHY=y
This combination can easily be obtained using make menuconfig.
It seems that driver/net/Makefile’s inclusion of the dsa directory as
an obj-m precludes the building of dsa_loop_bdinfo.c (which is an
obj-y):
driver/net/Makefile:
…
obj-$(CONFIG_NET_DSA) += dsa/
…
driver/net/dsa/Makefile:
…
ifdef CONFIG_NET_DSA_LOOP
obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
endif
…
Hello,
On 5/13/2024 8:33 AM, Stephen Langstaff wrote:
> [Corrected emails for f.fainelli and olteanv]
What is the build problem?
--
Florian
[Corrected emails for f.fainelli and olteanv]
dsa_loop_bdinfo.ko is not created as a module.
On Mon, May 13, 2024 at 4:34 PM Florian Fainelli <[email protected]> wrote:
>
> Hello,
>
> On 5/13/2024 8:33 AM, Stephen Langstaff wrote:
> > [Corrected emails for f.fainelli and olteanv]
>
> What is the build problem?
> --
> Florian
On 5/13/2024 8:36 AM, Stephen Langstaff wrote:
> dsa_loop_bdinfo.ko is not created as a module.
The Makefile has this:
obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
so it follows the value of CONFIG_FIXED_PHY, what is this configuration
symbol is to in your kernel configuration?
>
> On Mon, May 13, 2024 at 4:34 PM Florian Fainelli <[email protected]> wrote:
>>
>> Hello,
>>
>> On 5/13/2024 8:33 AM, Stephen Langstaff wrote:
>>> [Corrected emails for f.fainelli and olteanv]
>>
>> What is the build problem?
>> --
>> Florian
--
Florian
On Mon, May 13, 2024 at 4:40 PM Florian Fainelli <[email protected]> wrote:
> The Makefile has this:
>
> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
>
> so it follows the value of CONFIG_FIXED_PHY, what is this configuration
> symbol is to in your kernel configuration?
CONFIG_FIXED_PHY=y
On 5/13/2024 8:45 AM, Stephen Langstaff wrote:
> On Mon, May 13, 2024 at 4:40 PM Florian Fainelli <[email protected]> wrote:
>> The Makefile has this:
>>
>> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
>>
>> so it follows the value of CONFIG_FIXED_PHY, what is this configuration
>> symbol is to in your kernel configuration?
>
> CONFIG_FIXED_PHY=y
Then dsa_loop_bdinfo is going to be built into your kernel image, and it
will register a mdio_board_info structure which will create fixed PHY
devices and it will assume that the DSA conduit network device name is
"eth0". If this is not correct for your system, that needs changing.
What are you trying to achieve? Can you be a tad more verbose about what
you have tried and what is not working?
--
Florian
On Mon, May 13, 2024 at 4:50 PM Florian Fainelli <[email protected]> wrote:
> Then dsa_loop_bdinfo is going to be built into your kernel image, and it
> will register a mdio_board_info structure which will create fixed PHY
> devices and it will assume that the DSA conduit network device name is
> "eth0". If this is not correct for your system, that needs changing.
>
> What are you trying to achieve? Can you be a tad more verbose about what
> you have tried and what is not working?
Sure, sorry I was being a bit terse.
In a 6.1.55 Linux kernel build, after a full make, if I touch a file
marked as "=y" in the .config (e.g. net/devres.c) I see that the
compiler rebuilds the corresponding .o file, as expected:
user@machine:/opt/linux-imx$ touch net/devres.c
user@machine:/opt/linux-imx$ make
CALL scripts/checksyscalls.sh
CC net/devres.o
AR net/built-in.a
..
If I do the same for drivers/net/dsa/dsa_loop_bdinfo.c nothing is rebuilt!
user@machine:/opt/linux-imx$ touch drivers/net/dsa/dsa_loop_bdinfo.c
user@machine:/opt/linux-imx$ make
CALL scripts/checksyscalls.sh
user@machine:/opt/linux-imx$
What I am trying to achieve is to use dsa_loop to connect a CPU
running Linux to an Ethernet switch via a "conduit" Ethernet link
(e.g. eth0) using the port-based Ethertype DSA (Marvell) frame tagging
protocol, so that the Linux kernel will present the userspace with
separate Ethernet links for each of the switch's user ports (e.g.
lan1, lan2, lan3).
As I understand it this is usually done via a fixed phy-less conduit
(e.g. xMII) and an MDIO link between the processor and the switch so
that the processor can configure and control the physical user ports.
I want to do some software development ahead of receiving the
hardware, so I want to "fake" the MDIO link, or in some other way
configure the kernel, such that it uses port-based EDSA frame tagging
over an ordinary (PHY-equipped) Ethernet port and it looks to me like
dsa_loop should be capable of doing this.
> What I am trying to achieve is to use dsa_loop to connect a CPU
> running Linux to an Ethernet switch via a "conduit" Ethernet link
> (e.g. eth0) using the port-based Ethertype DSA (Marvell) frame tagging
> protocol, so that the Linux kernel will present the userspace with
> separate Ethernet links for each of the switch's user ports (e.g.
> lan1, lan2, lan3).
>
> As I understand it this is usually done via a fixed phy-less conduit
> (e.g. xMII) and an MDIO link between the processor and the switch so
> that the processor can configure and control the physical user ports.
>
> I want to do some software development ahead of receiving the
> hardware, so I want to "fake" the MDIO link, or in some other way
> configure the kernel, such that it uses port-based EDSA frame tagging
> over an ordinary (PHY-equipped) Ethernet port and it looks to me like
> dsa_loop should be capable of doing this.
That is an interesting use case. I don't know of anybody doing that
before.
As Florian pointed out, dsa_loop_bdinfo.o should be built into your
kernel. When you insmod dsa_loop.ko, it should match the data in
dsa_loop_bdinfo to the dsa_loop driver an instantiate an instance of
it. Do you see that happening?
You could hack dsa_loop_get_protocol() to return DSA_TAG_PROTO_EDSA,
in order to use the EDSA frame tagger. dsa_loop does not have an
implementation of .change_tag_protocol, so i don't think you can
change it at runtime. However an implementation of it should be
trivial.
Andrew
On Mon, May 13, 2024 at 5:54 PM Andrew Lunn <[email protected]> wrote:
> As Florian pointed out, dsa_loop_bdinfo.o should be built into your
> kernel. When you insmod dsa_loop.ko, it should match the data in
> dsa_loop_bdinfo to the dsa_loop driver an instantiate an instance of
> it. Do you see that happening?
Well, if I fiddle with the Makefiles to get dsa_loop_bdinfo built as a
module (as opposed to not building at all) then I see the following
behaviour:
root@machine:~/dsa_test# ls
dsa_loop.ko dsa_loop_bdinfo.ko
root@machine:~/dsa_test# modprobe dsa_core
root@machine:~/dsa_test# insmod dsa_loop_bdinfo.ko
root@machine:~/dsa_test# insmod dsa_loop.ko
root@machine:~/dsa_test# lsmod
Module Size Used by
dsa_loop 16384 0
dsa_loop_bdinfo 16384 0
dsa_core 118784 1 dsa_loop
..
..and I see no new interfaces reported by ifconfig -a
[just the existing eth0, eth1 and lo0 interfaces]
I will revert my changes to the Makefiles and confirm that
dsa_loop_bdinfo.o does not get built into the kernel - that's an
overnight build for me so bye for now!
On Mon, May 13, 2024 at 06:40:19PM +0100, Stephen Langstaff wrote:
> On Mon, May 13, 2024 at 5:54 PM Andrew Lunn <[email protected]> wrote:
> > As Florian pointed out, dsa_loop_bdinfo.o should be built into your
> > kernel. When you insmod dsa_loop.ko, it should match the data in
> > dsa_loop_bdinfo to the dsa_loop driver an instantiate an instance of
> > it. Do you see that happening?
>
> Well, if I fiddle with the Makefiles to get dsa_loop_bdinfo built as a
> module (as opposed to not building at all) then I see the following
> behaviour:
>
> root@machine:~/dsa_test# ls
> dsa_loop.ko dsa_loop_bdinfo.ko
> root@machine:~/dsa_test# modprobe dsa_core
> root@machine:~/dsa_test# insmod dsa_loop_bdinfo.ko
> root@machine:~/dsa_test# insmod dsa_loop.ko
> root@machine:~/dsa_test# lsmod
> Module Size Used by
> dsa_loop 16384 0
> dsa_loop_bdinfo 16384 0
> dsa_core 118784 1 dsa_loop
> ...
>
> ...and I see no new interfaces reported by ifconfig -a
> [just the existing eth0, eth1 and lo0 interfaces]
>
> I will revert my changes to the Makefiles and confirm that
> dsa_loop_bdinfo.o does not get built into the kernel - that's an
> overnight build for me so bye for now!
I think most developer cross compile the kernel on a big machine. I
also tend to build in everything i need, so its all in one file, which
i get uboot to tftpboot. It makes the whole build/boot/crash cycle
much master.
Andrew
On 5/13/24 11:55, Stephen Langstaff wrote:
>
> OK, just confirmed that dsa_loop_bdinfo.c is not compiled to either a .o
> or a .ko file when using the unmodified Makefiles.
Attached is a configuration that I use on a x86-64 VM for testing that
is started with:
qemu-system-x86_64 \
-s \
-cpu "max" \
-smp 4 \
-kernel ~/dev/linux/arch/x86/boot/bzImage \
-device pci-bridge,id=pci_bridge1,bus=pci.0,chassis_nr=1 \
-drive
file=debian.img,if=none,id=drive-virtio-disk0,format=qcow2 -device
virtio-blk-pci,scsi=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
\
-nographic \
-append "root=/dev/vda1 console=ttyS0,115200 mitigations=off" \
-net nic,model=e1000 -net tap,ifname=tap0
..
ip link set dev enp0s3 name eth0
modprobe dsa_loop_bdinfo
modprobe dsa_loop
ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode
DEFAULT group default qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode
DEFAULT group default qlen 1000
link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
altname enp0s3
3: lan1@eth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state
DOWN mode DEFAULT group default qlen 1000
link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
4: lan2@eth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state
DOWN mode DEFAULT group default qlen 1000
link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
5: lan3@eth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state
DOWN mode DEFAULT group default qlen 1000
link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
6: lan4@eth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state
DOWN mode DEFAULT group default qlen 1000
link/ether 52:54:00:12:34:56 brd ff:ff:ff:ff:ff:ff
--
Florian
On Mon, May 13, 2024 at 8:05 PM Florian Fainelli <[email protected]> wrote:
> Attached is a configuration that I use on a x86-64 VM for testing that
> is started with:
That's kind of what I expect to be happening on my target, but it's not.
I notice that your configuration does not define CONFIG_FIXED_PHY as
either a module or built-in - could that explain both the fact that my
dsa_loop_bdinfo.c does not get compiled when using the default
Makefiles, and that if I force compilation to a module it does not
appear to create the expected lanX@eth0 interfaces?
I have added some printk debug lines to the dsa_loop_init and
dsa_loop_drv_probe functions, and only see the one from the init
function, which makes me suspect that something is stopping the probe
from happening - I assume this should happen when the dsa_loop_bdinfo
module is inserted?
root@machine:~/dsa_test# ls
dsa_loop.ko dsa_loop_bdinfo.ko
root@machine:~/dsa_test# modprobe dsa_core
root@machine:~/dsa_test#
root@machine:~/dsa_test# insmod dsa_loop_bdinfo.ko
[ 51.742367] dsa_loop_bdinfo_init
root@machine:~/dsa_test# insmod dsa_loop.ko
[ 58.485104] dsa_loop_init
root@machine:~/dsa_test# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN
mode DEFAULT group default qlen 1000
link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP
mode DEFAULT group default qlen 1000
link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
3: eth1: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state
DOWN mode DEFAULT group default qlen 1000
link/ether xx:xx:xx:xx:xx:xx brd ff:ff:ff:ff:ff:ff
4: can0: <NOARP,ECHO> mtu 16 qdisc noop state DOWN mode DEFAULT group
default qlen 10
link/can
On Tue, May 14, 2024 at 10:28:55AM +0100, Stephen Langstaff wrote:
> On Mon, May 13, 2024 at 8:05 PM Florian Fainelli <[email protected]> wrote:
> > Attached is a configuration that I use on a x86-64 VM for testing that
> > is started with:
>
> That's kind of what I expect to be happening on my target, but it's not.
>
> I notice that your configuration does not define CONFIG_FIXED_PHY as
> either a module or built-in - could that explain both the fact that my
> dsa_loop_bdinfo.c does not get compiled when using the default
> Makefiles, and that if I force compilation to a module it does not
> appear to create the expected lanX@eth0 interfaces?
>
> I have added some printk debug lines to the dsa_loop_init and
> dsa_loop_drv_probe functions, and only see the one from the init
> function, which makes me suspect that something is stopping the probe
> from happening - I assume this should happen when the dsa_loop_bdinfo
> module is inserted?
>
> root@machine:~/dsa_test# ls
> dsa_loop.ko dsa_loop_bdinfo.ko
>
> root@machine:~/dsa_test# modprobe dsa_core
> root@machine:~/dsa_test#
>
> root@machine:~/dsa_test# insmod dsa_loop_bdinfo.ko
> [ 51.742367] dsa_loop_bdinfo_init
>
> root@machine:~/dsa_test# insmod dsa_loop.ko
> [ 58.485104] dsa_loop_init
This is probably an ordering problem.
dsa_loop_bdinfo calls mdiobus_register_board_info() with the bus_id
"fixed-0". That call adds it to a linked list, but does nothing else.
mdiobus_setup_mdiodev_from_board_info() is what walks the linked list
and sees if an MDIO bus matches the ID, and if so, calls the callback
to add a device to the bus. mdiobus_setup_mdiodev_from_board_info() is
only called from __mdiobus_register(). So the boardinfo needs to be
registered before the bus is registers. The other way around does not
work.
So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
Andrew
On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
In my configuration FIXED_PHY is selected by several other modules:
│ Selected by [y]:
│ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
│ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
(ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
│ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
&& PHYLIB [=y]
..so it looks pretty tied up with the MDIO support which I guess I
will need for the real PHY!
If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
you think that would solve the ordering issue?
On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
>
> > So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
>
> In my configuration FIXED_PHY is selected by several other modules:
> │ Selected by [y]:
> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
> && PHYLIB [=y]
>
> ...so it looks pretty tied up with the MDIO support which I guess I
> will need for the real PHY!
>
> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
> you think that would solve the ordering issue?
Probably.
Florian test setup shows it can work.
This mdio board info stuff is based on the same concept for I2C
devices. However, it is simplified a bit, since it is not really meant
for production use. What i suspect the I2C version does is that when a
new board info is registers, it walks all the existing I2C busses and
sees if there is a match. The MDIO version is missing this, it only
runs the match when a bus is added. So it is less forgiving of order.
Andrew
On 5/14/2024 9:17 AM, Andrew Lunn wrote:
> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
>> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
>>
>>> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
>>
>> In my configuration FIXED_PHY is selected by several other modules:
>> │ Selected by [y]:
>> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
>> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
>> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
>> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
>> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
>> && PHYLIB [=y]
>>
>> ...so it looks pretty tied up with the MDIO support which I guess I
>> will need for the real PHY!
>>
>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
>> you think that would solve the ordering issue?
I have re-created the issue with CONFIG_FIXED_PHY=y and for a reason I
do not yet understand the following rule:
obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
does not result in the kernel image containing the dsa_loop_bdinfo.o
object symbols. I am fairly sure this worked when this was submitted
back then, so give me a day or two to figure out why. AFAICT the make
rule is simply not executed.
>
> Probably.
>
> Florian test setup shows it can work.
>
> This mdio board info stuff is based on the same concept for I2C
> devices. However, it is simplified a bit, since it is not really meant
> for production use. What i suspect the I2C version does is that when a
> new board info is registers, it walks all the existing I2C busses and
> sees if there is a match. The MDIO version is missing this, it only
> runs the match when a bus is added. So it is less forgiving of order.
Yes that is fair enough, there could be improvements in that area. Or
nowadays, given that software node(s) are a thing and that we can apply
overlays to PCI(e) devices, we might consider doing that and ditch
dsa_loop_bdinfo.c entirely.
--
Florian
On Tue, May 14, 2024 at 5:17 PM Andrew Lunn <[email protected]> wrote:
>
> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
> > If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
> > you think that would solve the ordering issue?
>
> Probably.
Well, some minor progress... when dsa_loop and dsa_loop_bdinfo are
built-in to the kernel then it appears that, at least for this
configuration, the driver probe function is called:
[ 0.053068] dsa_loop_bdinfo_init
[ 1.509255] dsa_loop_init
[ 1.513178] dsa_loop_drv_probe
[ 2.633673] dsa_loop_drv_probe
..
[ 3.273496] dsa_loop_drv_probe
[ 3.276589] dsa_loop_get_protocol
[ 3.281608] dsa-loop fixed-0:1f: skipping link registration for CPU port 5
[ 3.288558] (null): phylink: error: empty supported_interfaces
[ 3.294500] error creating PHYLINK: -22
[ 3.298343] dsa-loop fixed-0:1f lan1 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 0
[ 3.308145] (null): phylink: error: empty supported_interfaces
[ 3.314070] error creating PHYLINK: -22
[ 3.317907] dsa-loop fixed-0:1f lan2 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 1
[ 3.327677] (null): phylink: error: empty supported_interfaces
[ 3.333609] error creating PHYLINK: -22
[ 3.337449] dsa-loop fixed-0:1f lan3 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 2
[ 3.347223] (null): phylink: error: empty supported_interfaces
[ 3.353145] error creating PHYLINK: -22
[ 3.356983] dsa-loop fixed-0:1f lan4 (uninitialized): error -22
setting up PHY for tree 0, switch 0, port 3
[ 3.366855] device eth0 entered promiscuous mode
[ 3.371497] DSA: tree 0 setup
[ 3.374474] dsa-loop fixed-0:1f: DSA mockup driver: 0x1f
The -22 error appears to be related to the issues mentioned here:
https://www.spinics.net/lists/netdev/msg922961.html so I will now try
to apply the suggested short-term fix.
Adding Olek and Masahiro,
On 5/14/24 21:05, Florian Fainelli wrote:
>
>
> On 5/14/2024 9:17 AM, Andrew Lunn wrote:
>> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
>>> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
>>>
>>>> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
>>>
>>> In my configuration FIXED_PHY is selected by several other modules:
>>> │ Selected by [y]:
>>> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
>>> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
>>> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
>>> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
>>> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
>>> && PHYLIB [=y]
>>>
>>> ...so it looks pretty tied up with the MDIO support which I guess I
>>> will need for the real PHY!
>>>
>>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
>>> you think that would solve the ordering issue?
>
> I have re-created the issue with CONFIG_FIXED_PHY=y and for a reason I
> do not yet understand the following rule:
>
> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
>
> does not result in the kernel image containing the dsa_loop_bdinfo.o
> object symbols. I am fairly sure this worked when this was submitted
> back then, so give me a day or two to figure out why. AFAICT the make
> rule is simply not executed.
Bisection landed on 227d72063fccb2d19b30fb4197fba478514f7d83 ("dsa:
simplify Kconfig symbols and dependencies") which appeared in v5.13 and
specifically this hunk being reverted back to how it was before gets us
the build results we want:
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 7ffd2d03efaf..5da6424bc6f8 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -45,7 +45,7 @@ obj-$(CONFIG_ARCNET) += arcnet/
obj-$(CONFIG_DEV_APPLETALK) += appletalk/
obj-$(CONFIG_CAIF) += caif/
obj-$(CONFIG_CAN) += can/
-obj-$(CONFIG_NET_DSA) += dsa/
+obj-y += dsa/
obj-$(CONFIG_ETHERNET) += ethernet/
obj-$(CONFIG_FDDI) += fddi/
obj-$(CONFIG_HIPPI) += hippi/
Masahiro, for context in drivers/net/dsa/Makefile we have this bit:
obj-$(CONFIG_NET_DSA_LOOP) += dsa_loop.o
ifdef CONFIG_NET_DSA_LOOP
obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
endif
whereby we want dsa_loop.o to follow the value of CONFIG_NET_DSA_LOOP,
and we want dsa_loop_bdinfo.o to be either built as a module or built
into the kernel and we want to follow the value of CONFIG_FIXED_PHY
because there is a functional dependency between the two objects.
Prior to Olek's change this would work just fine because we would always
descend into drivers/net/dsa/ but after his change, and assuming that
CONFIG_NET_DSA=m which is the case, then we no longer get
dsa_loop_bdinfo.o to be built at all when CONFIG_FIXED_PHY=y.
Essentially only obj-m rules are being processed, obj-y rules are not.
That does not really seem intuitive to me as to why any suggestions on
how to fix that, short of unconditionally descending into the tree like
we used to?
Reproducer configuration available here:
https://gist.github.com/ffainelli/2650a832803b502b94b2d247209f61b1
rm drivers/net/dsa/*.o
ARCH=x86 make drivers/net/dsa/
ls drivers/net/dsa/dsa_loop_bdinfo.o
Thanks!
--
Florian
On 5/15/24 02:23, Stephen Langstaff wrote:
> On Tue, May 14, 2024 at 5:17 PM Andrew Lunn <[email protected]> wrote:
>>
>> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
>>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
>>> you think that would solve the ordering issue?
>>
>> Probably.
>
> Well, some minor progress... when dsa_loop and dsa_loop_bdinfo are
> built-in to the kernel then it appears that, at least for this
> configuration, the driver probe function is called:
>
> [ 0.053068] dsa_loop_bdinfo_init
> [ 1.509255] dsa_loop_init
> [ 1.513178] dsa_loop_drv_probe
> [ 2.633673] dsa_loop_drv_probe
> ...
> [ 3.273496] dsa_loop_drv_probe
> [ 3.276589] dsa_loop_get_protocol
> [ 3.281608] dsa-loop fixed-0:1f: skipping link registration for CPU port 5
> [ 3.288558] (null): phylink: error: empty supported_interfaces
> [ 3.294500] error creating PHYLINK: -22
> [ 3.298343] dsa-loop fixed-0:1f lan1 (uninitialized): error -22
> setting up PHY for tree 0, switch 0, port 0
> [ 3.308145] (null): phylink: error: empty supported_interfaces
> [ 3.314070] error creating PHYLINK: -22
> [ 3.317907] dsa-loop fixed-0:1f lan2 (uninitialized): error -22
> setting up PHY for tree 0, switch 0, port 1
> [ 3.327677] (null): phylink: error: empty supported_interfaces
> [ 3.333609] error creating PHYLINK: -22
> [ 3.337449] dsa-loop fixed-0:1f lan3 (uninitialized): error -22
> setting up PHY for tree 0, switch 0, port 2
> [ 3.347223] (null): phylink: error: empty supported_interfaces
> [ 3.353145] error creating PHYLINK: -22
> [ 3.356983] dsa-loop fixed-0:1f lan4 (uninitialized): error -22
> setting up PHY for tree 0, switch 0, port 3
> [ 3.366855] device eth0 entered promiscuous mode
> [ 3.371497] DSA: tree 0 setup
> [ 3.374474] dsa-loop fixed-0:1f: DSA mockup driver: 0x1f
>
> The -22 error appears to be related to the issues mentioned here:
> https://www.spinics.net/lists/netdev/msg922961.html so I will now try
> to apply the suggested short-term fix.
What changes have you done to the dsa_loop.c file for this error to show
up? Currently the driver does the following:
static void dsa_loop_phylink_get_caps(struct dsa_switch *dsa, int port,
struct phylink_config *config)
{
bitmap_fill(config->supported_interfaces,
PHY_INTERFACE_MODE_MAX);
__clear_bit(PHY_INTERFACE_MODE_NA, config->supported_interfaces);
config->mac_capabilities = ~0;
}
which is basically to say: I support everything, except
PHY_INTERFACE_MDOE_NA.
--
Florian
On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <[email protected]> wrote:
>
> Adding Olek and Masahiro,
>
> On 5/14/24 21:05, Florian Fainelli wrote:
> >
> >
> > On 5/14/2024 9:17 AM, Andrew Lunn wrote:
> >> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
> >>> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
> >>>
> >>>> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
> >>>
> >>> In my configuration FIXED_PHY is selected by several other modules:
> >>> │ Selected by [y]:
> >>> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
> >>> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
> >>> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
> >>> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
> >>> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
> >>> && PHYLIB [=y]
> >>>
> >>> ...so it looks pretty tied up with the MDIO support which I guess I
> >>> will need for the real PHY!
> >>>
> >>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
> >>> you think that would solve the ordering issue?
> >
> > I have re-created the issue with CONFIG_FIXED_PHY=y and for a reason I
> > do not yet understand the following rule:
> >
> > obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
> >
> > does not result in the kernel image containing the dsa_loop_bdinfo.o
> > object symbols. I am fairly sure this worked when this was submitted
> > back then, so give me a day or two to figure out why. AFAICT the make
> > rule is simply not executed.
>
> Bisection landed on 227d72063fccb2d19b30fb4197fba478514f7d83 ("dsa:
> simplify Kconfig symbols and dependencies") which appeared in v5.13 and
> specifically this hunk being reverted back to how it was before gets us
> the build results we want:
>
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 7ffd2d03efaf..5da6424bc6f8 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -45,7 +45,7 @@ obj-$(CONFIG_ARCNET) += arcnet/
> obj-$(CONFIG_DEV_APPLETALK) += appletalk/
> obj-$(CONFIG_CAIF) += caif/
> obj-$(CONFIG_CAN) += can/
> -obj-$(CONFIG_NET_DSA) += dsa/
> +obj-y += dsa/
> obj-$(CONFIG_ETHERNET) += ethernet/
> obj-$(CONFIG_FDDI) += fddi/
> obj-$(CONFIG_HIPPI) += hippi/
>
> Masahiro, for context in drivers/net/dsa/Makefile we have this bit:
>
> obj-$(CONFIG_NET_DSA_LOOP) += dsa_loop.o
> ifdef CONFIG_NET_DSA_LOOP
> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
> endif
>
> whereby we want dsa_loop.o to follow the value of CONFIG_NET_DSA_LOOP,
> and we want dsa_loop_bdinfo.o to be either built as a module or built
> into the kernel and we want to follow the value of CONFIG_FIXED_PHY
> because there is a functional dependency between the two objects.
>
> Prior to Olek's change this would work just fine because we would always
> descend into drivers/net/dsa/ but after his change, and assuming that
> CONFIG_NET_DSA=m which is the case, then we no longer get
> dsa_loop_bdinfo.o to be built at all when CONFIG_FIXED_PHY=y.
> Essentially only obj-m rules are being processed, obj-y rules are not.
>
> That does not really seem intuitive to me as to why any suggestions on
> how to fix that, short of unconditionally descending into the tree like
> we used to?
"obj-m += dsa/" means everything under dsa/ must be modular.
If there is a built-in object under dsa/ with CONFIG_NET_DSA=m,
you cannot do "obj-$(CONFIG_NET_DSA) += dsa/".
You need to change it back to "obj-y += dsa/".
>
> Reproducer configuration available here:
>
> https://gist.github.com/ffainelli/2650a832803b502b94b2d247209f61b1
>
> rm drivers/net/dsa/*.o
> ARCH=x86 make drivers/net/dsa/
> ls drivers/net/dsa/dsa_loop_bdinfo.o
>
> Thanks!
> --
> Florian
>
--
Best Regards
Masahiro Yamada
On 5/15/2024 7:17 PM, Masahiro Yamada wrote:
> On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <[email protected]> wrote:
>>
>> Adding Olek and Masahiro,
>>
>> On 5/14/24 21:05, Florian Fainelli wrote:
>>>
>>>
>>> On 5/14/2024 9:17 AM, Andrew Lunn wrote:
>>>> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
>>>>> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
>>>>>
>>>>>> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
>>>>>
>>>>> In my configuration FIXED_PHY is selected by several other modules:
>>>>> │ Selected by [y]:
>>>>> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
>>>>> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
>>>>> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
>>>>> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
>>>>> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
>>>>> && PHYLIB [=y]
>>>>>
>>>>> ...so it looks pretty tied up with the MDIO support which I guess I
>>>>> will need for the real PHY!
>>>>>
>>>>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
>>>>> you think that would solve the ordering issue?
>>>
>>> I have re-created the issue with CONFIG_FIXED_PHY=y and for a reason I
>>> do not yet understand the following rule:
>>>
>>> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
>>>
>>> does not result in the kernel image containing the dsa_loop_bdinfo.o
>>> object symbols. I am fairly sure this worked when this was submitted
>>> back then, so give me a day or two to figure out why. AFAICT the make
>>> rule is simply not executed.
>>
>> Bisection landed on 227d72063fccb2d19b30fb4197fba478514f7d83 ("dsa:
>> simplify Kconfig symbols and dependencies") which appeared in v5.13 and
>> specifically this hunk being reverted back to how it was before gets us
>> the build results we want:
>>
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 7ffd2d03efaf..5da6424bc6f8 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -45,7 +45,7 @@ obj-$(CONFIG_ARCNET) += arcnet/
>> obj-$(CONFIG_DEV_APPLETALK) += appletalk/
>> obj-$(CONFIG_CAIF) += caif/
>> obj-$(CONFIG_CAN) += can/
>> -obj-$(CONFIG_NET_DSA) += dsa/
>> +obj-y += dsa/
>> obj-$(CONFIG_ETHERNET) += ethernet/
>> obj-$(CONFIG_FDDI) += fddi/
>> obj-$(CONFIG_HIPPI) += hippi/
>>
>> Masahiro, for context in drivers/net/dsa/Makefile we have this bit:
>>
>> obj-$(CONFIG_NET_DSA_LOOP) += dsa_loop.o
>> ifdef CONFIG_NET_DSA_LOOP
>> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
>> endif
>>
>> whereby we want dsa_loop.o to follow the value of CONFIG_NET_DSA_LOOP,
>> and we want dsa_loop_bdinfo.o to be either built as a module or built
>> into the kernel and we want to follow the value of CONFIG_FIXED_PHY
>> because there is a functional dependency between the two objects.
>>
>> Prior to Olek's change this would work just fine because we would always
>> descend into drivers/net/dsa/ but after his change, and assuming that
>> CONFIG_NET_DSA=m which is the case, then we no longer get
>> dsa_loop_bdinfo.o to be built at all when CONFIG_FIXED_PHY=y.
>> Essentially only obj-m rules are being processed, obj-y rules are not.
>>
>> That does not really seem intuitive to me as to why any suggestions on
>> how to fix that, short of unconditionally descending into the tree like
>> we used to?
>
>
>
> "obj-m += dsa/" means everything under dsa/ must be modular.
>
>
>
> If there is a built-in object under dsa/ with CONFIG_NET_DSA=m,
> you cannot do "obj-$(CONFIG_NET_DSA) += dsa/".
>
>
> You need to change it back to "obj-y += dsa/".
Thanks, posted a patch doing that! Is there anyway that Kbuild could be
warning about such a situation?
--
Florian
On Thu, May 16, 2024 at 12:35 PM Florian Fainelli <f.fainelli@gmailcom> wrote:
>
>
>
> On 5/15/2024 7:17 PM, Masahiro Yamada wrote:
> > On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <[email protected]> wrote:
> >>
> >> Adding Olek and Masahiro,
> >>
> >> On 5/14/24 21:05, Florian Fainelli wrote:
> >>>
> >>>
> >>> On 5/14/2024 9:17 AM, Andrew Lunn wrote:
> >>>> On Tue, May 14, 2024 at 05:08:24PM +0100, Stephen Langstaff wrote:
> >>>>> On Tue, May 14, 2024 at 1:32 PM Andrew Lunn <[email protected]> wrote:
> >>>>>
> >>>>>> So try to making FIXED_PHY = m, and load it after dsa_loop_bdinfo.ko.
> >>>>>
> >>>>> In my configuration FIXED_PHY is selected by several other modules:
> >>>>> │ Selected by [y]:
> >>>>> │ - FSL_DPAA_ETH [=y] && NETDEVICES [=y] && ETHERNET [=y] &&
> >>>>> NET_VENDOR_FREESCALE [=y] && FSL_DPAA [=y] && FSL_FMAN [=y]
> >>>>> │ - FWNODE_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] &&
> >>>>> (ACPI [=y] || OF [=y] || COMPILE_TEST [=n])
> >>>>> │ - OF_MDIO [=y] && NETDEVICES [=y] && MDIO_DEVICE [=y] && OF [=y]
> >>>>> && PHYLIB [=y]
> >>>>>
> >>>>> ...so it looks pretty tied up with the MDIO support which I guess I
> >>>>> will need for the real PHY!
> >>>>>
> >>>>> If I sorted out building the dsa_loop_bdinfo.c code as a built-in do
> >>>>> you think that would solve the ordering issue?
> >>>
> >>> I have re-created the issue with CONFIG_FIXED_PHY=y and for a reason I
> >>> do not yet understand the following rule:
> >>>
> >>> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
> >>>
> >>> does not result in the kernel image containing the dsa_loop_bdinfo.o
> >>> object symbols. I am fairly sure this worked when this was submitted
> >>> back then, so give me a day or two to figure out why. AFAICT the make
> >>> rule is simply not executed.
> >>
> >> Bisection landed on 227d72063fccb2d19b30fb4197fba478514f7d83 ("dsa:
> >> simplify Kconfig symbols and dependencies") which appeared in v5.13 and
> >> specifically this hunk being reverted back to how it was before gets us
> >> the build results we want:
> >>
> >> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> >> index 7ffd2d03efaf..5da6424bc6f8 100644
> >> --- a/drivers/net/Makefile
> >> +++ b/drivers/net/Makefile
> >> @@ -45,7 +45,7 @@ obj-$(CONFIG_ARCNET) += arcnet/
> >> obj-$(CONFIG_DEV_APPLETALK) += appletalk/
> >> obj-$(CONFIG_CAIF) += caif/
> >> obj-$(CONFIG_CAN) += can/
> >> -obj-$(CONFIG_NET_DSA) += dsa/
> >> +obj-y += dsa/
> >> obj-$(CONFIG_ETHERNET) += ethernet/
> >> obj-$(CONFIG_FDDI) += fddi/
> >> obj-$(CONFIG_HIPPI) += hippi/
> >>
> >> Masahiro, for context in drivers/net/dsa/Makefile we have this bit:
> >>
> >> obj-$(CONFIG_NET_DSA_LOOP) += dsa_loop.o
> >> ifdef CONFIG_NET_DSA_LOOP
> >> obj-$(CONFIG_FIXED_PHY) += dsa_loop_bdinfo.o
> >> endif
> >>
> >> whereby we want dsa_loop.o to follow the value of CONFIG_NET_DSA_LOOP,
> >> and we want dsa_loop_bdinfo.o to be either built as a module or built
> >> into the kernel and we want to follow the value of CONFIG_FIXED_PHY
> >> because there is a functional dependency between the two objects.
> >>
> >> Prior to Olek's change this would work just fine because we would always
> >> descend into drivers/net/dsa/ but after his change, and assuming that
> >> CONFIG_NET_DSA=m which is the case, then we no longer get
> >> dsa_loop_bdinfo.o to be built at all when CONFIG_FIXED_PHY=y.
> >> Essentially only obj-m rules are being processed, obj-y rules are not.
> >>
> >> That does not really seem intuitive to me as to why any suggestions on
> >> how to fix that, short of unconditionally descending into the tree like
> >> we used to?
> >
> >
> >
> > "obj-m += dsa/" means everything under dsa/ must be modular.
> >
> >
> >
> > If there is a built-in object under dsa/ with CONFIG_NET_DSA=m,
> > you cannot do "obj-$(CONFIG_NET_DSA) += dsa/".
> >
> >
> > You need to change it back to "obj-y += dsa/".
>
> Thanks, posted a patch doing that! Is there anyway that Kbuild could be
> warning about such a situation?
I posted such a patch a few years ago.
https://lore.kernel.org/lkml/[email protected]/#t
but I noticed some corner cases where false alarms are reported.
I can revisit it when I have spare time.
--
Best Regards
Masahiro Yamada
On Wed, May 15, 2024 at 8:27 PM Florian Fainelli <[email protected]> wrote:
> What changes have you done to the dsa_loop.c file for this error to show
> up? Currently the driver does the following:
>
> static void dsa_loop_phylink_get_caps(struct dsa_switch *dsa, int port,
> struct phylink_config *config)
> {
> bitmap_fill(config->supported_interfaces,
> PHY_INTERFACE_MODE_MAX);
> __clear_bit(PHY_INTERFACE_MODE_NA, config->supported_interfaces);
> config->mac_capabilities = ~0;
> }
>
> which is basically to say: I support everything, except
> PHY_INTERFACE_MDOE_NA.
I am currently on kernel version 6.1.55 which does not appear to have
this caps code in, and neither does 6.1.90.
The following commit to net/dsa/port.c, backported to the 6.1 tree,
appears to fix the issue
https://github.com/torvalds/linux/commit/9945c1fb03a3c9f7e0dcf9aa17041a70e551387a
Steve.