Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Keller, Jacob E" <jacob.e.keller@intel.com>
To:     Arnd Bergmann <arnd@kernel.org>,
        "Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
        "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Jakub Kicinski" <kuba@kernel.org>
CC:     Arnd Bergmann <arnd@arndb.de>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        "Saleem, Shiraz" <shiraz.saleem@intel.com>,
        "Ertman, David M" <david.m.ertman@intel.com>,
        "intel-wired-lan@lists.osuosl.org" <intel-wired-lan@lists.osuosl.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] ethernet/intel: fix PTP_1588_CLOCK dependencies
Thread-Topic: [PATCH] ethernet/intel: fix PTP_1588_CLOCK dependencies
Thread-Index: AQHXgfqoenHufbyXQU60AsxIki/9gatVwt5Q
Date:   Mon, 26 Jul 2021 21:21:09 +0000
Message-ID: <CO1PR11MB5089F97937FF0141FD02D3B3D6E89@CO1PR11MB5089.namprd11.prod.outlook.com>
References: <20210726084540.3282344-1-arnd@kernel.org>
In-Reply-To: <20210726084540.3282344-1-arnd@kernel.org>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?B57owayahNyp0NjnrryyEW2s2EiTyiCf7CBgijfzMFp3hr6KG3qrUMHFI5Cc?=
 =?us-ascii?Q?jtnJc21Z85Nv+TPZ36EZdwtKxxupX4tBsQTunfKoK5RfmynWgYpp+Y3FL/Zz?=
 =?us-ascii?Q?EtYYdU3QPNvdCOwnSsxx/d/vVWK8+mAa5K3Th1YZ7FIcJwarAkINF19pt0Eg?=
 =?us-ascii?Q?fsZasbL/vdClCIA+BfTI3e9xYbulPVaWKc3t5rhOBfPOHJIg+iHC2MDYygLI?=
 =?us-ascii?Q?pnTOlG+G70UPWHexVoRwktXrUBvMw4sOlc0IVKZCM54NHq2DMLLZWdZ6vuIC?=
 =?us-ascii?Q?uVkOmMKQY1sIILV5aDp/Eko2780T5Cc0Wn/YOfm/x0LypxL3CQHiXATELcTZ?=
 =?us-ascii?Q?InRniGtvoTR887wGSYre6dR2bVk69rtIdVz0GjXbvumedwoKbNHiC5/aLuY1?=
 =?us-ascii?Q?o4O1STioN3c1uxFa44YXJNWjvjDSorD+jVEw1GFFU/O2rU4OUhY6Ka0N5JPE?=
 =?us-ascii?Q?ytN03cQsEabXhcVaJ4OFbhBBXU3j1wHEvD0z+562jgOyKjQznHhLoeTh7B1z?=
 =?us-ascii?Q?ZTMsMuqBhyP8KQglo7DNp5opuVU1y0sQ7fKN2tdjFSYHLZsUz84nJg0OOiLs?=
 =?us-ascii?Q?I0CMrawYCODbrinjeSy507mbC8rquOro7KdmuJWSDPQ13wJj5iHam7fqgA/s?=
 =?us-ascii?Q?fUED3n9O5XEXdP4yBS3fI/an8FHCtPmyGLSG8g1UJ2sA9A1LFNjVd5kNA37m?=
 =?us-ascii?Q?lmTvq9u3UkSxEeuNKFK9+eu6p3R+9YCLL8BXeQN6pnxcl2ZuZQCDjIbRg/7G?=
 =?us-ascii?Q?JHO6VtrNK/IP/5JLPb6FXi/VgsqUlzksvg4s2XruSsk7DO47KjEgcCcwC3Mb?=
 =?us-ascii?Q?o/UT8IGZeLxtwX97AnX9F87PpUDkpqmeZS9xNyQy0ZkXQw+yGsbuiWkMozDC?=
 =?us-ascii?Q?HS6b6cveLgN/8ODWk4HQURPndBG4kdQox85YLX5+u06Du4zzo3n0xneT8Jhz?=
 =?us-ascii?Q?+gL5bcySpn333qCEXGA2PwxO5Yk2wUaXqbURbT4MedMWBXCN5SqURUecPqTw?=
 =?us-ascii?Q?G2VTeQpt6svxgh+yFVLZTcBPdKK7SmuDufokHXjSUFND604ScxKq3M2SBYkv?=
 =?us-ascii?Q?W1TnoEMDFXNvOOHyRl1xUbtrtUzhY7A/ZlcZVK/EdS/j2mp++OF9Hq2SzwA/?=
 =?us-ascii?Q?kW7mROvoTV24VBV2ZPefYO2DTkuIeOO2XlJ+FDbBbTpnrNtdA3veaXo3vosM?=
 =?us-ascii?Q?PL/+JwvfBrq5tkllKWvuLrblAWrebCAEx6rsMwKvVDoI2k0cXbhoccoQxbBO?=
 =?us-ascii?Q?vNklNjXgqTORCnw36hYcw8vKvgL+nw0p7H8PYol+bZjxTTaEacRN5bf7ayiw?=
 =?us-ascii?Q?/JE=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 51a0df4c-5ec0-4e8b-40aa-08d9507b4965
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jul 2021 21:21:09.1291
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Ap15PLRd7tsbdpSjQ7TyUCBh+ovZ14I7mN8zY0HdkYHimaOKZR+w3Z7rzhHfQFji2D1GGkROeYcUS+dJ5WFBfD3TtXZKHunwxFMEnRX63nw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4963
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Arnd Bergmann <arnd@kernel.org>
> Sent: Monday, July 26, 2021 1:45 AM
> To: Brandeburg, Jesse <jesse.brandeburg@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; David S. Miller <davem@davemloft.net>; Jaku=
b
> Kicinski <kuba@kernel.org>; Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Arnd Bergmann <arnd@arndb.de>; Kurt Kanzenbach <kurt@linutronix.de>;
> Saleem, Shiraz <shiraz.saleem@intel.com>; Ertman, David M
> <david.m.ertman@intel.com>; intel-wired-lan@lists.osuosl.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] ethernet/intel: fix PTP_1588_CLOCK dependencies
>=20
> From: Arnd Bergmann <arnd@arndb.de>
>=20
> The 'imply' keyword does not do what most people think it does, it only
> politely asks Kconfig to turn on another symbol, but does not prevent
> it from being disabled manually or built as a loadable module when the
> user is built-in. In the ICE driver, the latter now causes a link failure=
:
>=20
> aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function
> `ice_eth_ioctl':
> ice_main.c:(.text+0x13b0): undefined reference to `ice_ptp_get_ts_config'
> ice_main.c:(.text+0x13b0): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `ice_ptp_get_ts_config'
> aarch64-linux-ld: ice_main.c:(.text+0x13bc): undefined reference to
> `ice_ptp_set_ts_config'
> ice_main.c:(.text+0x13bc): relocation truncated to fit: R_AARCH64_CALL26
> against undefined symbol `ice_ptp_set_ts_config'
> aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function
> `ice_prepare_for_reset':
> ice_main.c:(.text+0x31fc): undefined reference to `ice_ptp_release'
> ice_main.c:(.text+0x31fc): relocation truncated to fit: R_AARCH64_CALL26 =
against
> undefined symbol `ice_ptp_release'
> aarch64-linux-ld: drivers/net/ethernet/intel/ice/ice_main.o: in function
> `ice_rebuild':
>=20
> For the other Intel network drivers, there is no link error when the
> drivers are built-in and PTP is a loadable module, because
> linux/ptp_clock_kernel.h contains an IS_REACHABLE() check, but this
> just changes the compile-time failure to a runtime failure, which is
> arguably worse.
>=20
> Change all the Intel drivers to use the 'depends on PTP_1588_CLOCK ||
> !PTP_1588_CLOCK' trick to prevent the broken configuration, as we
> already do for several other drivers. To avoid circular dependencies,
> this also requires changing the IGB driver back to using the normal
> 'depends on I2C' instead of 'select I2C'.
>=20
> Fixes: 06c16d89d2cb ("ice: register 1588 PTP clock device object for E810=
 devices")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for fixing this!

It feels like Kconfig should have a simpler way to write this, and/or we sh=
ould update the doc, since I would expect this to be a common issue with op=
tional dependencies

Obviously "depends" handles this right but it forces a dependency in all ca=
ses, instead of being optional. select is used to ensure that some bit is t=
urned on if you turn on that item, and imply is supposed to be that but opt=
ional...


> ---
>  drivers/net/ethernet/intel/Kconfig | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>=20
> diff --git a/drivers/net/ethernet/intel/Kconfig
> b/drivers/net/ethernet/intel/Kconfig
> index 2aa84bd97287..ab75cde0c4ec 100644
> --- a/drivers/net/ethernet/intel/Kconfig
> +++ b/drivers/net/ethernet/intel/Kconfig
> @@ -58,8 +58,8 @@ config E1000
>  config E1000E
>  	tristate "Intel(R) PRO/1000 PCI-Express Gigabit Ethernet support"
>  	depends on PCI && (!SPARC32 || BROKEN)
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	select CRC32
> -	imply PTP_1588_CLOCK
>  	help
>  	  This driver supports the PCI-Express Intel(R) PRO/1000 gigabit
>  	  ethernet family of adapters. For PCI or PCI-X e1000 adapters,
> @@ -87,7 +87,7 @@ config E1000E_HWTS
>  config IGB
>  	tristate "Intel(R) 82575/82576 PCI-Express Gigabit Ethernet support"
>  	depends on PCI
> -	imply PTP_1588_CLOCK
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	select I2C


Commit message said you changed IGB to use depends I2C, but the content doe=
sn't...

>  	select I2C_ALGOBIT
>  	help
> @@ -159,9 +159,9 @@ config IXGB
>  config IXGBE
>  	tristate "Intel(R) 10GbE PCI Express adapters support"
>  	depends on PCI
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	select MDIO
>  	select PHYLIB
> -	imply PTP_1588_CLOCK
>  	help
>  	  This driver supports Intel(R) 10GbE PCI Express family of
>  	  adapters.  For more information on how to identify your adapter, go
> @@ -239,7 +239,7 @@ config IXGBEVF_IPSEC
>=20
>  config I40E
>  	tristate "Intel(R) Ethernet Controller XL710 Family support"
> -	imply PTP_1588_CLOCK
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	depends on PCI
>  	select AUXILIARY_BUS
>  	help
> @@ -295,11 +295,11 @@ config ICE
>  	tristate "Intel(R) Ethernet Connection E800 Series Support"
>  	default n
>  	depends on PCI_MSI
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	select AUXILIARY_BUS
>  	select DIMLIB
>  	select NET_DEVLINK
>  	select PLDMFW
> -	imply PTP_1588_CLOCK
>  	help
>  	  This driver supports Intel(R) Ethernet Connection E800 Series of
>  	  devices.  For more information on how to identify your adapter, go
> @@ -317,7 +317,7 @@ config FM10K
>  	tristate "Intel(R) FM10000 Ethernet Switch Host Interface Support"
>  	default n
>  	depends on PCI_MSI
> -	imply PTP_1588_CLOCK
> +	depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
>  	help
>  	  This driver supports Intel(R) FM10000 Ethernet Switch Host
>  	  Interface.  For more information on how to identify your adapter,
> --
> 2.29.2