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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZHcXvFvR3H8Vmyok@shell.armlinux.org.uk>
Date: Wed, 31 May 2023 10:47:40 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: 'kernel test robot' <lkp@...el.com>, netdev@...r.kernel.org,
	jarkko.nikula@...ux.intel.com, andriy.shevchenko@...ux.intel.com,
	mika.westerberg@...ux.intel.com, jsd@...ihalf.com,
	Jose.Abreu@...opsys.com, andrew@...n.ch, hkallweit1@...il.com,
	oe-kbuild-all@...ts.linux.dev, linux-i2c@...r.kernel.org,
	linux-gpio@...r.kernel.org, mengyuanlou@...-swift.com,
	'Piotr Raczynski' <piotr.raczynski@...el.com>
Subject: Re: [PATCH net-next v9 5/9] net: txgbe: Add SFP module identify

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.

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