| Message ID | 6959a6a51582e8bc2343824d0cee56f1db246e23.1655797997.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: pcs: xpcs: depends on PHYLINK in Kconfig | expand |
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.
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
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.
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
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
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
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
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.