[PATCH net-next 0/3] mlxsw: Add fw_version_check devlink parameter

[PATCH net-next 0/3] mlxsw: Add fw_version_check devlink parameter

[Ido Schimmel]

Shalom says:

Currently, drivers do not have the ability to skip the firmware version
check during their initialization flow. Hence, drivers will always flash
a compatible version. This prevents drivers from running the device with
a different firmware version for testing and/or debugging purposes. For
example, testing a firmware bug fix.

For these situations, the new devlink generic parameter,
fw_version_check, gives the ability to skip the version check and allows
drivers to run with a different firmware version than what is required
by the driver.

Patch #1 adds the new parameter to devlink. The other two patches, #2
and #3, add support for this parameter in the mlxsw driver.

Shalom Toledo (3):
  devlink: Add fw_version_check generic parameter
  mlxsw: core: Reset firmware after flash during driver initialization
  mlxsw: spectrum: Skip firmware version check based on devlink
    parameter

 drivers/net/ethernet/mellanox/mlxsw/core.c    | 45 +++++++++++++++++--
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +
 drivers/net/ethernet/mellanox/mlxsw/pci.c     | 11 +----
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 45 +++++++++++++++++++
 include/net/devlink.h                         |  4 ++
 net/core/devlink.c                            |  5 +++
 6 files changed, 98 insertions(+), 14 deletions(-)

-- 
2.19.1

[PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Many drivers checking the device's firmware version during the
initialization flow and flashing a compatible version if the current
version is not.

fw_version_check gives the ability to skip this check which allows to run
the device with a different firmware version than required by the driver
for testing and/or debugging purposes.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 include/net/devlink.h | 4 ++++
 net/core/devlink.c    | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index 45db0c79462d..d47ea9d38252 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -365,6 +365,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_IGNORE_ARI,
 	DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX,
 	DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN,
+	DEVLINK_PARAM_GENERIC_ID_FW_VERSION_CHECK,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -392,6 +393,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_NAME "msix_vec_per_pf_min"
 #define DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_TYPE DEVLINK_PARAM_TYPE_U32
 
+#define DEVLINK_PARAM_GENERIC_FW_VERSION_CHECK_NAME "fw_version_check"
+#define DEVLINK_PARAM_GENERIC_FW_VERSION_CHECK_TYPE DEVLINK_PARAM_TYPE_BOOL
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 3a4b29a13d31..1a09ad057851 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2692,6 +2692,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_NAME,
 		.type = DEVLINK_PARAM_GENERIC_MSIX_VEC_PER_PF_MIN_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_FW_VERSION_CHECK,
+		.name = DEVLINK_PARAM_GENERIC_FW_VERSION_CHECK_NAME,
+		.type = DEVLINK_PARAM_GENERIC_FW_VERSION_CHECK_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.19.1

[PATCH net-next 2/3] mlxsw: core: Reset firmware after flash during driver initialization

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

After flashing new firmware during the driver initialization flow (reload
or not), the driver should do a firmware reset when it gets -EAGAIN in
order to load the new one.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 32 +++++++++++++++++++---
 drivers/net/ethernet/mellanox/mlxsw/pci.c  | 11 +-------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 30f751e69698..b2e1b83525db 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -965,10 +965,11 @@ static const struct devlink_ops mlxsw_devlink_ops = {
 	.sb_occ_tc_port_bind_get	= mlxsw_devlink_sb_occ_tc_port_bind_get,
 };
 
-int mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
-				   const struct mlxsw_bus *mlxsw_bus,
-				   void *bus_priv, bool reload,
-				   struct devlink *devlink)
+static int
+__mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
+				 const struct mlxsw_bus *mlxsw_bus,
+				 void *bus_priv, bool reload,
+				 struct devlink *devlink)
 {
 	const char *device_kind = mlxsw_bus_info->device_kind;
 	struct mlxsw_core *mlxsw_core;
@@ -1076,6 +1077,29 @@ int mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 err_devlink_alloc:
 	return err;
 }
