[PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Paolo Abeni]

This is another attempt at fixing:

>> ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
>> ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!

We can't select PHYLINK, or that will trigger a circular dependency
PHYLINK already selects PHYLIB, which in turn selects MDIO_DEVICE:
replacing the MDIO_DEVICE dependency with PHYLINK will pull all the
required configs.

Link: https://lore.kernel.org/netdev/20220620201915.1195280-1-kuba@kernel.org/
Reported-by: kernel test robot <lkp@intel.com>
Fixes: b47aec885bcd ("net: pcs: xpcs: add CL37 1000BASE-X AN support")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/net/pcs/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..70fd6a03e982 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -7,7 +7,7 @@ menu "PCS device drivers"
 
 config PCS_XPCS
 	tristate "Synopsys DesignWare XPCS controller"
-	depends on MDIO_DEVICE && MDIO_BUS
+	depends on PHYLINK && MDIO_BUS
 	help
 	  This module provides helper functions for Synopsys DesignWare XPCS
 	  controllers.
-- 
2.35.3

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Jakub Kicinski]

On Tue, 21 Jun 2022 09:55:35 +0200 Paolo Abeni wrote:
> This is another attempt at fixing:
> 
> >> ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
> >> ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!  
> 
> We can't select PHYLINK, or that will trigger a circular dependency
> PHYLINK already selects PHYLIB, which in turn selects MDIO_DEVICE:
> replacing the MDIO_DEVICE dependency with PHYLINK will pull all the
> required configs.

We can't use depends with PHYLINK, AFAIU, because PHYLINK is not 
a user-visible knob. Its always "select"ed and does not show up
in {x,n,menu}config.

If there is no string after bool/tristate the option is not visible 
to the user:

config PHYLINK                                                                  
        tristate  
        depends on NETDEVICES                                                   
        select PHYLIB       

> Link: https://lore.kernel.org/netdev/20220620201915.1195280-1-kuba@kernel.org/
> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: b47aec885bcd ("net: pcs: xpcs: add CL37 1000BASE-X AN support")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  drivers/net/pcs/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 22ba7b0b476d..70fd6a03e982 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -7,7 +7,7 @@ menu "PCS device drivers"
>  
>  config PCS_XPCS
>  	tristate "Synopsys DesignWare XPCS controller"
> -	depends on MDIO_DEVICE && MDIO_BUS
> +	depends on PHYLINK && MDIO_BUS
>  	help
>  	  This module provides helper functions for Synopsys DesignWare XPCS
>  	  controllers.

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Paolo Abeni]

On Tue, 2022-06-21 at 12:50 -0700, Jakub Kicinski wrote:
> On Tue, 21 Jun 2022 09:55:35 +0200 Paolo Abeni wrote:
> > This is another attempt at fixing:
> > 
> > > > ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
> > > > ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!  
> > 
> > We can't select PHYLINK, or that will trigger a circular dependency
> > PHYLINK already selects PHYLIB, which in turn selects MDIO_DEVICE:
> > replacing the MDIO_DEVICE dependency with PHYLINK will pull all the
> > required configs.
> 
> We can't use depends with PHYLINK, AFAIU, because PHYLINK is not 
> a user-visible knob. Its always "select"ed and does not show up
> in {x,n,menu}config.

So I guess we should resort to 'select'ing all the needed dependencies
(we can't mix select and depend on, as per kconfig documentation)

I'll give it another shot before surrender

Thanks!

Paolo

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Russell King (Oracle)]

On Tue, Jun 21, 2022 at 12:50:45PM -0700, Jakub Kicinski wrote:
> On Tue, 21 Jun 2022 09:55:35 +0200 Paolo Abeni wrote:
> > This is another attempt at fixing:
> > 
> > >> ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
> > >> ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!  
> > 
> > We can't select PHYLINK, or that will trigger a circular dependency
> > PHYLINK already selects PHYLIB, which in turn selects MDIO_DEVICE:
> > replacing the MDIO_DEVICE dependency with PHYLINK will pull all the
> > required configs.
> 
> We can't use depends with PHYLINK, AFAIU, because PHYLINK is not 
> a user-visible knob. Its always "select"ed and does not show up
> in {x,n,menu}config.

