[PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[Daniel Golle]

After commit 83216e3988cd1 ("of: net: pass the dst buffer to
of_get_mac_address()") build fails for kirkwood as ETH_ALEN is not
defined.

arch/arm/mach-mvebu/kirkwood.c: In function 'kirkwood_dt_eth_fixup':
arch/arm/mach-mvebu/kirkwood.c:87:13: error: 'ETH_ALEN' undeclared (first use in this function); did you mean 'ESTALE'?
   u8 tmpmac[ETH_ALEN];
             ^~~~~~~~
             ESTALE
arch/arm/mach-mvebu/kirkwood.c:87:13: note: each undeclared identifier is reported only once for each function it appears in
arch/arm/mach-mvebu/kirkwood.c:87:6: warning: unused variable 'tmpmac' [-Wunused-variable]
   u8 tmpmac[ETH_ALEN];
      ^~~~~~
make[5]: *** [scripts/Makefile.build:262: arch/arm/mach-mvebu/kirkwood.o] Error 1
make[5]: *** Waiting for unfinished jobs....

Add missing #include <linux/if_ether.h> to fix this.

Cc: David S. Miller <davem@davemloft.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Michael Walle <michael@walle.cc>
Reported-by: https://buildbot.openwrt.org/master/images/#/builders/56/builds/220/steps/44/logs/stdio
Fixes: 83216e3988cd1 ("of: net: pass the dst buffer to of_get_mac_address()")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 arch/arm/mach-mvebu/kirkwood.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
index 06b1706595f4c..a493a896e6ee3 100644
--- a/arch/arm/mach-mvebu/kirkwood.c
+++ b/arch/arm/mach-mvebu/kirkwood.c
@@ -14,6 +14,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/mbus.h>
+#include <linux/if_ether.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_net.h>
-- 
2.32.0

Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[Michael Walle]

Hi,

Am 2021-08-06 00:23, schrieb Daniel Golle:
> After commit 83216e3988cd1 ("of: net: pass the dst buffer to
> of_get_mac_address()") build fails for kirkwood as ETH_ALEN is not
> defined.
> 
> arch/arm/mach-mvebu/kirkwood.c: In function 'kirkwood_dt_eth_fixup':
> arch/arm/mach-mvebu/kirkwood.c:87:13: error: 'ETH_ALEN' undeclared
> (first use in this function); did you mean 'ESTALE'?
>    u8 tmpmac[ETH_ALEN];
>              ^~~~~~~~
>              ESTALE
> arch/arm/mach-mvebu/kirkwood.c:87:13: note: each undeclared identifier
> is reported only once for each function it appears in
> arch/arm/mach-mvebu/kirkwood.c:87:6: warning: unused variable 'tmpmac'
> [-Wunused-variable]
>    u8 tmpmac[ETH_ALEN];
>       ^~~~~~
> make[5]: *** [scripts/Makefile.build:262:
> arch/arm/mach-mvebu/kirkwood.o] Error 1
> make[5]: *** Waiting for unfinished jobs....
> 
> Add missing #include <linux/if_ether.h> to fix this.
> 
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Michael Walle <michael@walle.cc>
> Reported-by:
> https://buildbot.openwrt.org/master/images/#/builders/56/builds/220/steps/44/logs/stdio
> Fixes: 83216e3988cd1 ("of: net: pass the dst buffer to 
> of_get_mac_address()")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

What kernel is this? I've just tested with this exact commit as
base and it compiles just fine.

I'm not saying including the file is wrong, but it seems it isn't
needed in the upstream kernel and I don't know if it qualifies for
the stable queue therefore.

-michael

Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[Andrew Lunn]

> What kernel is this? I've just tested with this exact commit as
> base and it compiles just fine.
> 
> I'm not saying including the file is wrong, but it seems it isn't
> needed in the upstream kernel and I don't know if it qualifies for
> the stable queue therefore.

I would like to see a reproducer for mainline. Do you have a kernel
config which generates the problem.

The change itself does seems reasonable, so if we can reproduce it, i
would be happy to merge it for stable.

      Andrew

Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[Daniel Golle]

Hi Andrew,

On Sat, Aug 07, 2021 at 04:17:44PM +0200, Andrew Lunn wrote:
> > What kernel is this? I've just tested with this exact commit as
> > base and it compiles just fine.
> > 
> > I'm not saying including the file is wrong, but it seems it isn't
> > needed in the upstream kernel and I don't know if it qualifies for
> > the stable queue therefore.
> 
> I would like to see a reproducer for mainline. Do you have a kernel
> config which generates the problem.

I encountered the problem when building the 'kirkwood' target in
OpenWrt. I have now tried building vanilla, and the problem indeed
doesn't exist. After tracing the header includes with the precompiler
for some time I concluded that <linux/of_net.h> included in kirkwood.c
includes <linux/phy.h> which includes <linux/ethtool.h> which includes
<uapi/linux/ethtool.h> which includes <uapi/linux/if_ether.h> which
includes <linux/if_ether.h> which defined ETH_ALEN.

When building OpenWrt kernel which includes a backport of
"of: net: pass the dst buffer to of_get_mac_address()", this is not the
same as <linux/of_net.h> doesn't include <linux/phy.h> yet. This is
because we miss commit 0c65b2b90d13c1 ("net: of_get_phy_mode: Change
API to solve int/unit warnings") which has been in mainline for a long
time.

> The change itself does seems reasonable, so if we can reproduce it, i
> would be happy to merge it for stable.

Sorry for the noise caused, I'm not sure what the policy is in this
case, but certainly this is *not* a regression which should make it to
stable asap. The long and confusing chain of includes which lead to the
ETH_ALEN macro being defined in arch/arm/mach-mvebu/kirkwood.c is
certainly not ideal, and in case you still consider this patch worth
merging, I will post v2 with re-written commit description.

Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN

[Andrew Lunn]

> When building OpenWrt kernel which includes a backport of
> "of: net: pass the dst buffer to of_get_mac_address()", this is not the
> same as <linux/of_net.h> doesn't include <linux/phy.h> yet. This is
> because we miss commit 0c65b2b90d13c1 ("net: of_get_phy_mode: Change
> API to solve int/unit warnings") which has been in mainline for a long
> time.

That is quiet a big invasive patch, so i can understand it not being
backported. But on the flip side, it will make it harder getting
drivers upstream to mainline. And there are is one other big change,
how the MAC address is fetches from EEPROM, DT, etc.

> Sorry for the noise caused, I'm not sure what the policy is in this
> case

There is nothing in the coding style that all headers must be directly
included in the .c file. And it slows down the compiler having to pull
in a header file multiple times. You do see patches removing unused
includes. So i think OpenWRT should add yet another patch do deal with
its own breakage.

If you have more kirkwood, or Marvell boards in general in OpenWRT
which you want merged to mainline, i'm happy to review them.

      Andrew