+
+int mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
+				   const struct mlxsw_bus *mlxsw_bus,
+				   void *bus_priv, bool reload,
+				   struct devlink *devlink)
+{
+	bool called_again = false;
+	int err;
+
+again:
+	err = __mlxsw_core_bus_device_register(mlxsw_bus_info, mlxsw_bus,
+					       bus_priv, reload, devlink);
+	/* -EAGAIN is returned in case the FW was updated. FW needs
+	 * a reset, so lets try to call __mlxsw_core_bus_device_register()
+	 * again.
+	 */
+	if (err == -EAGAIN && !called_again) {
+		called_again = true;
+		goto again;
+	}
+
+	return err;
+}
 EXPORT_SYMBOL(mlxsw_core_bus_device_register);
 
 void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
diff --git a/drivers/net/ethernet/mellanox/mlxsw/pci.c b/drivers/net/ethernet/mellanox/mlxsw/pci.c
index 5890fdfd62c3..66b8098c6fd2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/pci.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/pci.c
@@ -1720,7 +1720,6 @@ static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const char *driver_name = pdev->driver->name;
 	struct mlxsw_pci *mlxsw_pci;
-	bool called_again = false;
 	int err;
 
 	mlxsw_pci = kzalloc(sizeof(*mlxsw_pci), GFP_KERNEL);
@@ -1777,18 +1776,10 @@ static int mlxsw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mlxsw_pci->bus_info.dev = &pdev->dev;
 	mlxsw_pci->id = id;
 
-again:
 	err = mlxsw_core_bus_device_register(&mlxsw_pci->bus_info,
 					     &mlxsw_pci_bus, mlxsw_pci, false,
 					     NULL);
-	/* -EAGAIN is returned in case the FW was updated. FW needs
-	 * a reset, so lets try to call mlxsw_core_bus_device_register()
-	 * again.
-	 */
-	if (err == -EAGAIN && !called_again) {
-		called_again = true;
-		goto again;
-	} else if (err) {
+	if (err) {
 		dev_err(&pdev->dev, "cannot register bus device\n");
 		goto err_bus_device_register;
 	}
-- 
2.19.1

[PATCH net-next 3/3] mlxsw: spectrum: Skip firmware version check based on devlink parameter

[Ido Schimmel]

From: Shalom Toledo <shalomt@mellanox.com>

Based on fw_version_check devlink parameter, skip firmware version check in
order to run the device with different firmware version than required by
the driver for testing and/or debugging purposes.

Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c    | 13 ++++++
 drivers/net/ethernet/mellanox/mlxsw/core.h    |  2 +
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 45 +++++++++++++++++++
 3 files changed, 60 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index b2e1b83525db..281aeb1c2386 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1036,6 +1036,12 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 			goto err_devlink_register;
 	}
 
+	if (mlxsw_driver->params_register && !reload) {
+		err = mlxsw_driver->params_register(mlxsw_core);
+		if (err)
+			goto err_register_params;
+	}
+
 	err = mlxsw_hwmon_init(mlxsw_core, mlxsw_bus_info, &mlxsw_core->hwmon);
 	if (err)
 		goto err_hwmon_init;
@@ -1058,6 +1064,9 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 err_thermal_init:
 	mlxsw_hwmon_fini(mlxsw_core->hwmon);
 err_hwmon_init:
+	if (mlxsw_driver->params_unregister && !reload)
+		mlxsw_driver->params_unregister(mlxsw_core);
+err_register_params:
 	if (!reload)
 		devlink_unregister(devlink);
 err_devlink_register:
@@ -1121,6 +1130,8 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 		mlxsw_core->driver->fini(mlxsw_core);
 	mlxsw_thermal_fini(mlxsw_core->thermal);
 	mlxsw_hwmon_fini(mlxsw_core->hwmon);
+	if (mlxsw_core->driver->params_unregister && !reload)
+		mlxsw_core->driver->params_unregister(mlxsw_core);
 	if (!reload)
 		devlink_unregister(devlink);
 	mlxsw_emad_fini(mlxsw_core);
@@ -1133,6 +1144,8 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 	return;
 
 reload_fail_deinit:
