Hello Emmanuel Grumbach,
The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12,
2021, leads to the following Smatch static checker warning:
drivers/net/wireless/intel/iwlwifi/mvm/ops.c:740 iwl_mvm_start_get_nvm()
warn: inconsistent returns '&mvm->hw->wiphy->mtx'.
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 = 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 = iwl_mei_get_nvm();
695 if (mvm->mei_nvm_data) {
696 /*
697 * mvm->mei_nvm_data is set and because of that,
698 * we'll load the NVM from the FW when we'll get
699 * ownership.
700 */
701 mvm->nvm_data =
702 iwl_parse_mei_nvm_data(trans, trans->cfg,
703 mvm->mei_nvm_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);
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.
715
716 ret = iwl_trans_start_hw(mvm->trans);
717 if (ret) {
718 mutex_unlock(&mvm->mutex);
719 return ret;
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?
720 }
721
722 ret = iwl_run_init_mvm_ucode(mvm);
723 if (ret && ret != -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 |= REGULATORY_WIPHY_SELF_MANAGED;
727 ret = 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", ret);
739
--> 740 return ret;
741 }
regards,
dan carpenter
Hi Dan,
>
> Hello Emmanuel Grumbach,
>
> The patch 6d19a5eba5cd: "iwlwifi: integrate with iwlmei" from Nov 12, 2021,
> leads to the following Smatch static checker warning:
>
> drivers/net/wireless/intel/iwlwifi/mvm/ops.c:740
> iwl_mvm_start_get_nvm()
> warn: inconsistent returns '&mvm->hw->wiphy->mtx'.
>
> 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 = 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 = iwl_mei_get_nvm();
> 695 if (mvm->mei_nvm_data) {
> 696 /*
> 697 * mvm->mei_nvm_data is set and because of that,
> 698 * we'll load the NVM from the FW when we'll get
> 699 * ownership.
> 700 */
> 701 mvm->nvm_data =
> 702 iwl_parse_mei_nvm_data(trans, trans->cfg,
> 703 mvm->mei_nvm_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);
>
> 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.
>
>
> 715
> 716 ret = iwl_trans_start_hw(mvm->trans);
> 717 if (ret) {
> 718 mutex_unlock(&mvm->mutex);
> 719 return ret;
>
> 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 implemented
in a certain order (iwlmei before the load of the regdomain in INIT) and upstream
they were merged the other way around. This caused merge issues that weren't handled
properly apparently.
Luca, can you take a look?
>
> 720 }
> 721
> 722 ret = iwl_run_init_mvm_ucode(mvm);
> 723 if (ret && ret != -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 |=
> REGULATORY_WIPHY_SELF_MANAGED;
> 727 ret = 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", ret);
> 739
> --> 740 return ret;
> 741 }
>
> regards,
> dan carpenter