From:   "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>,
        Stephen Rothwell <sfr@canb.auug.org.au>,
        Luca Coelho <luca@coelho.fi>
CC:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: RE: [bug report] iwlwifi: integrate with iwlmei
Thread-Topic: [bug report] iwlwifi: integrate with iwlmei
Thread-Index: AQHX5byIEmmivywbRkyKKA+xHudB6qwbuUkg
Date:   Tue, 30 Nov 2021 08:17:37 +0000
Message-ID: <SA1PR11MB582559489B02E37FAAFFEAA1F2679@SA1PR11MB5825.namprd11.prod.outlook.com>
References: <20211130073226.GA21360@kili>
In-Reply-To: <20211130073226.GA21360@kili>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.200.16
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?pweR53so8xmrht05pJOAYRHLHhHuSbh3VuRWmSm7hcdkUuCXZrfbk+MxO8wT?=
 =?us-ascii?Q?h6BnP7j8V9aZvShc1diyBYSN4H4FssIhRYj0Oh1ya/TD29S2ovQMPbTKljbe?=
 =?us-ascii?Q?7mRV4lQKnUeoIpap3W5y4w/M2wH9RlfGxPXyNLpY4CJ2XJ/Aa6BWr/McIFk2?=
 =?us-ascii?Q?8+XHxUoJjOxVGYs5YNv2pp2AdjtE0aiBvBzyKYtpfh3YIyjAPoX21c0pOenU?=
 =?us-ascii?Q?izI8GIgDRRVDEXtrpRGHvWUzDhmzq88eYt6Zm69mn8DRfYDKxaK1Iz/H/jnB?=
 =?us-ascii?Q?FQUm2LI4Zz5B9P2Wjz2b/1dT6JMiqj867RO75BouGL62/eugWUgE585CLt/u?=
 =?us-ascii?Q?821tmWJ7zFPoRT2KTR/reel1jP5j8WHtHbRC/WqqG7glmGwhZh8nnVwPIddB?=
 =?us-ascii?Q?PgbDWb3MsYgAgskg/wJsXdattq3ZZXgBp8K9E02GOHm1nnaVJPqu1pjLTbCd?=
 =?us-ascii?Q?7aYRlf/K0+/6x4QxeWS9TpdBz7DHhxU5QO3T31AH3VdWZaJfZwkmA1LDS6qF?=
 =?us-ascii?Q?golUwlVg3UCzUa9wQWSZHxkrVQwgtVvHOp/wY29a47K/7sYoFW6EhIyD/9Hq?=
 =?us-ascii?Q?dlcm7mTTOuWRM/3cOBdj//a+ky/UU9K9pH6yAQFU4fQeMHCo4hkkUeuCJ90K?=
 =?us-ascii?Q?ZbOoq2Z1QPTA3FK/NaIuCkPWsDBtR9goeT4nwQ55YNGzxdwxdFQ8b+WxZEQq?=
 =?us-ascii?Q?FB4/xakoXYS3cEeVPrMWz91OBhM284Ik7KpzFE3g/6J74b5tTAU5iYkvRNBq?=
 =?us-ascii?Q?AFzuKpLYIlbhhZnlD2ZVxRVZFuyP8OlW0bF8TAk8pATH9CKjUPhBD0qEKL6O?=
 =?us-ascii?Q?FAXLGkAqWpEpmFnMocdl2Ial8yNGQgpd3DGh5A+ye6zOmVZtu2YAakrT/eat?=
 =?us-ascii?Q?MtM8tIKgff8V+UbzqYGrXDtoHVn56YwsJcpFee4rGI8cDi6DSKb6LyuwyQWh?=
 =?us-ascii?Q?EHScyML7toq8LfYY2H2CqTbDIU0Hp93VNLKeIfW0oacYkUFYAj817DeERJJc?=
 =?us-ascii?Q?3CzUcgnhVAyUoSHoXmudHkyG8ywBCN8otJ1qFLB3pSYQU5WKUn5EQ8nvfd+l?=
 =?us-ascii?Q?sK5jxK/Q0C4yyYyCXrwHUT6E/2s9LnNrzKVzmkeM24eq7QK9fo3gysfRzULT?=
 =?us-ascii?Q?HVZiXw2S3R0SIgOpnjcqzZ2a+X8xhJTADXBKrjoIVbRCPkj6X1+O6t0NlXR4?=
 =?us-ascii?Q?ieQIHds3QQL1V2W4MSRGrgiErmzjMPxJGFJeY2DN9ftEhJzOH3DqJajpIwbd?=
 =?us-ascii?Q?58vXW95PpT/JWUKxEn+4rWIh+P4RczdXbJ2r1p58eAHHuVZH8cABKV+DVc5q?=
 =?us-ascii?Q?yXWbMjrD9T1NIHIKvobJ63L/rv4lcHQhhDmT6v9lCORRHfnnYX48irS772YZ?=
 =?us-ascii?Q?e3m165SaOJNOwBay1J/5LcEYpT4gA2bW1AepdOEce/Ba7JBLKirfTHj/bH0j?=
 =?us-ascii?Q?ZJqBPLHrZ3s1ntA7lAYSOYCy5uh2AsgrlY+1I0vhscjG08bmWbdNMMa2UdVU?=
 =?us-ascii?Q?qGudo2/V8oQhV6qXlFIiaXIKbio9Wp4VCQo1BJQHhPqG8FnCNsK80UbIYLi/?=
 =?us-ascii?Q?wA5oHISz1o8lCvcb1Hg+4SHsqt9pf43KU/ZU7YzEMhjWrH8A1ZiFIzTCvIGp?=
 =?us-ascii?Q?dpzRMqI3oeJngnwjzgrR28O2CJYZlo+GIpJ09bReFtCkzPokC9Z4g9TIEc/q?=
 =?us-ascii?Q?KIBvvw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB5825.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ec20bb99-07af-4522-3f42-08d9b3d9dee8
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Nov 2021 08:17:37.7438
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 1OLTnPJ4FGcHkLD1IQu9YpgQMUENDSyJStRed+apylrSo0uANQcgbybV9JiPHghE1sI3PWg4V9MwoCbSH5ipOfZQnFAIeZpl1/zBM/clnJ0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR11MB3325
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