+	if (mlxsw_core->driver->params_unregister)
+		mlxsw_core->driver->params_unregister(mlxsw_core);
 	devlink_unregister(devlink);
 	devlink_resources_unregister(devlink, NULL);
 	devlink_free(devlink);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index c35be477856f..d811be8989b0 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -282,6 +282,8 @@ struct mlxsw_driver {
 			     const struct mlxsw_config_profile *profile,
 			     u64 *p_single_size, u64 *p_double_size,
 			     u64 *p_linear_size);
+	int (*params_register)(struct mlxsw_core *mlxsw_core);
+	void (*params_unregister)(struct mlxsw_core *mlxsw_core);
 	u8 txhdr_len;
 	const struct mlxsw_config_profile *profile;
 	bool res_query_enabled;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 9bec940330a4..ab1ca8c1d6df 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -318,6 +318,7 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
 	const struct mlxsw_fw_rev *rev = &mlxsw_sp->bus_info->fw_rev;
 	const struct mlxsw_fw_rev *req_rev = mlxsw_sp->req_rev;
 	const char *fw_filename = mlxsw_sp->fw_filename;
+	union devlink_param_value value;
 	const struct firmware *firmware;
 	int err;
 
@@ -325,6 +326,15 @@ static int mlxsw_sp_fw_rev_validate(struct mlxsw_sp *mlxsw_sp)
 	if (!req_rev || !fw_filename)
 		return 0;
 
