On Wed, 2022-01-12 at 10:35 +0100, Johannes Berg wrote: > On Wed, 2022-01-12 at 09:33 +0000, Coelho, Luciano wrote: > > On Wed, 2022-01-12 at 10:27 +0100, Johannes Berg wrote: > > > > > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > > > > b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > > > > index 863fec150e53..f13825185094 100644 > > > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > > > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/fw.c > > > > @@ -834,6 +834,9 @@ static int iwl_mvm_sar_geo_init(struct iwl_mvm > > > > *mvm) > > > > offsetof(struct > > > > iwl_geo_tx_power_profiles_cmd_v4, ops) != > > > > offsetof(struct > > > > iwl_geo_tx_power_profiles_cmd_v5, ops)); > > > > > > > > + if (!iwl_sar_geo_support(&mvm->fwrt)) > > > > + return -EOPNOTSUPP; > > > > + > > > > /* the ops field is at the same spot for all versions, so > > > > set in v1 */ > > > > cmd.v1.ops = cpu_to_le32(IWL_PER_CHAIN_OFFSET_SET_TABLES); > > > > > > I was going to say it should probably return 0, but the caller looks > > > a > > > bit fishy too? > > > > > > ret = iwl_mvm_sar_init(mvm); > > > if (ret == 0) > > > ret = iwl_mvm_sar_geo_init(mvm); > > > else if (ret < 0) > > > goto error; > > > > > > ret = iwl_mvm_sgom_init(mvm); > > > > > > should that "else" be removed? > > > > Yeah, I noticed the same thing when I checked the return value... I > > don't think we want to abort everything if SAR GEO init failed, so > > maybe we should just remove the return value from the function? > > > > Well the only real failure path there is "we cannot send the command", > in which case we might as well abort? > > IOW, we already return 0 in the cases where we don't have the data or > something else happened (also for !CONFIG_ACPI). Yeah, okay, not being able to send the command is a bit bad. My point was that SAR GEO is not that critical. It will improve connection in certain cases in certain locations (by increasing the TX power), but would still work and abide to regulatory rules... -- Luca.
