Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN
@ 2021-08-05 22:23 Daniel Golle
  2021-08-06  7:21 ` Michael Walle
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Golle @ 2021-08-05 22:23 UTC (permalink / raw)
  To: linux-arm-kernel, netdev, linux-kernel
  Cc: David S. Miller, Andrew Lunn, Michael Walle

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN
  2021-08-05 22:23 [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN Daniel Golle
@ 2021-08-06  7:21 ` Michael Walle
  2021-08-07 14:17   ` Andrew Lunn
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Walle @ 2021-08-06  7:21 UTC (permalink / raw)
  To: Daniel Golle
  Cc: linux-arm-kernel, netdev, linux-kernel, David S. Miller, Andrew Lunn

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN
  2021-08-06  7:21 ` Michael Walle
@ 2021-08-07 14:17   ` Andrew Lunn
  2021-08-08  3:27     ` Daniel Golle
  0 siblings, 1 reply; 5+ messages in thread
From: Andrew Lunn @ 2021-08-07 14:17 UTC (permalink / raw)
  To: Michael Walle
  Cc: Daniel Golle, linux-arm-kernel, netdev, linux-kernel, David S. Miller

> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN
  2021-08-07 14:17   ` Andrew Lunn
@ 2021-08-08  3:27     ` Daniel Golle
  2021-08-08 15:18       ` Andrew Lunn
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Golle @ 2021-08-08  3:27 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Michael Walle, linux-arm-kernel, netdev, linux-kernel, David S. Miller

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN
  2021-08-08  3:27     ` Daniel Golle
@ 2021-08-08 15:18       ` Andrew Lunn
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Lunn @ 2021-08-08 15:18 UTC (permalink / raw)
  To: Daniel Golle
  Cc: Michael Walle, linux-arm-kernel, netdev, linux-kernel, David S. Miller

> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-08 15:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-05 22:23 [PATCH] ARM: kirkwood: add missing <linux/if_ether.h> for ETH_ALEN Daniel Golle
2021-08-06  7:21 ` Michael Walle
2021-08-07 14:17   ` Andrew Lunn
2021-08-08  3:27     ` Daniel Golle
2021-08-08 15:18       ` Andrew Lunn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).