+	/* Don't check if devlink fw_version_check param is false */
+	err = devlink_param_driverinit_value_get(priv_to_devlink(mlxsw_sp->core),
+						 DEVLINK_PARAM_GENERIC_ID_FW_VERSION_CHECK,
+						 &value);
+	if (err)
+		return err;
+	if (!value.vbool)
+		return 0;
+
 	/* Validate driver & FW are compatible */
 	if (rev->major != req_rev->major) {
 		WARN(1, "Mismatch in major FW version [%d:%d] is never expected; Please contact support\n",
@@ -4171,6 +4181,37 @@ static int mlxsw_sp_kvd_sizes_get(struct mlxsw_core *mlxsw_core,
 	return 0;
 }
 
+static const struct devlink_param mlxsw_sp_devlink_params[] = {
+	DEVLINK_PARAM_GENERIC(FW_VERSION_CHECK,
+			      BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
+			      NULL, NULL, NULL),
+};
+
+static int mlxsw_sp_params_register(struct mlxsw_core *mlxsw_core)
+{
+	struct devlink *devlink = priv_to_devlink(mlxsw_core);
+	union devlink_param_value value;
+	int err;
+
+	err = devlink_params_register(devlink, mlxsw_sp_devlink_params,
+				      ARRAY_SIZE(mlxsw_sp_devlink_params));
+	if (err)
+		return err;
+
+	value.vbool = true;
+	devlink_param_driverinit_value_set(devlink,
+					   DEVLINK_PARAM_GENERIC_ID_FW_VERSION_CHECK,
+					   value);
+	return 0;
+}
+
+static void mlxsw_sp_params_unregister(struct mlxsw_core *mlxsw_core)
+{
+	devlink_params_unregister(priv_to_devlink(mlxsw_core),
+				  mlxsw_sp_devlink_params,
+				  ARRAY_SIZE(mlxsw_sp_devlink_params));
+}
+
 static struct mlxsw_driver mlxsw_sp1_driver = {
 	.kind				= mlxsw_sp1_driver_name,
 	.priv_size			= sizeof(struct mlxsw_sp),
@@ -4192,6 +4233,8 @@ static struct mlxsw_driver mlxsw_sp1_driver = {
 	.txhdr_construct		= mlxsw_sp_txhdr_construct,
 	.resources_register		= mlxsw_sp1_resources_register,
 	.kvd_sizes_get			= mlxsw_sp_kvd_sizes_get,
+	.params_register		= mlxsw_sp_params_register,
+	.params_unregister		= mlxsw_sp_params_unregister,
 	.txhdr_len			= MLXSW_TXHDR_LEN,
 	.profile			= &mlxsw_sp1_config_profile,
 	.res_query_enabled		= true,
@@ -4217,6 +4260,8 @@ static struct mlxsw_driver mlxsw_sp2_driver = {
 	.sb_occ_tc_port_bind_get	= mlxsw_sp_sb_occ_tc_port_bind_get,
 	.txhdr_construct		= mlxsw_sp_txhdr_construct,
 	.resources_register		= mlxsw_sp2_resources_register,
+	.params_register		= mlxsw_sp_params_register,
+	.params_unregister		= mlxsw_sp_params_unregister,
 	.txhdr_len			= MLXSW_TXHDR_LEN,
 	.profile			= &mlxsw_sp2_config_profile,
 	.res_query_enabled		= true,
-- 
2.19.1

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Jakub Kicinski]

On Tue, 6 Nov 2018 20:05:00 +0000, Ido Schimmel wrote:
> From: Shalom Toledo <shalomt@mellanox.com>
> 
> Many drivers checking the device's firmware version during the
> initialization flow and flashing a compatible version if the current
> version is not.
> 
> fw_version_check gives the ability to skip this check which allows to run
> the device with a different firmware version than required by the driver
> for testing and/or debugging purposes.
> 
> Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>

The documentation is missing, so it's hard to comment on the definition
of the parameter...  We have a FW loading policy for NFP, too, so it'd
be good to see if we can find a common ground.

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Ido Schimmel]

On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:
> On Tue, 6 Nov 2018 20:05:00 +0000, Ido Schimmel wrote:
> > From: Shalom Toledo <shalomt@mellanox.com>
> > 
> > Many drivers checking the device's firmware version during the
> > initialization flow and flashing a compatible version if the current
> > version is not.
> > 
> > fw_version_check gives the ability to skip this check which allows to run
> > the device with a different firmware version than required by the driver
> > for testing and/or debugging purposes.
> > 
> > Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> 
> The documentation is missing, so it's hard to comment on the definition
> of the parameter...  

I assume you mean Documentation/networking/devlink-params.txt ?

> We have a FW loading policy for NFP, too, so it'd be good to see if we
> can find a common ground.

If the parameter is set, then device runs with whatever firmware version
was last flashed (via ethtool, for example). Otherwise, the driver will
flash a version according to its policy. In mlxsw, it is a specific
version.

Will that work for you?

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Jakub Kicinski]

On Tue, 6 Nov 2018 22:37:51 +0200, Ido Schimmel wrote:
> On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:
> > On Tue, 6 Nov 2018 20:05:00 +0000, Ido Schimmel wrote:  
> > > From: Shalom Toledo <shalomt@mellanox.com>
> > > 
> > > Many drivers checking the device's firmware version during the
> > > initialization flow and flashing a compatible version if the current
> > > version is not.
> > > 
> > > fw_version_check gives the ability to skip this check which allows to run
> > > the device with a different firmware version than required by the driver
> > > for testing and/or debugging purposes.
> > > 
> > > Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> > > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > > Signed-off-by: Ido Schimmel <idosch@mellanox.com>  
> > 
> > The documentation is missing, so it's hard to comment on the definition
> > of the parameter...    
> 
> I assume you mean Documentation/networking/devlink-params.txt ?

Yes

> > We have a FW loading policy for NFP, too, so it'd be good to see if we
> > can find a common ground.  
> 
> If the parameter is set, then device runs with whatever firmware version
> was last flashed (via ethtool, for example). Otherwise, the driver will
> flash a version according to its policy. In mlxsw, it is a specific
> version.
> 
> Will that work for you?

Our FW is always backward compatible so there is no need to downgrade.

What we have is this more along these lines: there are two images one
on disk and second in the flash.  The FW loading policy can decide
which of those should be preferred, or should the versions be compared
and the newer one win (default).  But we don't flash the newer FW, just
potentially load it from disk today.

I'm not sure whether 'fw_version_check' describes the general behaviour
of not updating the FW in flash.  The policy of updating the FW in the
flash if the one on disk is newer seems to be something we could adopt
as well.  Can we come up with a more general parameter which could
select FW loading policy that'd for both cases?

Would values like these make any sense to you?
 - driver preferred (your default behaviour, we don't support since
   driver doesn't care);
 - newest (our default, device compares images and picks newer);
 - always disk (always run with what's on the disk, regardless of
   versions);
 - always flash (always run with what's already in flash, don't look at
   disk);

Separate bool parameter 'fw_flash_auto_update' would decide whether the
selected FW should be flashed to the device (always true for you AFAIU).

Let me know if that makes sense, it would be nice if we could converge
on a common solution, or at least name our parameters sufficiently
distinctly to avoid confusion :)

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Ido Schimmel]

On Tue, Nov 06, 2018 at 02:47:13PM -0800, Jakub Kicinski wrote:
> On Tue, 6 Nov 2018 22:37:51 +0200, Ido Schimmel wrote:
> > On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:
> > > On Tue, 6 Nov 2018 20:05:00 +0000, Ido Schimmel wrote:  
> > > > From: Shalom Toledo <shalomt@mellanox.com>
> > > > 
> > > > Many drivers checking the device's firmware version during the
> > > > initialization flow and flashing a compatible version if the current
> > > > version is not.
> > > > 
> > > > fw_version_check gives the ability to skip this check which allows to run
> > > > the device with a different firmware version than required by the driver
> > > > for testing and/or debugging purposes.
> > > > 
> > > > Signed-off-by: Shalom Toledo <shalomt@mellanox.com>
> > > > Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> > > > Signed-off-by: Ido Schimmel <idosch@mellanox.com>  
> > > 
> > > The documentation is missing, so it's hard to comment on the definition
> > > of the parameter...    
> > 
> > I assume you mean Documentation/networking/devlink-params.txt ?
> 
> Yes
> 
> > > We have a FW loading policy for NFP, too, so it'd be good to see if we
> > > can find a common ground.  
> > 
> > If the parameter is set, then device runs with whatever firmware version
> > was last flashed (via ethtool, for example). Otherwise, the driver will
> > flash a version according to its policy. In mlxsw, it is a specific
> > version.
> > 
> > Will that work for you?
> 
> Our FW is always backward compatible so there is no need to downgrade.
> 
> What we have is this more along these lines: there are two images one
> on disk and second in the flash.  The FW loading policy can decide
> which of those should be preferred, or should the versions be compared
> and the newer one win (default).  But we don't flash the newer FW, just
> potentially load it from disk today.

Not sure I understand. You have a currently flashed firmware and another
firmware image on disk. You potentially load the firmware from the disk,
but never flash it? If so, why load it?

> I'm not sure whether 'fw_version_check' describes the general behaviour
> of not updating the FW in flash.  The policy of updating the FW in the
> flash if the one on disk is newer seems to be something we could adopt
> as well.  Can we come up with a more general parameter which could
> select FW loading policy that'd for both cases?
> 
> Would values like these make any sense to you?
>  - driver preferred (your default behaviour, we don't support since
>    driver doesn't care);
>  - newest (our default, device compares images and picks newer);
>  - always disk (always run with what's on the disk, regardless of
>    versions);
>  - always flash (always run with what's already in flash, don't look at
>    disk);
> 
> Separate bool parameter 'fw_flash_auto_update' would decide whether the
> selected FW should be flashed to the device (always true for you AFAIU).
> 
> Let me know if that makes sense, it would be nice if we could converge
> on a common solution, or at least name our parameters sufficiently
> distinctly to avoid confusion :)

I think that the above scheme is a bit too complicated and I'm not sure
this is warranted. I'll try to better explain the motivation for this
parameter and where we are coming from.

We want to keep things as simple as possible. This means we don't want
users to fiddle with devlink parameter unless they have to. Things
should just work. This parameter should only be used in exceptional
cases.

For example, when user reports a problem with current firmware version
enforced by the driver. Assuming we have a new firmware version with a
fix, we would like the user to try it and confirm bug was fixed.
Ideally, the user would do something like this:

1. Flash new firmware via ethtool
2. Perform a reset via devlink to have changes take effect

Problem is that after the reset the driver's init sequence will run and
overwrite the new firmware version with the one specified in its source
as a compatible version. The driver needs to enforce a specific version
because newer versions are not necessarily backward compatible.

Therefore, we added this new parameter that gives the user the ability
to explicitly run with a different version than what was specified as
compatible. New sequence is therefore:

1. Flash new firmware via ethtool
2. Toggle devlink parameter
3. Perform a reset via devlink to have changes take effect

Firmware loading policy is basically always go with what the driver is
enforcing (it knows best), unless user specified he/she knows better.

I think this is both generic and simple, but I possibly didn't
understand the full scope of your use cases.

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Jakub Kicinski]

