| Message ID | 20240920165916.9592-3-marcin.szycik@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [iwl-net,v2,1/2] ice: Fix entering Safe Mode | expand |
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.kitszel@intel.com> > Signed-off-by: Marcin Szycik <marcin.szycik@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 > >
On 20.09.2024 19:14, Brett Creeley wrote: > > > 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.kitszel@intel.com> >> Signed-off-by: Marcin Szycik <marcin.szycik@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); This sounds reasonable, I'll change it if there will be no more comments. > 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. ice_request_fw() already prints a dev_err() message when entering safe mode, so I don't think it's necessary here. Thanks, Marcin > > 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 >> >>
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); /* 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
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.kitszel@intel.com> Signed-off-by: Marcin Szycik <marcin.szycik@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(-)