Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <770267e4ad06661383dd16afbd51690cfad58d49.camel@sipsolutions.net>
Subject: Re: iwlwifi 0000:3a:00.0: Microcode SW error detected. Restarting
 0x2000000.
From:   Johannes Berg <johannes@sipsolutions.net>
To:     "Coelho, Luciano" <luciano.coelho@intel.com>,
        "lenb@kernel.org" <lenb@kernel.org>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Date:   Wed, 12 Jan 2022 10:35:02 +0100
In-Reply-To: <9644c62f580e30a2b7fc3d62b67db0b1e64872c7.camel@intel.com>
References: <CAJvTdKm9PQrnyEG=b-8DbdnOHup8Ec6VqoKp3ZCQXwmvXU3FbA@mail.gmail.com>
         <0bb2a3b794c19514ad30ecc0ca60d1d6e9e438b0.camel@sipsolutions.net>
         <4c1089474bb5982aee82d58a9d300ca55e7e0c69.camel@intel.com>
         <ab9e48ae8dad3187d206d46fc1d997245ecbc3ef.camel@sipsolutions.net>
         <9644c62f580e30a2b7fc3d62b67db0b1e64872c7.camel@intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.2 (3.42.2-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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).

johannes