On Wed, 7 Nov 2018 12:11:32 +0200, Ido Schimmel wrote:
> On Tue, Nov 06, 2018 at 02:47:13PM -0800, Jakub Kicinski wrote:
> > On Tue, 6 Nov 2018 22:37:51 +0200, Ido Schimmel wrote:  
> > > On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:  
> > > > We have a FW loading policy for NFP, too, so it'd be good to see if we
> > > > can find a common ground.    
> > > 
> > > If the parameter is set, then device runs with whatever firmware version
> > > was last flashed (via ethtool, for example). Otherwise, the driver will
> > > flash a version according to its policy. In mlxsw, it is a specific
> > > version.
> > > 
> > > Will that work for you?  
> > 
> > Our FW is always backward compatible so there is no need to downgrade.
> > 
> > What we have is this more along these lines: there are two images one
> > on disk and second in the flash.  The FW loading policy can decide
> > which of those should be preferred, or should the versions be compared
> > and the newer one win (default).  But we don't flash the newer FW, just
> > potentially load it from disk today.  
> 
> Not sure I understand. You have a currently flashed firmware and another
> firmware image on disk. 

Correct.

> You potentially load the firmware from the disk, but never flash it?

You can flash it if you want, but default is to load the
linux-firmware/disk one when system boots.  