I'm not sure I understand the point you're making. You seem to be
saying we can't use "depend on PHYLINK" for this PCS driver, but
then you sent a patch doing exactly that.

As these PCS drivers are only usable if PHYLINK is already enabled,
there is a clear dependency between them and phylink. The drivers
that make use of xpcs are:

stmmac, which selects both PCS_XPCS and PHYLINK.
sja1105 (dsa driver), which selects PCS_XPCS. All DSA drivers depend
on NET_DSA, and NET_DSA selects PHYLINK.

So, for PCS_XPCS, PHYLINK will be enabled whenever PCS_XPCS is
selected. No other drivers in drivers/net appear to make use of
the XPCS driver (I couldn't find any other references to
xpcs_create()) so using "depends on PHYLINK" for it should be safe.

Moreover, the user-visible nature of PCS_XPCS doesn't add anything
to the kernel - two drivers require PCS_XPCS due to code references
to the xpcs code, these two select that symbol. Offering it to the
user just gives the user an extra knob to twiddle with no useful
result (other than more files to be built.)

It could be argued that it helps compile coverage, which I think is
the only reason to make PCS_XPCS visible... but then we get compile
coverage when stmmac or sja1105 are enabled.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Jakub Kicinski]

On Wed, 22 Jun 2022 15:15:46 +0100 Russell King (Oracle) wrote:
> > We can't use depends with PHYLINK, AFAIU, because PHYLINK is not 
> > a user-visible knob. Its always "select"ed and does not show up
> > in {x,n,menu}config.  
> 
> I'm not sure I understand the point you're making. You seem to be
> saying we can't use "depend on PHYLINK" for this PCS driver, but
> then you sent a patch doing exactly that.

Nuh uh, I sent a patch which does _select_ PHYLINK.

My concern is that since PHYLINK is not visible user will not be able
to see PCS_XPCS unless something else already enabled PHYLINK.

I may well be missing some higher level relations here, on the surface
"depending" on a symbol which is not user-visible seems.. unusual.

> As these PCS drivers are only usable if PHYLINK is already enabled,
> there is a clear dependency between them and phylink. The drivers
> that make use of xpcs are:
> 
> stmmac, which selects both PCS_XPCS and PHYLINK.
> sja1105 (dsa driver), which selects PCS_XPCS. All DSA drivers depend
> on NET_DSA, and NET_DSA selects PHYLINK.
> 
> So, for PCS_XPCS, PHYLINK will be enabled whenever PCS_XPCS is
> selected. No other drivers in drivers/net appear to make use of
> the XPCS driver (I couldn't find any other references to
> xpcs_create()) so using "depends on PHYLINK" for it should be safe.
> 
> Moreover, the user-visible nature of PCS_XPCS doesn't add anything
> to the kernel - two drivers require PCS_XPCS due to code references
> to the xpcs code, these two select that symbol. Offering it to the
> user just gives the user an extra knob to twiddle with no useful
> result (other than more files to be built.)
> 
> It could be argued that it helps compile coverage, which I think is
> the only reason to make PCS_XPCS visible... but then we get compile
> coverage when stmmac or sja1105 are enabled.

Interesting, hiding PCS_XPCS sounds good then. PCS_LYNX is not visible.

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..9eb32220efea 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -6,7 +6,7 @@
 menu "PCS device drivers"
 
 config PCS_XPCS
-	tristate "Synopsys DesignWare XPCS controller"
+	tristate
 	depends on MDIO_DEVICE && MDIO_BUS
 	help
 	  This module provides helper functions for Synopsys DesignWare XPCS

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Paolo Abeni]

