diff mbox series

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

Message ID 20230524091722.522118-6-jiawenwu@trustnetic.com
State New
Headers show
Series TXGBE PHYLINK support | expand

Commit Message

Jiawen Wu May 24, 2023, 9:17 a.m. UTC 
Register SFP platform device to get modules information.

Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
---
 drivers/net/ethernet/wangxun/Kconfig          |  1 +
 .../net/ethernet/wangxun/txgbe/txgbe_phy.c    | 28 +++++++++++++++++++
 .../net/ethernet/wangxun/txgbe/txgbe_type.h   |  1 +
 3 files changed, 30 insertions(+)

Comments

kernel test robot May 26, 2023, 11:30 a.m. UTC | #1 
Hi Jiawen,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jiawen-Wu/net-txgbe-Add-software-nodes-to-support-phylink/20230524-173221
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230524091722.522118-6-jiawenwu%40trustnetic.com
patch subject: [PATCH net-next v9 5/9] net: txgbe: Add SFP module identify
config: csky-randconfig-r003-20230525 (https://download.01.org/0day-ci/archive/20230526/202305261959.mnGUW17n-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c382745a6443e8ff9b3fab9b10c90b216b2ca59b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jiawen-Wu/net-txgbe-Add-software-nodes-to-support-phylink/20230524-173221
        git checkout c382745a6443e8ff9b3fab9b10c90b216b2ca59b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross W=1 O=build_dir ARCH=csky SHELL=/bin/bash drivers/net/phy/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305261959.mnGUW17n-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/phy/sfp.c: In function 'sfp_i2c_read':
>> drivers/net/phy/sfp.c:609:23: error: implicit declaration of function 'i2c_transfer' [-Werror=implicit-function-declaration]
     609 |                 ret = i2c_transfer(sfp->i2c, msgs, ARRAY_SIZE(msgs));
         |                       ^~~~~~~~~~~~
   drivers/net/phy/sfp.c: In function 'sfp_i2c_configure':
>> drivers/net/phy/sfp.c:653:14: error: implicit declaration of function 'i2c_check_functionality' [-Werror=implicit-function-declaration]
     653 |         if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
         |              ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/phy/sfp.c: In function 'sfp_cleanup':
>> drivers/net/phy/sfp.c:2919:17: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration]
    2919 |                 i2c_put_adapter(sfp->i2c);
         |                 ^~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

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]


vim +/i2c_transfer +609 drivers/net/phy/sfp.c

