| Message ID | 20230219150321.2683358-1-trix@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: lan743x: LAN743X selects FIXED_PHY to resolve a link error | expand |
On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > A rand config causes this link error > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > lan743x_netdev_open is controlled by LAN743X > fixed_phy_register is controlled by FIXED_PHY > > So LAN743X should also select FIXED_PHY > > Signed-off-by: Tom Rix <trix@redhat.com> Hi Tom, I am a little confused by this. I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. But I do not see a build failure, and I believe that is because when FIXED_PHY is not set then a stub version of fixed_phy_register(), defined as static inline in include/linux/phy_fixed.h, is used. Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 > --- > drivers/net/ethernet/microchip/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig > index 24c994baad13..43ba71e82260 100644 > --- a/drivers/net/ethernet/microchip/Kconfig > +++ b/drivers/net/ethernet/microchip/Kconfig > @@ -47,6 +47,7 @@ config LAN743X > depends on PCI > depends on PTP_1588_CLOCK_OPTIONAL > select PHYLIB > + select FIXED_PHY > select CRC16 > select CRC32 > help > -- > 2.27.0 >
On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: > On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > > A rand config causes this link error > > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > > > lan743x_netdev_open is controlled by LAN743X > > fixed_phy_register is controlled by FIXED_PHY > > > > So LAN743X should also select FIXED_PHY > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > Hi Tom, > > I am a little confused by this. > > I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. > But I do not see a build failure, and I believe that is because > when FIXED_PHY is not set then a stub version of fixed_phy_register(), > defined as static inline in include/linux/phy_fixed.h, is used. > > Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY is a module? What might be needed is depends on FIXED_PHY || FIXED_PHY=n Andrew
On Mon, Feb 20, 2023 at 02:19:34AM +0100, Andrew Lunn wrote: > On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: > > On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > > > A rand config causes this link error > > > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > > > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > > > > > lan743x_netdev_open is controlled by LAN743X > > > fixed_phy_register is controlled by FIXED_PHY > > > > > > So LAN743X should also select FIXED_PHY > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > Hi Tom, > > > > I am a little confused by this. > > > > I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. > > But I do not see a build failure, and I believe that is because > > when FIXED_PHY is not set then a stub version of fixed_phy_register(), > > defined as static inline in include/linux/phy_fixed.h, is used. > > > > Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 > > I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY > is a module? What might be needed is > > depends on FIXED_PHY || FIXED_PHY=n Thanks Andrew, LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom describes. And his patch does appear to resolve the problem. Unfortunately your proposed solution seems to run foul of a complex dependency situation. $ make ... drivers/net/ethernet/microchip/Kconfig:45:error: recursive dependency detected! drivers/net/ethernet/microchip/Kconfig:45: symbol LAN743X depends on FIXED_PHY drivers/net/phy/Kconfig:48: symbol FIXED_PHY depends on PHYLIB drivers/net/phy/Kconfig:16: symbol PHYLIB is selected by LAN743X For a resolution refer to Documentation/kbuild/kconfig-language.rst subsection "Kconfig recursive dependency limitations"
On Mon, Feb 20, 2023 at 07:46:13AM +0100, Simon Horman wrote: > On Mon, Feb 20, 2023 at 02:19:34AM +0100, Andrew Lunn wrote: > > On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: > > > On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > > > > A rand config causes this link error > > > > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > > > > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > > > > > > > lan743x_netdev_open is controlled by LAN743X > > > > fixed_phy_register is controlled by FIXED_PHY > > > > > > > > So LAN743X should also select FIXED_PHY > > > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > > > Hi Tom, > > > > > > I am a little confused by this. > > > > > > I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. > > > But I do not see a build failure, and I believe that is because > > > when FIXED_PHY is not set then a stub version of fixed_phy_register(), > > > defined as static inline in include/linux/phy_fixed.h, is used. > > > > > > Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 > > > > I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY > > is a module? What might be needed is > > > > depends on FIXED_PHY || FIXED_PHY=n > > Thanks Andrew, > > LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom > describes. And his patch does appear to resolve the problem. O.K. So the commit message needs updating to describe the actual problem. > Unfortunately your proposed solution seems to run foul of a complex > dependency situation. I was never any good at Kconfig. Arnd is the expert at solving problems like this. You want either everything built in, or FIXED_PHY built in and LAN743X modular, or both modular. Andrew
+Arnd On Tue, Feb 21, 2023 at 03:29:39AM +0100, Andrew Lunn wrote: > On Mon, Feb 20, 2023 at 07:46:13AM +0100, Simon Horman wrote: > > On Mon, Feb 20, 2023 at 02:19:34AM +0100, Andrew Lunn wrote: > > > On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: > > > > On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > > > > > A rand config causes this link error > > > > > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > > > > > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > > > > > > > > > lan743x_netdev_open is controlled by LAN743X > > > > > fixed_phy_register is controlled by FIXED_PHY > > > > > > > > > > So LAN743X should also select FIXED_PHY > > > > > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > > > > > Hi Tom, > > > > > > > > I am a little confused by this. > > > > > > > > I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. > > > > But I do not see a build failure, and I believe that is because > > > > when FIXED_PHY is not set then a stub version of fixed_phy_register(), > > > > defined as static inline in include/linux/phy_fixed.h, is used. > > > > > > > > Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 > > > > > > I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY > > > is a module? What might be needed is > > > > > > depends on FIXED_PHY || FIXED_PHY=n > > > > Thanks Andrew, > > > > LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom > > describes. And his patch does appear to resolve the problem. > > O.K. So the commit message needs updating to describe the actual > problem. Yes, that would be a good improvement. Perhaps a fixes tag too? > > Unfortunately your proposed solution seems to run foul of a complex > > dependency situation. > > I was never any good at Kconfig. Arnd is the expert at solving > problems like this. > > You want either everything built in, or FIXED_PHY built in and LAN743X > modular, or both modular. I _think_ the patch, which uses select FIXED_PHY for LAN743X, achieves that. I CCed Arnd in case he has any input. Though I think I read in an recent email from him that he is out most of this week.
On 2/21/23 8:20 AM, Simon Horman wrote: > +Arnd > > On Tue, Feb 21, 2023 at 03:29:39AM +0100, Andrew Lunn wrote: >> On Mon, Feb 20, 2023 at 07:46:13AM +0100, Simon Horman wrote: >>> On Mon, Feb 20, 2023 at 02:19:34AM +0100, Andrew Lunn wrote: >>>> On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: >>>>> On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: >>>>>> A rand config causes this link error >>>>>> drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': >>>>>> drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' >>>>>> >>>>>> lan743x_netdev_open is controlled by LAN743X >>>>>> fixed_phy_register is controlled by FIXED_PHY >>>>>> >>>>>> So LAN743X should also select FIXED_PHY >>>>>> >>>>>> Signed-off-by: Tom Rix <trix@redhat.com> >>>>> Hi Tom, >>>>> >>>>> I am a little confused by this. >>>>> >>>>> I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. >>>>> But I do not see a build failure, and I believe that is because >>>>> when FIXED_PHY is not set then a stub version of fixed_phy_register(), >>>>> defined as static inline in include/linux/phy_fixed.h, is used. >>>>> >>>>> Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 >>>> I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY >>>> is a module? What might be needed is >>>> >>>> depends on FIXED_PHY || FIXED_PHY=n >>> Thanks Andrew, >>> >>> LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom >>> describes. And his patch does appear to resolve the problem. >> O.K. So the commit message needs updating to describe the actual >> problem. > Yes, that would be a good improvement. > > Perhaps a fixes tag too? > >>> Unfortunately your proposed solution seems to run foul of a complex >>> dependency situation. >> I was never any good at Kconfig. Arnd is the expert at solving >> problems like this. >> >> You want either everything built in, or FIXED_PHY built in and LAN743X >> modular, or both modular. > I _think_ the patch, which uses select FIXED_PHY for LAN743X, > achieves that. > > I CCed Arnd in case he has any input. Though I think I read > in an recent email from him that he is out most of this week. I was out last week as well :) I considered this a cleanup rather than a fix, I can add that fixes tag. From my point of view this is a linking problem, I don't mind changing, what commit message should I use ? Tom >
On Sun, Feb 26, 2023, at 16:15, Tom Rix wrote: > On 2/21/23 8:20 AM, Simon Horman wrote: >> On Tue, Feb 21, 2023 at 03:29:39AM +0100, Andrew Lunn wrote: >>> On Mon, Feb 20, 2023 at 07:46:13AM +0100, Simon Horman wrote: >>>> >>>> LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom >>>> describes. And his patch does appear to resolve the problem. >>> O.K. So the commit message needs updating to describe the actual >>> problem. >> Yes, that would be a good improvement. >> >> Perhaps a fixes tag too? >> >>>> Unfortunately your proposed solution seems to run foul of a complex >>>> dependency situation. >>> I was never any good at Kconfig. Arnd is the expert at solving >>> problems like this. >>> >>> You want either everything built in, or FIXED_PHY built in and LAN743X >>> modular, or both modular. >> I _think_ the patch, which uses select FIXED_PHY for LAN743X, >> achieves that. >> >> I CCed Arnd in case he has any input. Though I think I read >> in an recent email from him that he is out most of this week. FWIW, the original patch looks good to me, Reviewed-by: Arnd Bergmann <arnd@arndb.de> I'm not sure if the #else path in include/linux/phy_fixed.h is actually helpful, as it does not avoid a link failure but just makes it less common. Maybe we should just drop that from the header and require all users of fixed_phy to 'select' that symbol even when they are built-in? Arnd
On Sun, Feb 26, 2023 at 07:15:01AM -0800, Tom Rix wrote: > > On 2/21/23 8:20 AM, Simon Horman wrote: > > +Arnd > > > > On Tue, Feb 21, 2023 at 03:29:39AM +0100, Andrew Lunn wrote: > > > On Mon, Feb 20, 2023 at 07:46:13AM +0100, Simon Horman wrote: > > > > On Mon, Feb 20, 2023 at 02:19:34AM +0100, Andrew Lunn wrote: > > > > > On Sun, Feb 19, 2023 at 07:16:07PM +0100, Simon Horman wrote: > > > > > > On Sun, Feb 19, 2023 at 10:03:21AM -0500, Tom Rix wrote: > > > > > > > A rand config causes this link error > > > > > > > drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': > > > > > > > drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' > > > > > > > > > > > > > > lan743x_netdev_open is controlled by LAN743X > > > > > > > fixed_phy_register is controlled by FIXED_PHY > > > > > > > > > > > > > > So LAN743X should also select FIXED_PHY > > > > > > > > > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > > > Hi Tom, > > > > > > > > > > > > I am a little confused by this. > > > > > > > > > > > > I did manage to cook up a config with LAN743X=m and FIXED_PHY not set. > > > > > > But I do not see a build failure, and I believe that is because > > > > > > when FIXED_PHY is not set then a stub version of fixed_phy_register(), > > > > > > defined as static inline in include/linux/phy_fixed.h, is used. > > > > > > > > > > > > Ref: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/phy_fixed.h?h=main&id=675f176b4dcc2b75adbcea7ba0e9a649527f53bd#n42 > > > > > I'n guessing, but it could be that LAN743X is built in, and FIXED_PHY > > > > > is a module? What might be needed is > > > > > > > > > > depends on FIXED_PHY || FIXED_PHY=n > > > > Thanks Andrew, > > > > > > > > LAN743X=y and FIXED_PHY=m does indeed produce the problem that Tom > > > > describes. And his patch does appear to resolve the problem. > > > O.K. So the commit message needs updating to describe the actual > > > problem. > > Yes, that would be a good improvement. > > > > Perhaps a fixes tag too? > > > > > > Unfortunately your proposed solution seems to run foul of a complex > > > > dependency situation. > > > I was never any good at Kconfig. Arnd is the expert at solving > > > problems like this. > > > > > > You want either everything built in, or FIXED_PHY built in and LAN743X > > > modular, or both modular. > > I _think_ the patch, which uses select FIXED_PHY for LAN743X, > > achieves that. > > > > I CCed Arnd in case he has any input. Though I think I read > > in an recent email from him that he is out most of this week. > > I was out last week as well :) :) > I considered this a cleanup rather than a fix, I can add that fixes tag. FWIIW, I'm happy with a cleanup. And in that chase there should be no fixes tag (conversely, if it is a fix, then there should be a fixes tag). As a cleanup, I suggest targeting this at net-next ([PATCH net-next]). As net-next is currently closed you'll need to wait for it to re-open. I expect that will happen next week, after v6.3-rc2 has been tagged. > From my point of view this is a linking problem, I don't mind changing, > what commit message should I use ? I think updating the patch description by adding something like "This problem occurs when LAN743X=y and FIXED_PHY=m." would be sufficient.
diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig index 24c994baad13..43ba71e82260 100644 --- a/drivers/net/ethernet/microchip/Kconfig +++ b/drivers/net/ethernet/microchip/Kconfig @@ -47,6 +47,7 @@ config LAN743X depends on PCI depends on PTP_1588_CLOCK_OPTIONAL select PHYLIB + select FIXED_PHY select CRC16 select CRC32 help
A rand config causes this link error drivers/net/ethernet/microchip/lan743x_main.o: In function `lan743x_netdev_open': drivers/net/ethernet/microchip/lan743x_main.c:1512: undefined reference to `fixed_phy_register' lan743x_netdev_open is controlled by LAN743X fixed_phy_register is controlled by FIXED_PHY So LAN743X should also select FIXED_PHY Signed-off-by: Tom Rix <trix@redhat.com> --- drivers/net/ethernet/microchip/Kconfig | 1 + 1 file changed, 1 insertion(+)