Flashing is useful for example if you have some super special FW that
you prefer over whatever comes from linux-firmware updates, or (most
commonly) if distributing FWs is hard in your provisioning system (ugh).

> If so, why load it?

We need to load some FW..

> > I'm not sure whether 'fw_version_check' describes the general behaviour
> > of not updating the FW in flash.  The policy of updating the FW in the
> > flash if the one on disk is newer seems to be something we could adopt
> > as well.  Can we come up with a more general parameter which could
> > select FW loading policy that'd for both cases?
> > 
> > Would values like these make any sense to you?
> >  - driver preferred (your default behaviour, we don't support since
> >    driver doesn't care);
> >  - newest (our default, device compares images and picks newer);
> >  - always disk (always run with what's on the disk, regardless of
> >    versions);
> >  - always flash (always run with what's already in flash, don't look at
> >    disk);
> > 
> > Separate bool parameter 'fw_flash_auto_update' would decide whether the
> > selected FW should be flashed to the device (always true for you AFAIU).
> > 
> > Let me know if that makes sense, it would be nice if we could converge
> > on a common solution, or at least name our parameters sufficiently
> > distinctly to avoid confusion :)  
> 
> I think that the above scheme is a bit too complicated and I'm not sure
> this is warranted. I'll try to better explain the motivation for this
> parameter and where we are coming from.

Certainly, let me know if what I wrote above helps to understand the
motivation.

> We want to keep things as simple as possible. This means we don't want
> users to fiddle with devlink parameter unless they have to. Things
> should just work.

100% agree, you can choose the default for the parameter to be whatever
you want, nobody will have to touch it in normal operation.

I'm just proposing widening the values of the parameter so it works for
others (given you propose it as generic).

> This parameter should only be used in exceptional cases.
> 
> For example, when user reports a problem with current firmware version
> enforced by the driver. Assuming we have a new firmware version with a
> fix, we would like the user to try it and confirm bug was fixed.
> Ideally, the user would do something like this:
> 
> 1. Flash new firmware via ethtool
> 2. Perform a reset via devlink to have changes take effect
> 
> Problem is that after the reset the driver's init sequence will run and
> overwrite the new firmware version with the one specified in its source
> as a compatible version. The driver needs to enforce a specific version
> because newer versions are not necessarily backward compatible.
> 
> Therefore, we added this new parameter that gives the user the ability
> to explicitly run with a different version than what was specified as
> compatible. New sequence is therefore:
> 
> 1. Flash new firmware via ethtool
> 2. Toggle devlink parameter
> 3. Perform a reset via devlink to have changes take effect
> 
> Firmware loading policy is basically always go with what the driver is
> enforcing (it knows best), unless user specified he/she knows better.