73970055450eeb Russell King  2017-07-25  583  
3bb35261c74e39 Jon Nettleton 2018-02-27  584  static int sfp_i2c_read(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
3bb35261c74e39 Jon Nettleton 2018-02-27  585  			size_t len)
73970055450eeb Russell King  2017-07-25  586  {
73970055450eeb Russell King  2017-07-25  587  	struct i2c_msg msgs[2];
426c6cbc409cbd Pali Rohár    2021-01-25  588  	u8 bus_addr = a2 ? 0x51 : 0x50;
426c6cbc409cbd Pali Rohár    2021-01-25  589  	size_t block_size = sfp->i2c_block_size;
28e74a7cfd6403 Russell King  2019-06-02  590  	size_t this_len;
73970055450eeb Russell King  2017-07-25  591  	int ret;
73970055450eeb Russell King  2017-07-25  592  
73970055450eeb Russell King  2017-07-25  593  	msgs[0].addr = bus_addr;
73970055450eeb Russell King  2017-07-25  594  	msgs[0].flags = 0;
73970055450eeb Russell King  2017-07-25  595  	msgs[0].len = 1;
73970055450eeb Russell King  2017-07-25  596  	msgs[0].buf = &dev_addr;
73970055450eeb Russell King  2017-07-25  597  	msgs[1].addr = bus_addr;
73970055450eeb Russell King  2017-07-25  598  	msgs[1].flags = I2C_M_RD;
73970055450eeb Russell King  2017-07-25  599  	msgs[1].len = len;
73970055450eeb Russell King  2017-07-25  600  	msgs[1].buf = buf;
73970055450eeb Russell King  2017-07-25  601  
28e74a7cfd6403 Russell King  2019-06-02  602  	while (len) {
28e74a7cfd6403 Russell King  2019-06-02  603  		this_len = len;
0d035bed2a4a6c Russell King  2020-12-09  604  		if (this_len > block_size)
0d035bed2a4a6c Russell King  2020-12-09  605  			this_len = block_size;
28e74a7cfd6403 Russell King  2019-06-02  606  
28e74a7cfd6403 Russell King  2019-06-02  607  		msgs[1].len = this_len;
28e74a7cfd6403 Russell King  2019-06-02  608  
3bb35261c74e39 Jon Nettleton 2018-02-27 @609  		ret = i2c_transfer(sfp->i2c, msgs, ARRAY_SIZE(msgs));
73970055450eeb Russell King  2017-07-25  610  		if (ret < 0)
73970055450eeb Russell King  2017-07-25  611  			return ret;
73970055450eeb Russell King  2017-07-25  612  
28e74a7cfd6403 Russell King  2019-06-02  613  		if (ret != ARRAY_SIZE(msgs))
28e74a7cfd6403 Russell King  2019-06-02  614  			break;
28e74a7cfd6403 Russell King  2019-06-02  615  
28e74a7cfd6403 Russell King  2019-06-02  616  		msgs[1].buf += this_len;
28e74a7cfd6403 Russell King  2019-06-02  617  		dev_addr += this_len;
28e74a7cfd6403 Russell King  2019-06-02  618  		len -= this_len;
28e74a7cfd6403 Russell King  2019-06-02  619  	}
28e74a7cfd6403 Russell King  2019-06-02  620  
28e74a7cfd6403 Russell King  2019-06-02  621  	return msgs[1].buf - (u8 *)buf;
73970055450eeb Russell King  2017-07-25  622  }
73970055450eeb Russell King  2017-07-25  623  
3bb35261c74e39 Jon Nettleton 2018-02-27  624  static int sfp_i2c_write(struct sfp *sfp, bool a2, u8 dev_addr, void *buf,
73970055450eeb Russell King  2017-07-25  625  	size_t len)
73970055450eeb Russell King  2017-07-25  626  {
3bb35261c74e39 Jon Nettleton 2018-02-27  627  	struct i2c_msg msgs[1];
3bb35261c74e39 Jon Nettleton 2018-02-27  628  	u8 bus_addr = a2 ? 0x51 : 0x50;
3bb35261c74e39 Jon Nettleton 2018-02-27  629  	int ret;
3bb35261c74e39 Jon Nettleton 2018-02-27  630  
3bb35261c74e39 Jon Nettleton 2018-02-27  631  	msgs[0].addr = bus_addr;
3bb35261c74e39 Jon Nettleton 2018-02-27  632  	msgs[0].flags = 0;
3bb35261c74e39 Jon Nettleton 2018-02-27  633  	msgs[0].len = 1 + len;
3bb35261c74e39 Jon Nettleton 2018-02-27  634  	msgs[0].buf = kmalloc(1 + len, GFP_KERNEL);
3bb35261c74e39 Jon Nettleton 2018-02-27  635  	if (!msgs[0].buf)
3bb35261c74e39 Jon Nettleton 2018-02-27  636  		return -ENOMEM;
3bb35261c74e39 Jon Nettleton 2018-02-27  637  
3bb35261c74e39 Jon Nettleton 2018-02-27  638  	msgs[0].buf[0] = dev_addr;
3bb35261c74e39 Jon Nettleton 2018-02-27  639  	memcpy(&msgs[0].buf[1], buf, len);
3bb35261c74e39 Jon Nettleton 2018-02-27  640  
3bb35261c74e39 Jon Nettleton 2018-02-27  641  	ret = i2c_transfer(sfp->i2c, msgs, ARRAY_SIZE(msgs));
3bb35261c74e39 Jon Nettleton 2018-02-27  642  
3bb35261c74e39 Jon Nettleton 2018-02-27  643  	kfree(msgs[0].buf);
3bb35261c74e39 Jon Nettleton 2018-02-27  644  
3bb35261c74e39 Jon Nettleton 2018-02-27  645  	if (ret < 0)
3bb35261c74e39 Jon Nettleton 2018-02-27  646  		return ret;
3bb35261c74e39 Jon Nettleton 2018-02-27  647  
3bb35261c74e39 Jon Nettleton 2018-02-27  648  	return ret == ARRAY_SIZE(msgs) ? len : 0;
73970055450eeb Russell King  2017-07-25  649  }
73970055450eeb Russell King  2017-07-25  650  
73970055450eeb Russell King  2017-07-25  651  static int sfp_i2c_configure(struct sfp *sfp, struct i2c_adapter *i2c)
73970055450eeb Russell King  2017-07-25  652  {
73970055450eeb Russell King  2017-07-25 @653  	if (!i2c_check_functionality(i2c, I2C_FUNC_I2C))
73970055450eeb Russell King  2017-07-25  654  		return -EINVAL;
73970055450eeb Russell King  2017-07-25  655  
73970055450eeb Russell King  2017-07-25  656  	sfp->i2c = i2c;
73970055450eeb Russell King  2017-07-25  657  	sfp->read = sfp_i2c_read;
3bb35261c74e39 Jon Nettleton 2018-02-27  658  	sfp->write = sfp_i2c_write;
73970055450eeb Russell King  2017-07-25  659  
e85b1347ace677 Marek Behún   2022-09-30  660  	return 0;
e85b1347ace677 Marek Behún   2022-09-30  661  }
e85b1347ace677 Marek Behún   2022-09-30  662
Russell King (Oracle) May 26, 2023, 11:36 a.m. UTC | #2 
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.
Jiawen Wu May 29, 2023, 2:06 a.m. UTC | #3 
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?
Jiawen Wu May 30, 2023, 8:40 a.m. UTC | #4 
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?
Jiawen Wu May 31, 2023, 9:19 a.m. UTC | #5 
On Tuesday, May 30, 2023 4:41 PM, 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?

