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]
Date:   Fri, 4 Jan 2019 00:00:23 +0000
From:   Steve Douthit <stephend@...icom-usa.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Dave Jones <davej@...emonkey.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH] ixgbe: fix Kconfig when driver is not a module

On 1/3/19 4:35 PM, Jeff Kirsher wrote:
> The new ability added to the driver to use mii_bus to handle MII related
> ioctls is causing compile issues when the driver is compiled into the
> kernel (i.e. not a module).
> 
> The simple solution of requiring the driver to be compiled as a module when
> MDIO_BUS is a module, causes a recursion Kconfig issue due to IPSec
> dependencies.
> 
> So created another Kconfig option for ixgbe, to enable mdio_bus support for
> DSA devices.  This solution solves the problem when the ixgbe driver is
> compiled into the kernel and MDIO_BUS is compiled as a module.  In this
> case, the IXGBE_MDIO option is disabled and the code is not compiled
> into the driver.
> 
> CC: Dave Jones <davej@...emonkey.org.uk>
> CC: Steve Douthit <stephend@...icom-usa.com>
> CC: Florian Fainelli <f.fainelli@...il.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> ---
>   drivers/net/ethernet/intel/Kconfig            | 11 ++++++++++-
>   drivers/net/ethernet/intel/ixgbe/ixgbe.h      |  4 ++++
>   drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 12 ++++++++++--
>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c  |  4 ++++
>   drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h  |  3 ++-
>   5 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
> index 31fb76ee9d82..35317ecdd0c3 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -159,7 +159,6 @@ config IXGBE
>   	tristate "Intel(R) 10GbE PCI Express adapters support"
>   	depends on PCI
>   	select MDIO
> -	select MDIO_DEVICE
>   	imply PTP_1588_CLOCK
>   	---help---
>   	  This driver supports Intel(R) 10GbE PCI Express family of
> @@ -210,6 +209,16 @@ config IXGBE_IPSEC
>   	---help---
>   	  Enable support for IPSec offload in ixgbe.ko
>   
> +config IXGBE_MDIO
> +	bool "MDIO Support for DSA devices"
> +	default n
> +	depends on IXGBE && MDIO_BUS && !(IXGBE=y && MDIO_BUS=m)
> +	---help---
> +	  Say Y here if you want MDIO_BUS support for DSA devices in the
> +	  driver.

This is also useful for accessing the entire clause 22/45 address space,
which may be worth a mention in the help text.