Thanks, got it.

> I think this is both generic and simple, but I possibly didn't
> understand the full scope of your use cases.

I agree, it is simple, but the semantics of the parameter you're adding
unnecessarily involve firmware flashing, which is mlxsw quirk.  My
impression (mostly from my WiFi driver days) is that the FW is either in
flash or in /lib/firmware, but rarely /lib/firmware autofeeds the flash.

The commit message says "Many drivers checking the device's firmware
version during the initialization flow and flashing a compatible
version if the current version is not."  Do Ethernet drivers do that 
or some other drivers?

To reiterate I think the definition of the flag unnecessarily involves
flashing.  It may make sense to mlxsw users, but I'd postulate it won't
to almost everyone else :)

Therefore AFAICS we could make one of two improvements:
 - make this a full-fledged flash update policy with clear operational
   semantics which others can reuse; or 
 - remove the flashing part and leave it unmentioned - definition of the
   parameter becomes in a nutshell "ignore all FW version
   incompatibilities"; the limitation of mlxsw having to flash a FW
   image to load it is then an implementation detail.

Re: [PATCH net-next 1/3] devlink: Add fw_version_check generic parameter

[Ido Schimmel]

On Wed, Nov 07, 2018 at 11:05:18AM -0800, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 12:11:32 +0200, Ido Schimmel wrote:
> > On Tue, Nov 06, 2018 at 02:47:13PM -0800, Jakub Kicinski wrote:
> > > On Tue, 6 Nov 2018 22:37:51 +0200, Ido Schimmel wrote:  
> > > > On Tue, Nov 06, 2018 at 12:19:13PM -0800, Jakub Kicinski wrote:  
> > > > > We have a FW loading policy for NFP, too, so it'd be good to see if we
> > > > > can find a common ground.    
> > > > 
> > > > If the parameter is set, then device runs with whatever firmware version
> > > > was last flashed (via ethtool, for example). Otherwise, the driver will
> > > > flash a version according to its policy. In mlxsw, it is a specific
> > > > version.
> > > > 
> > > > Will that work for you?  
> > > 
> > > Our FW is always backward compatible so there is no need to downgrade.
> > > 
> > > What we have is this more along these lines: there are two images one
> > > on disk and second in the flash.  The FW loading policy can decide
> > > which of those should be preferred, or should the versions be compared
> > > and the newer one win (default).  But we don't flash the newer FW, just
> > > potentially load it from disk today.  
> > 
> > Not sure I understand. You have a currently flashed firmware and another
> > firmware image on disk. 
> 
> Correct.
> 
> > You potentially load the firmware from the disk, but never flash it?
> 
> You can flash it if you want, but default is to load the
> linux-firmware/disk one when system boots.  
> 
> Flashing is useful for example if you have some super special FW that
> you prefer over whatever comes from linux-firmware updates, or (most
> commonly) if distributing FWs is hard in your provisioning system (ugh).
> 
> > If so, why load it?
> 
> We need to load some FW..

OK. Got it now. mlxsw must flash a firmware in order to load it, but in
your case it's not necessary.