On Wed, 2022-06-22 at 08:35 -0700, Jakub Kicinski wrote:
> On Wed, 22 Jun 2022 15:15:46 +0100 Russell King (Oracle) wrote:
> > > We can't use depends with PHYLINK, AFAIU, because PHYLINK is not 
> > > a user-visible knob. Its always "select"ed and does not show up
> > > in {x,n,menu}config.  
> > 
> > I'm not sure I understand the point you're making. You seem to be
> > saying we can't use "depend on PHYLINK" for this PCS driver, but
> > then you sent a patch doing exactly that.
> 
> Nuh uh, I sent a patch which does _select_ PHYLINK.
> 
> My concern is that since PHYLINK is not visible user will not be able
> to see PCS_XPCS unless something else already enabled PHYLINK.
> 
> I may well be missing some higher level relations here, on the surface
> "depending" on a symbol which is not user-visible seems.. unusual.
> 
> > As these PCS drivers are only usable if PHYLINK is already enabled,
> > there is a clear dependency between them and phylink. The drivers
> > that make use of xpcs are:
> > 
> > stmmac, which selects both PCS_XPCS and PHYLINK.
> > sja1105 (dsa driver), which selects PCS_XPCS. All DSA drivers depend
> > on NET_DSA, and NET_DSA selects PHYLINK.
> > 
> > So, for PCS_XPCS, PHYLINK will be enabled whenever PCS_XPCS is
> > selected. No other drivers in drivers/net appear to make use of
> > the XPCS driver (I couldn't find any other references to
> > xpcs_create()) so using "depends on PHYLINK" for it should be safe.
> > 
> > Moreover, the user-visible nature of PCS_XPCS doesn't add anything
> > to the kernel - two drivers require PCS_XPCS due to code references
> > to the xpcs code, these two select that symbol. Offering it to the
> > user just gives the user an extra knob to twiddle with no useful
> > result (other than more files to be built.)
> > 
> > It could be argued that it helps compile coverage, which I think is
> > the only reason to make PCS_XPCS visible... but then we get compile
> > coverage when stmmac or sja1105 are enabled.
> 
> Interesting, hiding PCS_XPCS sounds good then. PCS_LYNX is not visible.
> 
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 22ba7b0b476d..9eb32220efea 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -6,7 +6,7 @@
>  menu "PCS device drivers"
>  
>  config PCS_XPCS
> -	tristate "Synopsys DesignWare XPCS controller"
> +	tristate
>  	depends on MDIO_DEVICE && MDIO_BUS
>  	help
>  	  This module provides helper functions for Synopsys DesignWare XPCS
> 
@Jakub: please let me know if you prefer to go ahead yourself, or me
sending a v3 with 'depends PHYLINK' + the above (or any other option ;)

Thanks!

Paolo

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Jakub Kicinski]

On Wed, 22 Jun 2022 17:42:13 +0200 Paolo Abeni wrote:
> @Jakub: please let me know if you prefer to go ahead yourself, or me
> sending a v3 with 'depends PHYLINK' + the above (or any other option ;)

[resending, sorry, Russell let me know that my MUA broke the headers]

Well, IDK. You said "depends PHYLINK" which makes me feel like I
haven't convinced you at all :)  Unless you mean add the dependency
on the consumers not on PCS_XPCS itself, but that's awkward.

What I was saying is that "depends" in a symbol which is only
"select"ed by other symbols makes no sense. IIUC "select" does not
visit dependencies, so putting "depends" on an user-invisible symbol
(i.e. symbol without a prompt) achieves nothing.

So PCS_XPCS can have no "depends" if we hide it.

The way I see it - PHYLINK already selects MDIO_DEVICE. So we can drop
the MDIO business from PCS_XPCS, add "select PHYLINK", hide it by
removing the prompt, and we're good. Then again, I admit I have not
tested this at all so I could be speaking gibberish...

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..f778e5155fae 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -6,8 +6,8 @@
 menu "PCS device drivers"
 
 config PCS_XPCS
