On 9/20/2024 9:59 AM, Marcin Szycik wrote:
Caution: This message originated from an External Source. Use proper caution
when opening attachments, clicking links, or responding.
If DDP package is missing or corrupted, the driver should enter Safe Mode.
Instead, an error is returned and probe fails.
Don't check return value of ice_init_ddp_config() to fix this.
Change ice_init_ddp_config() type to void, as now its return is never
checked.
Repro:
* Remove or rename DDP package (/lib/firmware/intel/ice/ddp/ice.pkg)
* Load ice
Fixes: cc5776fe1832 ("ice: Enable switching default Tx scheduler topology")
Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
Signed-off-by: Marcin Szycik <marcin.szy...@linux.intel.com>
---
v2: Change ice_init_ddp_config() type to void
---
drivers/net/ethernet/intel/ice/ice_main.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index 0f5c9d347806..aeebf4ae25ae 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4548,34 +4548,27 @@ ice_init_tx_topology(struct ice_hw *hw, const struct
firmware *firmware)
*
* This function loads DDP file from the disk, then initializes Tx
* topology. At the end DDP package is loaded on the card.
- *
- * Return: zero when init was successful, negative values otherwise.
*/
-static int ice_init_ddp_config(struct ice_hw *hw, struct ice_pf *pf)
+static void ice_init_ddp_config(struct ice_hw *hw, struct ice_pf *pf)
{
struct device *dev = ice_pf_to_dev(pf);
const struct firmware *firmware = NULL;
int err;
err = ice_request_fw(pf, &firmware);
- if (err) {
+ if (err)
dev_err(dev, "Fail during requesting FW: %d\n", err);
- return err;
- }
err = ice_init_tx_topology(hw, firmware);
if (err) {
dev_err(dev, "Fail during initialization of Tx topology: %d\n",
err);
release_firmware(firmware);
- return err;
}
/* Download firmware to device */
ice_load_pkg(firmware, pf);
release_firmware(firmware);
-
- return 0;
}
/**
@@ -4748,9 +4741,7 @@ int ice_init_dev(struct ice_pf *pf)
ice_init_feature_support(pf);
- err = ice_init_ddp_config(hw, pf);
- if (err)
- return err;
+ ice_init_ddp_config(hw, pf);
I just commented this on v1 as I didn't expect it to be resent. I'm also
okay with Maciej's suggestion, but I wanted to offer an alternative option.
As an alternative solution you could potentially do the following, which
would make the flow more readable:
err = ice_init_ddp_config(hw, pf);
if (err || ice_is_safe_mode(pf))
ice_set_safe_mode_caps(hw);
Also, should there be some sort of messaging if the device goes into
safe mode? I wonder if a dev_dbg() would be better than nothing. If
ice_init_ddp_config() fails, then it will print an error message, so
maybe a dev_warn/info() is warranted if (err)? Of course this would
depend on ice_init_ddp_config() to return a non-void value.
Thanks,
Brett
/* if ice_init_ddp_config fails, ICE_FLAG_ADV_FEATURES bit won't be
* set in pf->state, which will cause ice_is_safe_mode to return
--
2.45.0