> > > I'm not sure whether 'fw_version_check' describes the general behaviour
> > > of not updating the FW in flash.  The policy of updating the FW in the
> > > flash if the one on disk is newer seems to be something we could adopt
> > > as well.  Can we come up with a more general parameter which could
> > > select FW loading policy that'd for both cases?
> > > 
> > > Would values like these make any sense to you?
> > >  - driver preferred (your default behaviour, we don't support since
> > >    driver doesn't care);
> > >  - newest (our default, device compares images and picks newer);
> > >  - always disk (always run with what's on the disk, regardless of
> > >    versions);
> > >  - always flash (always run with what's already in flash, don't look at
> > >    disk);
> > > 
> > > Separate bool parameter 'fw_flash_auto_update' would decide whether the
> > > selected FW should be flashed to the device (always true for you AFAIU).
> > > 
> > > Let me know if that makes sense, it would be nice if we could converge
> > > on a common solution, or at least name our parameters sufficiently
> > > distinctly to avoid confusion :)  
> > 
> > I think that the above scheme is a bit too complicated and I'm not sure
> > this is warranted. I'll try to better explain the motivation for this
> > parameter and where we are coming from.
> 
> Certainly, let me know if what I wrote above helps to understand the
> motivation.

Yes, it does. Thanks!

> 
> > We want to keep things as simple as possible. This means we don't want
> > users to fiddle with devlink parameter unless they have to. Things
> > should just work.
> 
> 100% agree, you can choose the default for the parameter to be whatever
> you want, nobody will have to touch it in normal operation.
> 
> I'm just proposing widening the values of the parameter so it works for
> others (given you propose it as generic).
> 
> > This parameter should only be used in exceptional cases.
> > 
> > For example, when user reports a problem with current firmware version
> > enforced by the driver. Assuming we have a new firmware version with a
> > fix, we would like the user to try it and confirm bug was fixed.
> > Ideally, the user would do something like this:
> > 
> > 1. Flash new firmware via ethtool
> > 2. Perform a reset via devlink to have changes take effect
> > 
> > Problem is that after the reset the driver's init sequence will run and
> > overwrite the new firmware version with the one specified in its source
> > as a compatible version. The driver needs to enforce a specific version
> > because newer versions are not necessarily backward compatible.
> > 
> > Therefore, we added this new parameter that gives the user the ability
> > to explicitly run with a different version than what was specified as
> > compatible. New sequence is therefore:
> > 
> > 1. Flash new firmware via ethtool
> > 2. Toggle devlink parameter
> > 3. Perform a reset via devlink to have changes take effect
> > 
> > Firmware loading policy is basically always go with what the driver is
> > enforcing (it knows best), unless user specified he/she knows better.
> 
> Thanks, got it.
> 
> > I think this is both generic and simple, but I possibly didn't
> > understand the full scope of your use cases.
> 
> I agree, it is simple, but the semantics of the parameter you're adding
> unnecessarily involve firmware flashing, which is mlxsw quirk.  My
> impression (mostly from my WiFi driver days) is that the FW is either in
> flash or in /lib/firmware, but rarely /lib/firmware autofeeds the flash.
> 
> The commit message says "Many drivers checking the device's firmware
> version during the initialization flow and flashing a compatible
> version if the current version is not."  Do Ethernet drivers do that 
> or some other drivers?

Yes, the terminology is not accurate. The intention was that drivers
load (we used flashing...) a compatible firmware version during their
initialization routine.

> To reiterate I think the definition of the flag unnecessarily involves
> flashing.  It may make sense to mlxsw users, but I'd postulate it won't
> to almost everyone else :)
> 
> Therefore AFAICS we could make one of two improvements:
>  - make this a full-fledged flash update policy with clear operational
>    semantics which others can reuse; or 
>  - remove the flashing part and leave it unmentioned - definition of the
>    parameter becomes in a nutshell "ignore all FW version
>    incompatibilities"; the limitation of mlxsw having to flash a FW
>    image to load it is then an implementation detail.

Discussed the topic with Jakub during the bi-weekly switchdev call. We
will submit a v2 with a parameter called 'fw_load_policy' that will have
these options:

* driver: Load firmware version preferred by the driver (default in 
  mlxsw)
* flash: Load firmware currently stored in flash

Therefore, new sequence is:

1. Flash new firmware via ethtool
2. Set devlink 'fw_load_policy' parameter to 'flash'
3. Perform a reset via devlink to have changes take effect

The parameter can be later extended with more options such as 'newest'
and 'disk' that Jakub mentioned earlier in the thread.

We will not add the 'fw_flash_auto_update' parameter as it is not needed
for mlxsw, but can be added for nfp/others in the future.

Jakub, thanks again for your time!