> +	  If unsure, say N.
> +
>   config IXGBEVF
>   	tristate "Intel(R) 10GbE PCI Express Virtual Function Ethernet support"
>   	depends on PCI_MSI
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index 08d85e336bd4..9d7496508ee0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -12,7 +12,9 @@
>   #include <linux/aer.h>
>   #include <linux/if_vlan.h>
>   #include <linux/jiffies.h>
> +#ifdef CONFIG_IXGBE_MDIO
>   #include <linux/phy.h>
> +#endif
>   
>   #include <linux/timecounter.h>
>   #include <linux/net_tstamp.h>
> @@ -562,7 +564,9 @@ struct ixgbe_adapter {
>   	struct net_device *netdev;
>   	struct bpf_prog *xdp_prog;
>   	struct pci_dev *pdev;
> +#ifdef CONFIG_IXGBE_MDIO
>   	struct mii_bus *mii_bus;
> +#endif
>   
>   	unsigned long state;
>   
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index d2ce7f0bc32d..afa0337f7ba0 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -39,7 +39,9 @@
>   #include "ixgbe.h"
>   #include "ixgbe_common.h"
>   #include "ixgbe_dcb_82599.h"
> +#ifdef CONFIG_IXGBE_MDIO
>   #include "ixgbe_phy.h"
> +#endif
>   #include "ixgbe_sriov.h"
>   #include "ixgbe_model.h"
>   #include "ixgbe_txrx_common.h"
> @@ -8791,6 +8793,7 @@ ixgbe_mdio_read(struct net_device *netdev, int prtad, int devad, u16 addr)
>   	u16 value;
>   	int rc;
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   	if (adapter->mii_bus) {
>   		int regnum = addr;
>   
> @@ -8799,7 +8802,7 @@ ixgbe_mdio_read(struct net_device *netdev, int prtad, int devad, u16 addr)
>   
>   		return mdiobus_read(adapter->mii_bus, prtad, regnum);
>   	}
> -
> +#endif
>   	if (prtad != hw->phy.mdio.prtad)
>   		return -EINVAL;
>   	rc = hw->phy.ops.read_reg(hw, addr, devad, &value);
> @@ -8814,6 +8817,7 @@ static int ixgbe_mdio_write(struct net_device *netdev, int prtad, int devad,
>   	struct ixgbe_adapter *adapter = netdev_priv(netdev);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   	if (adapter->mii_bus) {
>   		int regnum = addr;
>   
> @@ -8822,7 +8826,7 @@ static int ixgbe_mdio_write(struct net_device *netdev, int prtad, int devad,
>   
>   		return mdiobus_write(adapter->mii_bus, prtad, regnum, value);
>   	}
> -
> +#endif
>   	if (prtad != hw->phy.mdio.prtad)
>   		return -EINVAL;
>   	return hw->phy.ops.write_reg(hw, addr, devad, value);
> @@ -11141,7 +11145,9 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   			IXGBE_LINK_SPEED_10GB_FULL | IXGBE_LINK_SPEED_1GB_FULL,
>   			true);
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   	ixgbe_mii_bus_init(hw);
> +#endif
>   
>   	return 0;
>   
> @@ -11193,8 +11199,10 @@ static void ixgbe_remove(struct pci_dev *pdev)
>   	set_bit(__IXGBE_REMOVING, &adapter->state);
>   	cancel_work_sync(&adapter->service_task);
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   	if (adapter->mii_bus)
>   		mdiobus_unregister(adapter->mii_bus);
> +#endif
>   
>   #ifdef CONFIG_IXGBE_DCA
>   	if (adapter->flags & IXGBE_FLAG_DCA_ENABLED) {
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> index cc4907f9ff02..05da21920863 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> @@ -3,7 +3,9 @@
>   
>   #include <linux/pci.h>
>   #include <linux/delay.h>
> +#ifdef CONFIG_IXGBE_MDIO
>   #include <linux/iopoll.h>
> +#endif
>   #include <linux/sched.h>
>   
>   #include "ixgbe.h"
> @@ -659,6 +661,7 @@ s32 ixgbe_write_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
>   	return status;
>   }
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   #define IXGBE_HW_READ_REG(addr) IXGBE_READ_REG(hw, addr)
>   
>   /**
> @@ -956,6 +959,7 @@ s32 ixgbe_mii_bus_init(struct ixgbe_hw *hw)
>   	adapter->mii_bus = NULL;
>   	return -ENODEV;
>   }
> +#endif
>   
>   /**
>    *  ixgbe_setup_phy_link_generic - Set and restart autoneg
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> index 214b01085718..88b851178d7e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.h
> @@ -120,8 +120,9 @@
>   /* SFP+ SFF-8472 Compliance code */
>   #define IXGBE_SFF_SFF_8472_UNSUP      0x00
>   
> +#ifdef CONFIG_IXGBE_MDIO
>   s32 ixgbe_mii_bus_init(struct ixgbe_hw *hw);
> -
> +#endif
>   s32 ixgbe_identify_phy_generic(struct ixgbe_hw *hw);
>   s32 ixgbe_reset_phy_generic(struct ixgbe_hw *hw);
>   s32 ixgbe_read_phy_reg_generic(struct ixgbe_hw *hw, u32 reg_addr,
> 

Just tested this against a DSA dev in hardware and it works, but I
still think the simpler solution would be to just select PHYLIB:

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 31fb76ee9d82..a1246e89aad4 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -159,7 +159,7 @@ config IXGBE
  	tristate "Intel(R) 10GbE PCI Express adapters support"
  	depends on PCI
  	select MDIO
-	select MDIO_DEVICE
+	select PHYLIB
  	imply PTP_1588_CLOCK
  	---help---
  	  This driver supports Intel(R) 10GbE PCI Express family of

That seems to be the pattern for most of the other ethernet adapters:
$ grep -r 'select PHYLIB' drivers/net/ethernet/ | wc -l
71

vs checking MDIO_BUS (not counting this patch):
$ grep -r 'depends on.*MDIO_BUS' drivers/net/ethernet/ | wc -l
0

AFAICT in the Kconfig language a || between two dependency expressions
returns the max() of the two expr where (n, m, y) = (0, 1, 2).  So in
the case of multiple drivers selecting the same symbol if any are
built-in then the selected symbol must also be built-in.  This has held
true for the little bit of poking around I've done via menuconfig, but
I'm definitely not a Kconfig expert.

In any case you can have my:

Tested-by: Stephen Douthit <stephend@...icom-usa.com>

Powered by blists - more mailing lists