[PATCHv2 0/3] net: phy: prevent linking breakage

[PATCHv2 0/3] net: phy: prevent linking breakage

[Alexandre Belloni]

There is a linking issue when using phy_register_fixup{,_for_uid,_for_id} and
CONFIG_PHYLIB is not a builtin:

arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
:(.text+0x1174): undefined reference to `mdiobus_write'
:(.text+0x1188): undefined reference to `mdiobus_write'
:(.text+0x119c): undefined reference to `mdiobus_write'
:(.text+0x11b0): undefined reference to `mdiobus_write'
arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
:(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'

This has been solved for arch/arm/mach-mxs/ and arch/arm/mach-imx/ by testing
IS_BUILTIN(CONFIG_PHYLIB) before calling the functions.

The first patch is an attempt at solving that issue globally.

The following patches remove the unnecessary IS_BUILTIN(CONFIG_PHYLIB) checks.

Quickly greping into the code shows that the issue may arise in:

arch/powerpc/platforms/85xx/mpc85xx_mds.c
arch/arm/mach-davinci/board-dm644x-evm.c
arch/arm/mach-orion5x/dns323-setup.c
arch/arm/mach-at91/board-dt-sama5.c

Changes in v2:
 - indentation fixes
 - use static inline function instead of defines and return -ENOTSUPP

Alexandre Belloni (3):
  net: phy: prevent linking breakage
  arm: mxs: don't check for CONFIG_PHYLIB as builtin
  arm: imx: don't check for CONFIG_PHYLIB as builtin

 arch/arm/mach-imx/mach-imx6q.c | 23 ++++++++++-------------
 arch/arm/mach-mxs/mach-mxs.c   |  5 ++---
 drivers/net/phy/phy_device.c   |  6 ++++++
 include/linux/phy.h            | 26 ++++++++++++++++++++++++++
 4 files changed, 44 insertions(+), 16 deletions(-)

-- 
1.8.1.2

[PATCHv2 1/3] net: phy: prevent linking breakage

[Alexandre Belloni]

phy_register_fixup{,_for_uid,_for_id} are called from arch/, quite
often, there is no protection to check whether CONFIG_PHYLIB=y which is
the only case where this would work. Having phylib as a module or not
compiled at all will result in that kind of linking failure:

arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
:(.text+0x1174): undefined reference to `mdiobus_write'
:(.text+0x1188): undefined reference to `mdiobus_write'
:(.text+0x119c): undefined reference to `mdiobus_write'
:(.text+0x11b0): undefined reference to `mdiobus_write'
arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
:(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/net/phy/phy_device.c |  6 ++++++
 include/linux/phy.h          | 33 ++++++++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3657b4a..df36367 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -64,6 +64,11 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 			     u32 flags, phy_interface_t interface);
 
 /*
+ *  phy_register_fixup{,_for_uid,_for_id} are called from arch/ so this won't
+ *  work unless phylib is compiled in the kernel.
+ */
+#ifdef CONFIG_PHYLIB
+/*
  * Creates a new phy_fixup and adds it to the list
  * @bus_id: A string which matches phydev->dev.bus_id (or PHY_ANY_ID)
  * @phy_uid: Used to match against phydev->phy_id (the UID of the PHY)
@@ -109,6 +114,7 @@ int phy_register_fixup_for_id(const char *bus_id,
 	return phy_register_fixup(bus_id, PHY_ANY_UID, 0xffffffff, run);
 }
 EXPORT_SYMBOL(phy_register_fixup_for_id);
+#endif /* CONFIG_PHYLIB */
 
 /*
  * Returns 1 if fixup matches phydev in bus_id and phy_uid.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9e11039..2cc3383 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -556,12 +556,39 @@ int phy_start_interrupts(struct phy_device *phydev);
 void phy_print_status(struct phy_device *phydev);
 void phy_device_free(struct phy_device *phydev);
 
+/*
+ *  phy_register_fixup{,_for_uid,_for_id} are called from arch/ so this won't
+ *  work unless phylib is compiled in the kernel.
+ *  Defining stubs allows to prevent linking errors.
+ */
+#ifdef CONFIG_PHYLIB
 int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask,
-		int (*run)(struct phy_device *));
+		       int (*run)(struct phy_device *));
 int phy_register_fixup_for_id(const char *bus_id,
-		int (*run)(struct phy_device *));
+			      int (*run)(struct phy_device *));
 int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