Hi Dan,

>=20
> Hello Emmanuel Grumbach,
>=20
> The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 202=
1,
> leads to the following Smatch static checker warning:
>=20
> 	drivers/net/wireless/intel/iwlwifi/mvm/ops.c:740
> iwl_mvm_start_get_nvm()
> 	warn: inconsistent returns '&mvm->hw->wiphy->mtx'.
>=20
> drivers/net/wireless/intel/iwlwifi/mvm/ops.c
>     684 static int iwl_mvm_start_get_nvm(struct iwl_mvm *mvm)
>     685 {
>     686         struct iwl_trans *trans =3D mvm->trans;
>     687         int ret;
>     688
>     689         if (trans->csme_own) {
>     690                 if (WARN(!mvm->mei_registered,
>     691                          "csme is owner, but we aren't registered=
 to iwlmei\n"))
>     692                         goto get_nvm_from_fw;
>     693
>     694                 mvm->mei_nvm_data =3D iwl_mei_get_nvm();
>     695                 if (mvm->mei_nvm_data) {
>     696                         /*
>     697                          * mvm->mei_nvm_data is set and because o=
f that,
>     698                          * we'll load the NVM from the FW when we=
'll get
>     699                          * ownership.
>     700                          */
>     701                         mvm->nvm_data =3D
>     702                                 iwl_parse_mei_nvm_data(trans, tra=
ns->cfg,
>     703                                                        mvm->mei_n=
vm_data, mvm->fw);
>     704                         return 0;
>     705                 }
>     706
>     707                 IWL_ERR(mvm,
>     708                         "Got a NULL NVM from CSME, trying to get =
it from the
> device\n");
>     709         }
>     710
>     711 get_nvm_from_fw:
>     712         rtnl_lock();
>     713         wiphy_lock(mvm->hw->wiphy);
>     714         mutex_lock(&mvm->mutex);
>=20
> This code takes three lines.  I'm looking at linux-next next-20211129, so=
 it's a
> little bit different.  The original patch is buggy but it's made worse by=
 a merge
> issue.
>=20
>=20
>     715
>     716         ret =3D iwl_trans_start_hw(mvm->trans);
>     717         if (ret) {
>     718                 mutex_unlock(&mvm->mutex);
>     719                 return ret;
>=20
> This only drops one lock before returning.  It should probably be a goto
> unlock; and we add an unlock at the end of the function.  I would send a
> patch for that but it gets a bit confusing because of the merge.
> Emmanuel, could you take a look at this?

Luca has a patch in the pipe to fix this. Clearly, we had merge issues here=
.
Luca, it looks like the commit:
"iwlwifi: mvm: unlock RTNL when starting HW fails" is the one we need.

What happened here is that, internally, we had two features that were imple=
mented
in a certain order (iwlmei before the load of the regdomain in INIT) and up=
stream
they were merged the other way around. This caused merge issues that weren'=
t handled
properly apparently.

Luca, can you take a  look?

>=20
>     720         }
>     721
>     722         ret =3D iwl_run_init_mvm_ucode(mvm);
>     723         if (ret && ret !=3D -ERFKILL)
>     724                 iwl_fw_dbg_error_collect(&mvm->fwrt,
> FW_DBG_TRIGGER_DRIVER);
>     725         if (!ret && iwl_mvm_is_lar_supported(mvm)) {
>     726                 mvm->hw->wiphy->regulatory_flags |=3D
> REGULATORY_WIPHY_SELF_MANAGED;
>     727                 ret =3D iwl_mvm_init_mcc(mvm);
>     728         }
>     729
>     730         if (!iwlmvm_mod_params.init_dbg || !ret)
>     731                 iwl_mvm_stop_device(mvm);
>     732
>     733         mutex_unlock(&mvm->mutex);
>     734         wiphy_unlock(mvm->hw->wiphy);
>     735         rtnl_unlock();
>     736
>     737         if (ret)
>     738                 IWL_ERR(mvm, "Failed to run INIT ucode: %d\n", re=
t);
>     739
> --> 740         return ret;
>     741 }
>=20
> regards,
> dan carpenter