RE: [PATCH net-next v9 5/9] net: txgbe: Add SFP module identify

[Jiawen Wu <jiawenwu@xxxxxxxxxxxxxx>]

On Wednesday, May 31, 2023 5:48 PM, Russell King (Oracle) wrote:
> On Tue, May 30, 2023 at 04:40:36PM +0800, Jiawen Wu wrote:
> > On Monday, May 29, 2023 10:06 AM, Jiawen Wu wrote:
> > > On Friday, May 26, 2023 7:37 PM, Russell King (Oracle) wrote:
> > > > On Fri, May 26, 2023 at 07:30:45PM +0800, kernel test robot wrote:
> > > > > Kconfig warnings: (for reference only)
> > > > >    WARNING: unmet direct dependencies detected for I2C_DESIGNWARE_PLATFORM
> > > > >    Depends on [n]: I2C [=n] && HAS_IOMEM [=y] && (ACPI && COMMON_CLK [=y] || !ACPI)
> > > > >    Selected by [y]:
> > > > >    - TXGBE [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_WANGXUN [=y] && PCI [=y]
> > > > >    WARNING: unmet direct dependencies detected for SFP
> > > > >    Depends on [n]: NETDEVICES [=y] && PHYLIB [=y] && I2C [=n] && PHYLINK [=y] && (HWMON [=n] || HWMON [=n]=n)
> > > > >    Selected by [y]:
> > > > >    - TXGBE [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_WANGXUN [=y] && PCI [=y]
> > > >
> > > > ... and is basically caused by "select SFP". No. Do not do this unless
> > > > you look at the dependencies for SFP and ensure that those are also
> > > > satisfied - because if you don't you create messes like the above
> > > > build errors.
> > >
> > > So how do I make sure that the module I need compiles and loads correctly,
> > > rely on the user to manually select it?
> >
> > When I changed the TXGBE config to:
> > ...
> > 	depends on SFP
> > 	select PCS_XPCS
> > ...
> > the compilation gave an error:
> >
> > drivers/net/phy/Kconfig:16:error: recursive dependency detected!
> > drivers/net/phy/Kconfig:16:     symbol PHYLIB is selected by PHYLINK
> > drivers/net/phy/Kconfig:6:      symbol PHYLINK is selected by PCS_XPCS
> > drivers/net/pcs/Kconfig:8:      symbol PCS_XPCS is selected by TXGBE
> > drivers/net/ethernet/wangxun/Kconfig:40:        symbol TXGBE depends on SFP
> > drivers/net/phy/Kconfig:63:     symbol SFP depends on PHYLIB
> > For a resolution refer to Documentation/kbuild/kconfig-language.rst
> > subsection "Kconfig recursive dependency limitations"
> >
> > Seems deleting "depends on SFP" is the correct way. But is this normal?
> > How do we ensure the dependency between TXGBE and SFP?
> 
> First, I would do this:
> 
> 	select PHYLINK
> 	select PCS_XPCS
> 
> but then I'm principled, and I don't agree that PCS_XPCS should be
> selecting PHYLINK.
> 
> The second thing I don't particularly like is selecting user visible
> symbols, but as I understand it, with TXGBE, the SFP slot is not an
> optional feature, so there's little option.
> 
> So, because SFP requires I2C:
> 
> 	select I2C
> 	select SFP
> 
> That is basically what I meant by "you look at the dependencies for
> SFP and ensure that those are also satisfied".
> 
> Adding that "select I2C" also solves the unmet dependencies for
> I2C_DESIGNWARE_PLATFORM.
> 
> However, even with that, we're not done with the evilness of select,
> because there's one more permitted configuration combination that
> will break.
> 
> If you build TXGBE into the kernel, that will force SFP=y, I2C=y,
> PHYLINK=y, PHYLIB=y. So far so good. However, if HWMON=m, then things
> will again break. So I would also suggest:
> 
> 	select HWMON if TXGBE=y
> 
> even though you don't require it, it solves the build fallout from
> where HWMON=m but you force SFP=y.
> 
> Maybe someone else has better ideas how to do this, but the above is
> the best I can come up with.
> 
> 
> IMHO, select is nothing but pure evil, and should be used with utmost
> care and a full understanding of its ramifications, and a realisation
> that it *totally* and *utterly* blows away any "depends on" on the
> target of the select statement.
> 
> An option that states that it depends on something else generally does
> because... oddly enough, it _depends_ on that other option. So, if
> select forces an option on without its dependencies, then it's not
> surprising that stuff fails to build.
> 
> Whenever a select statement is added, one must _always_ look at the
> target symbol and consider any "depends on" there, and how to ensure
> that those dependencies are guaranteed to always be satisfied.

Thanks for the detailed explanation. I'll check each of the required options,
and use "depends on" whenever possible.