| Message ID | YQxk4jrbm31NM1US@makrotopia.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
> 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
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.
> 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
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>
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(+)