-		int (*run)(struct phy_device *));
+			       int (*run)(struct phy_device *));
+#else
+static inline int phy_register_fixup(const char *bus_id, u32 phy_uid,
+				     u32 phy_uid_mask,
+				     int (*run)(struct phy_device *))
+{
+	return -ENOTSUPP;
+}
+
+static inline int phy_register_fixup_for_id(const char *bus_id,
+					    int (*run)(struct phy_device *))
+{
+	return -ENOTSUPP;
+}
+
+static inline int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
+					     int (*run)(struct phy_device *))
+{
+	return -ENOTSUPP;
+}
+#endif /* CONFIG_PHYLIB */
+
 int phy_scan_fixups(struct phy_device *phydev);
 
 int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable);
-- 
1.8.1.2

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[David Miller]

From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Date: Tue, 28 May 2013 13:43:21 +0200

> phy_register_fixup{,_for_uid,_for_id} are called from arch/, quite
> often, there is no protection to check whether CONFIG_PHYLIB=y which is
> the only case where this would work. Having phylib as a module or not
> compiled at all will result in that kind of linking failure:
> 
> arch/arm/mach-at91/built-in.o: In function `ksz9021rn_phy_fixup':
> :(.text+0x1174): undefined reference to `mdiobus_write'
> :(.text+0x1188): undefined reference to `mdiobus_write'
> :(.text+0x119c): undefined reference to `mdiobus_write'
> :(.text+0x11b0): undefined reference to `mdiobus_write'
> arch/arm/mach-at91/built-in.o: In function `sama5_dt_device_init':
> :(.init.text+0x1e34): undefined reference to `phy_register_fixup_for_uid'
> 
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

This is the wrong way to go about this.

If the arch code absolutely requires CONFIG_PHYLIB=y then express that
dependency in the arch Kconfig.  The arch code in question should not
be compiled at all if CONFIG_PHYLIB has an incompatible setting.

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Alexandre Belloni]

Hi,

On 28/05/2013 22:09, David Miller wrote:
>
> This is the wrong way to go about this.
>
> If the arch code absolutely requires CONFIG_PHYLIB=y then express that
> dependency in the arch Kconfig.  The arch code in question should not
> be compiled at all if CONFIG_PHYLIB has an incompatible setting.
But that is making it impossible to compile a kernel without any network
stack for those platforms or we are going back to either enclosing the
calls to phy_register_fixup{,_for_uid,_for_id} with #ifdef CONFIG_PHYLIB
or if(IS_BUILTIN(CONFIG_PHYLIB)). And as you can see, it is quite error
prone and is done only done for 2 platforms on a total of 6. I believe
fixing that in phy.h is more foolproof.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[David Miller]

From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
Date: Wed, 29 May 2013 10:21:26 +0200

> On 28/05/2013 22:09, David Miller wrote:
>>
>> This is the wrong way to go about this.
>>
>> If the arch code absolutely requires CONFIG_PHYLIB=y then express that
>> dependency in the arch Kconfig.  The arch code in question should not
>> be compiled at all if CONFIG_PHYLIB has an incompatible setting.
> But that is making it impossible to compile a kernel without any network
> stack for those platforms or we are going back to either enclosing the
> calls to phy_register_fixup{,_for_uid,_for_id} with #ifdef CONFIG_PHYLIB
> or if(IS_BUILTIN(CONFIG_PHYLIB)). And as you can see, it is quite error
> prone and is done only done for 2 platforms on a total of 6. I believe
> fixing that in phy.h is more foolproof.

Or you properly segregate the networking bits of the platform code so
that it can be built only when the necessary networking portions are
enabled.

Sometimes having dummy stubs makes sense, but not in this situation.

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Arnd Bergmann]

On Thursday 30 May 2013 02:42:01 David Miller wrote:
> From: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > On 28/05/2013 22:09, David Miller wrote:
> > But that is making it impossible to compile a kernel without any network
> > stack for those platforms or we are going back to either enclosing the
> > calls to phy_register_fixup{,_for_uid,_for_id} with #ifdef CONFIG_PHYLIB
> > or if(IS_BUILTIN(CONFIG_PHYLIB)). And as you can see, it is quite error
> > prone and is done only done for 2 platforms on a total of 6. I believe
> > fixing that in phy.h is more foolproof.
> 
> Or you properly segregate the networking bits of the platform code so
> that it can be built only when the necessary networking portions are
> enabled.
> 
> Sometimes having dummy stubs makes sense, but not in this situation.

Currently most users of this function are doing something like

static int foo_phy_fixup(struct phy_device *phydev)
{
	...
}

static int __init boo_board_init(void)
{
	 if (IS_BUILTIN(CONFIG_PHYLIB))
		phy_register_fixup_for_uid(phy_id, foo_phy_fixup);
}

which is practically the same as having a dummy stub. It leads to
the foo_phy_fixup() function always getting compiled and then discarded
by gcc when CONFIG_PHYLIB is disabled.

The method is currently broken when network drivers are enabled as
modules, because we are missing the fixup then.

I think we should use IS_ENABLED() here to force a build error
in that case, and have something like

config ARCH_FOO
	bool "support for the foo platform"
	select PHYLIB if NET

in the platform Kconfig file, to ensure PHYLIB is always built-in.

I still think the inline alternatives would be helpful, but using
if (IS_ENABLED(CONFIG_PHYLIB)) in the platform code would also
work.

	Arnd

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Florian Fainelli]

2013/6/4 Arnd Bergmann <arnd@arndb.de>:
>> Or you properly segregate the networking bits of the platform code so
>> that it can be built only when the necessary networking portions are
>> enabled.
>>
>> Sometimes having dummy stubs makes sense, but not in this situation.
>
> Currently most users of this function are doing something like
>
> static int foo_phy_fixup(struct phy_device *phydev)
> {
>         ...
> }
>
> static int __init boo_board_init(void)
> {
>          if (IS_BUILTIN(CONFIG_PHYLIB))
>                 phy_register_fixup_for_uid(phy_id, foo_phy_fixup);
> }
>
> which is practically the same as having a dummy stub. It leads to
> the foo_phy_fixup() function always getting compiled and then discarded
> by gcc when CONFIG_PHYLIB is disabled.
>
> The method is currently broken when network drivers are enabled as
> modules, because we are missing the fixup then.
>
> I think we should use IS_ENABLED() here to force a build error
> in that case, and have something like
>
> config ARCH_FOO
>         bool "support for the foo platform"
>         select PHYLIB if NET
>
> in the platform Kconfig file, to ensure PHYLIB is always built-in.
>
> I still think the inline alternatives would be helpful, but using
> if (IS_ENABLED(CONFIG_PHYLIB)) in the platform code would also
> work.

It seems to me that what David proposes is to have say an
arch/arm/mach-foo/phy-fixups.c file which is only enabled when
CONFIG_PHYLIB is set (obj-$(CONFIG_PHYLIB) += phy-fixup.o), such that
it does not need to have any conditionnals when calling
phy_register_fixup. This sounds a little unusual, but why not.
--
Florian

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Arnd Bergmann]

On Tuesday 04 June 2013 16:36:50 Florian Fainelli wrote:
> It seems to me that what David proposes is to have say an
> arch/arm/mach-foo/phy-fixups.c file which is only enabled when
> CONFIG_PHYLIB is set (obj-$(CONFIG_PHYLIB) += phy-fixup.o), such that
> it does not need to have any conditionnals when calling
> phy_register_fixup. This sounds a little unusual, but why not.

I don't think it would actually help us, because then we still need
to declare a local function that gets called from the board init
code. Instead of doing

	if (IS_ENABLED(CONFIG_PHYLIB))
		 phy_register_fixup_for_uid(phy_id, foo_phy_fixup);

we would then do

	if (IS_ENABLED(CONFIG_PHYLIB))
		foo_phy_fixup_register();

which is not much different at all.

	Arnd

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Florian Fainelli]

2013/6/4 Arnd Bergmann <arnd@arndb.de>:
> On Tuesday 04 June 2013 16:36:50 Florian Fainelli wrote:
>> It seems to me that what David proposes is to have say an
>> arch/arm/mach-foo/phy-fixups.c file which is only enabled when
>> CONFIG_PHYLIB is set (obj-$(CONFIG_PHYLIB) += phy-fixup.o), such that
>> it does not need to have any conditionnals when calling
>> phy_register_fixup. This sounds a little unusual, but why not.
>
> I don't think it would actually help us, because then we still need
> to declare a local function that gets called from the board init
> code. Instead of doing
>
>         if (IS_ENABLED(CONFIG_PHYLIB))
>                  phy_register_fixup_for_uid(phy_id, foo_phy_fixup);
>
> we would then do
>
>         if (IS_ENABLED(CONFIG_PHYLIB))
>                 foo_phy_fixup_register();
>
> which is not much different at all.

You would just need to define a stub for your arch_foo_phy_fixup()
which has a different definition depending on whether CONFIG_PHYLIB is
defined or not.

This would be just one function, instead of the whole bunch of stubs
needed for phylib. Right now its probably 1 vs 3, so it does not make
that much of a difference but who knows, if we had more phylib stubs
and forget to update the stubs? (which tends to happen pretty often).

The size savings are exactly the same in both approaches anyway.
--
Florian

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Arnd Bergmann]

On Tuesday 04 June 2013 17:09:26 Florian Fainelli wrote:
> You would just need to define a stub for your arch_foo_phy_fixup()
> which has a different definition depending on whether CONFIG_PHYLIB is
> defined or not.

Yes, same thing. For a function that is called in only one place,
I would always prefer if(IS_ENABLED()) over a stub though.

> This would be just one function, instead of the whole bunch of stubs
> needed for phylib. Right now its probably 1 vs 3, so it does not make
> that much of a difference but who knows, if we had more phylib stubs
> and forget to update the stubs? (which tends to happen pretty often).
> 
> The size savings are exactly the same in both approaches anyway.

So should we just stick to the current method then and use
if (IS_ENABLED(CONFIG_NET)) for calling the function?

	Arnd

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Alexandre Belloni]

On 04/06/2013 18:09, Florian Fainelli wrote:
> 2013/6/4 Arnd Bergmann <arnd@arndb.de>:
>> On Tuesday 04 June 2013 16:36:50 Florian Fainelli wrote:
>>> It seems to me that what David proposes is to have say an
>>> arch/arm/mach-foo/phy-fixups.c file which is only enabled when
>>> CONFIG_PHYLIB is set (obj-$(CONFIG_PHYLIB) += phy-fixup.o), such that
>>> it does not need to have any conditionnals when calling
>>> phy_register_fixup. This sounds a little unusual, but why not.
>> I don't think it would actually help us, because then we still need
>> to declare a local function that gets called from the board init
>> code. Instead of doing
>>
>>         if (IS_ENABLED(CONFIG_PHYLIB))
>>                  phy_register_fixup_for_uid(phy_id, foo_phy_fixup);
>>
>> we would then do
>>
>>         if (IS_ENABLED(CONFIG_PHYLIB))
>>                 foo_phy_fixup_register();
>>
>> which is not much different at all.
> You would just need to define a stub for your arch_foo_phy_fixup()
> which has a different definition depending on whether CONFIG_PHYLIB is
> defined or not.
>
> This would be just one function, instead of the whole bunch of stubs
> needed for phylib. Right now its probably 1 vs 3, so it does not make
> that much of a difference but who knows, if we had more phylib stubs
> and forget to update the stubs? (which tends to happen pretty often).

The fact is that for now it is called on 6 different platforms. Of those:
 - two (imx and mxs) are now doing it right *after* discovering the issue
 - two always selecting PHYLIB and can't be compiled without NET
 - two will break at compile time.

I'm really concerned about getting more platforms getting it wrong. I
think it boils down to who wants to take the maintenance burden. I would
believe that doing it in phylib is more future proof. But I'm fine
with whatever the consensus will be.

However, it is becoming urgent for Atmel to get the fix in 3.10 so that
they never get a kernel compilation that breaks.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCHv2 1/3] net: phy: prevent linking breakage

[Arnd Bergmann]

On Wednesday 05 June 2013, Alexandre Belloni wrote:
> However, it is becoming urgent for Atmel to get the fix in 3.10 so that
> they never get a kernel compilation that breaks.

I don't think it's urgent since this is only about rare configurations,
and not a regression. We can queue the patch "ARM: at91: Fix link
breakage when !CONFIG_PHYLIB" you just sent to fix this, but I
would not consider it a serious problem to have it only in 3.11.

	Arnd

[PATCHv2 2/3] arm: mxs: don't check for CONFIG_PHYLIB as builtin

[Alexandre Belloni]

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/mach-mxs/mach-mxs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index 5b62b64..2dad553 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -273,9 +273,8 @@ static void __init apx4devkit_init(void)
 {
 	enable_clk_enet_out();
 
-	if (IS_BUILTIN(CONFIG_PHYLIB))
-		phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK,
-					   apx4devkit_phy_fixup);
+	phy_register_fixup_for_uid(PHY_ID_KSZ8051, MICREL_PHY_ID_MASK,
+				   apx4devkit_phy_fixup);
 }
 
 #define ENET0_MDC__GPIO_4_0	MXS_GPIO_NR(4, 0)
-- 
1.8.1.2

[PATCHv2 3/3] arm: imx: don't check for CONFIG_PHYLIB as builtin

[Alexandre Belloni]

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 arch/arm/mach-imx/mach-imx6q.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c
index 5536fd8..9094849 100644
--- a/arch/arm/mach-imx/mach-imx6q.c
+++ b/arch/arm/mach-imx/mach-imx6q.c
@@ -99,16 +99,14 @@ soft:
 /* For imx6q sabrelite board: set KSZ9021RN RGMII pad skew */
 static int ksz9021rn_phy_fixup(struct phy_device *phydev)
 {
-	if (IS_BUILTIN(CONFIG_PHYLIB)) {
-		/* min rx data delay */
-		phy_write(phydev, 0x0b, 0x8105);
-		phy_write(phydev, 0x0c, 0x0000);
-
-		/* max rx/tx clock delay, min rx/tx control delay */
-		phy_write(phydev, 0x0b, 0x8104);
-		phy_write(phydev, 0x0c, 0xf0f0);
-		phy_write(phydev, 0x0b, 0x104);
-	}
+	/* min rx data delay */
+	phy_write(phydev, 0x0b, 0x8105);
+	phy_write(phydev, 0x0c, 0x0000);
+
+	/* max rx/tx clock delay, min rx/tx control delay */
+	phy_write(phydev, 0x0b, 0x8104);
+	phy_write(phydev, 0x0c, 0xf0f0);
+	phy_write(phydev, 0x0b, 0x104);
 
 	return 0;
 }
@@ -139,9 +137,8 @@ put_clk:
 
 static void __init imx6q_sabrelite_init(void)
 {
-	if (IS_BUILTIN(CONFIG_PHYLIB))
-		phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
-				ksz9021rn_phy_fixup);
+	phy_register_fixup_for_uid(PHY_ID_KSZ9021, MICREL_PHY_ID_MASK,
+				   ksz9021rn_phy_fixup);
 	imx6q_sabrelite_cko1_setup();
 }
 
-- 
1.8.1.2