| Message ID | 20230524091722.522118-6-jiawenwu@trustnetic.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | TXGBE PHYLINK support | expand |
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.
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?
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?
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.
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 --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;