Hi Russell,

Could you please give me some suggestions?

I checked "kconfig-language" doc, the practical solution is that swap all
"select FOO" to "depends on FOO" or swap all "depends on FOO" to
"select FOO". Config PCS_XPCS has to be selected in order to load modules
properly, so how should I fix the warning?
Russell King (Oracle) May 31, 2023, 9:47 a.m. UTC | #6 
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.
Jiawen Wu May 31, 2023, 10:11 a.m. UTC | #7 
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.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/wangxun/Kconfig b/drivers/net/ethernet/wangxun/Kconfig
index ec058a72afb6..90812d76181d 100644
--- a/drivers/net/ethernet/wangxun/Kconfig
+++ b/drivers/net/ethernet/wangxun/Kconfig
@@ -44,6 +44,7 @@  config TXGBE
 	select REGMAP
 	select COMMON_CLK
 	select LIBWX
+	select SFP
 	help
 	  This driver supports Wangxun(R) 10GbE PCI Express family of
 	  adapters.
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
index 6ea33e753df4..3da5f5538f34 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_phy.c
@@ -158,6 +158,25 @@  static int txgbe_i2c_register(struct txgbe *txgbe)
 	return 0;
 }
 
+static int txgbe_sfp_register(struct txgbe *txgbe)
+{
+	struct pci_dev *pdev = txgbe->wx->pdev;
+	struct platform_device_info info = {};
+	struct platform_device *sfp_dev;
+
+	info.parent = &pdev->dev;
+	info.fwnode = software_node_fwnode(txgbe->nodes.group[SWNODE_SFP]);
+	info.name = "sfp";
+	info.id = (pdev->bus->number << 8) | pdev->devfn;
+	sfp_dev = platform_device_register_full(&info);
+	if (IS_ERR(sfp_dev))
+		return PTR_ERR(sfp_dev);
+
+	txgbe->sfp_dev = sfp_dev;
+
+	return 0;
+}
+
 int txgbe_init_phy(struct txgbe *txgbe)
 {
 	int ret;
@@ -180,8 +199,16 @@  int txgbe_init_phy(struct txgbe *txgbe)
 		goto err_unregister_clk;
 	}
 
+	ret = txgbe_sfp_register(txgbe);
+	if (ret) {
+		wx_err(txgbe->wx, "failed to register sfp\n");
+		goto err_unregister_i2c;
+	}
+
 	return 0;
 
+err_unregister_i2c:
+	platform_device_unregister(txgbe->i2c_dev);
 err_unregister_clk:
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
@@ -193,6 +220,7 @@  int txgbe_init_phy(struct txgbe *txgbe)
 
 void txgbe_remove_phy(struct txgbe *txgbe)
 {
+	platform_device_unregister(txgbe->sfp_dev);
 	platform_device_unregister(txgbe->i2c_dev);
 	clkdev_drop(txgbe->clock);
 	clk_unregister(txgbe->clk);
diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
index 55979abf01f2..fc91e0fc37a6 100644
--- a/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
+++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_type.h
@@ -149,6 +149,7 @@  struct txgbe_nodes {
 struct txgbe {
 	struct wx *wx;
 	struct txgbe_nodes nodes;
+	struct platform_device *sfp_dev;
 	struct platform_device *i2c_dev;
 	struct clk_lookup *clock;
 	struct clk *clk;