-       tristate "Synopsys DesignWare XPCS controller"
-       depends on MDIO_DEVICE && MDIO_BUS
+       tristate
+       select PHYLINK
        help
          This module provides helper functions for Synopsys DesignWare XPCS

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Paolo Abeni]

On Wed, 2022-06-22 at 16:12 -0700, Jakub Kicinski wrote:
> On Wed, 22 Jun 2022 17:42:13 +0200 Paolo Abeni wrote:
> > @Jakub: please let me know if you prefer to go ahead yourself, or me
> > sending a v3 with 'depends PHYLINK' + the above (or any other option ;)
> 
> [resending, sorry, Russell let me know that my MUA broke the headers]
> 
> Well, IDK. You said "depends PHYLINK" which makes me feel like I
> haven't convinced you at all :)  Unless you mean add the dependency
> on the consumers not on PCS_XPCS itself, but that's awkward.

Well, I have misread your previous email.
> 
> What I was saying is that "depends" in a symbol which is only
> "select"ed by other symbols makes no sense. IIUC "select" does not
> visit dependencies, so putting "depends" on an user-invisible symbol
> (i.e. symbol without a prompt) achieves nothing.
> 
> So PCS_XPCS can have no "depends" if we hide it.
> 
> The way I see it - PHYLINK already selects MDIO_DEVICE. So we can drop
> the MDIO business from PCS_XPCS, add "select PHYLINK", hide it by
> removing the prompt, and we're good. Then again, I admit I have not
> tested this at all so I could be speaking gibberish...
> 
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 22ba7b0b476d..f778e5155fae 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -6,8 +6,8 @@
>  menu "PCS device drivers"
>  
>  config PCS_XPCS
> -       tristate "Synopsys DesignWare XPCS controller"
> -       depends on MDIO_DEVICE && MDIO_BUS
> +       tristate
> +       select PHYLINK
>         help
>           This module provides helper functions for Synopsys DesignWare XPCS
> 

AFAICS it should works, thanks

Paolo

Re: [PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 23 Jun 2022 13:29:33 -0700 you wrote:
> This is yet another attempt at fixing:
> 
> >> ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
> >> ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!
> 
> Switch XPCS to be invisible, as Russell points out it's
> "selected" by its consumers. Drop the dependency on MDIO_BUS
> as "depends" is meaningless on "selected" symbols.
> 
> [...]

Here is the summary with links:
  - [net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig
    https://git.kernel.org/netdev/net-next/c/ebeae54d3a77

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

[PATCH net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig

[Jakub Kicinski]

This is yet another attempt at fixing:

>> ERROR: modpost: "phylink_mii_c22_pcs_encode_advertisement" [drivers/net/pcs/pcs_xpcs.ko] undefined!
>> ERROR: modpost: "phylink_mii_c22_pcs_decode_state" [drivers/net/pcs/pcs_xpcs.ko] undefined!

Switch XPCS to be invisible, as Russell points out it's
"selected" by its consumers. Drop the dependency on MDIO_BUS
as "depends" is meaningless on "selected" symbols.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: b47aec885bcd ("net: pcs: xpcs: add CL37 1000BASE-X AN support")
Link: https://lore.kernel.org/netdev/20220620201915.1195280-1-kuba@kernel.org/
Link: https://lore.kernel.org/r/20220622083521.0de3ea5c@kernel.org/
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: andrew@lunn.ch
CC: hkallweit1@gmail.com
CC: linux@armlinux.org.uk
CC: rmk+kernel@armlinux.org.uk
CC: boon.leong.ong@intel.com
---
 drivers/net/pcs/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
index 22ba7b0b476d..f778e5155fae 100644
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig
@@ -6,8 +6,8 @@
 menu "PCS device drivers"
 
 config PCS_XPCS
-	tristate "Synopsys DesignWare XPCS controller"
-	depends on MDIO_DEVICE && MDIO_BUS
+	tristate
+	select PHYLINK
 	help
 	  This module provides helper functions for Synopsys DesignWare XPCS
 	  controllers.
-- 
2.36.1