Re: drivers/net/dsa/dsa_loop_bdinfo.c build problems

Re: drivers/net/dsa/dsa_loop_bdinfo.c build problems

[Florian Fainelli]

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 <andrew@lunn.ch> 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

Re: drivers/net/dsa/dsa_loop_bdinfo.c build problems

[Masahiro Yamada]

On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <f.fainelli@gmail.com> 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 <andrew@lunn.ch> 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

Re: drivers/net/dsa/dsa_loop_bdinfo.c build problems

[Florian Fainelli]

On 5/15/2024 7:17 PM, Masahiro Yamada wrote:
> On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <f.fainelli@gmail.com> 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 <andrew@lunn.ch> 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

Re: drivers/net/dsa/dsa_loop_bdinfo.c build problems

[Masahiro Yamada]

On Thu, May 16, 2024 at 12:35 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 5/15/2024 7:17 PM, Masahiro Yamada wrote:
> > On Thu, May 16, 2024 at 4:21 AM Florian Fainelli <f.fainelli@gmail.com> 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 <andrew@lunn.ch> 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/20190912162254.9603-2-yamada.masahiro@socionext.com/#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