| Message ID | 1369741403-25315-2-git-send-email-alexandre.belloni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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,
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.
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
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
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
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
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
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,
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
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);
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(-)