| Message ID | 20181003073556.28154-3-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | None | expand |
Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > To avoid the firmware loading race between Bluetooth and WiFi on Intel > 8260, load firmware exclusively when BT_INTEL is enabled. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Still the commit log tells nothing about the actual problem which makes review impossible.
> On Oct 3, 2018, at 5:10 PM, Kalle Valo <kvalo@codeaurora.org> wrote: > > Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > >> To avoid the firmware loading race between Bluetooth and WiFi on Intel >> 8260, load firmware exclusively when BT_INTEL is enabled. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Still the commit log tells nothing about the actual problem which makes > review impossible. Sorry for that. The first two patches [1] only sends to linux-bluetooth and LMKL. I don’t know what really happened at hardware/firmware level, but making btusb and iwlwifi load firmware sequentially can workaround the issue. Matt Chen may be able to explain this issue with more detail. [1] https://lkml.org/lkml/2018/10/3/322 > > -- > Kalle Valo
Kai Heng Feng <kai.heng.feng@canonical.com> writes: >> On Oct 3, 2018, at 5:10 PM, Kalle Valo <kvalo@codeaurora.org> wrote: >> >> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >> >>> To avoid the firmware loading race between Bluetooth and WiFi on Intel >>> 8260, load firmware exclusively when BT_INTEL is enabled. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> Still the commit log tells nothing about the actual problem which makes >> review impossible. > > Sorry for that. The first two patches [1] only sends to linux-bluetooth and LMKL. For a patchset like this you should CC linux-wireless for all patches, otherwise people just get confused. And even more so as patch 3 seems to depend on the other patches. > I don’t know what really happened at hardware/firmware level, but > making btusb and iwlwifi load firmware sequentially can workaround the > issue. We don't apply ugly workarounds without understanding the issue. > Matt Chen may be able to explain this issue with more detail. Then you need to work with Matt so that the issue is properly explained in the commit log.
Kalle Valo <kvalo@codeaurora.org> writes: > Kai Heng Feng <kai.heng.feng@canonical.com> writes: > >>> On Oct 3, 2018, at 5:10 PM, Kalle Valo <kvalo@codeaurora.org> wrote: >>> >>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >>> >>>> To avoid the firmware loading race between Bluetooth and WiFi on Intel >>>> 8260, load firmware exclusively when BT_INTEL is enabled. >>>> >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> >>> Still the commit log tells nothing about the actual problem which makes >>> review impossible. >> >> Sorry for that. The first two patches [1] only sends to linux-bluetooth and LMKL. > > For a patchset like this you should CC linux-wireless for all patches, > otherwise people just get confused. And even more so as patch 3 seems to > depend on the other patches. > >> I don’t know what really happened at hardware/firmware level, but >> making btusb and iwlwifi load firmware sequentially can workaround the >> issue. > > We don't apply ugly workarounds without understanding the issue. > >> Matt Chen may be able to explain this issue with more detail. > > Then you need to work with Matt so that the issue is properly explained > in the commit log. linux-bluetooth was not CCed, adding that.
> > > On Oct 3, 2018, at 5:10 PM, Kalle Valo <kvalo@codeaurora.org> wrote: > > > > Kai-Heng Feng <kai.heng.feng@canonical.com> writes: > > > >> To avoid the firmware loading race between Bluetooth and WiFi on > >> Intel 8260, load firmware exclusively when BT_INTEL is enabled. > >> > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > > > Still the commit log tells nothing about the actual problem which > > makes review impossible. > > Sorry for that. The first two patches [1] only sends to linux-bluetooth and > LMKL. > > I don’t know what really happened at hardware/firmware level, but making > btusb and iwlwifi load firmware sequentially can workaround the issue. > > Matt Chen may be able to explain this issue with more detail. > > [1] https://lkml.org/lkml/2018/10/3/322 > I just read the code of this patch and I don't quite understand. You have a function that is declared as a non-inline function in two different header files? btintel_firmware_lock is declared here: --- /dev/null +++ b/include/linux/intel-wifi-bt.h @@ -0,0 +1,8 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __INTEL_WIFI_BT_H__ +#define __INTEL_WIFI_BT_H__ + +void btintel_firmware_lock(void); And ... diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h index 41c642cc523f..1373ffc2b575 100644 --- a/drivers/bluetooth/btintel.h +++ b/drivers/bluetooth/btintel.h @@ -102,6 +102,8 @@ int btintel_read_boot_params(struct hci_dev *hdev, struct intel_boot_params *params); int btintel_download_firmware(struct hci_dev *dev, const struct firmware *fw, u32 *boot_param); +void btintel_firmware_lock(void); This can't be right.
+ linux-bluetooth "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> writes: >> >> > On Oct 3, 2018, at 5:10 PM, Kalle Valo <kvalo@codeaurora.org> wrote: >> > >> > Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >> > >> >> To avoid the firmware loading race between Bluetooth and WiFi on >> >> Intel 8260, load firmware exclusively when BT_INTEL is enabled. >> >> >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> > >> > Still the commit log tells nothing about the actual problem which >> > makes review impossible. >> >> Sorry for that. The first two patches [1] only sends to linux-bluetooth and >> LMKL. >> >> I don’t know what really happened at hardware/firmware level, but making >> btusb and iwlwifi load firmware sequentially can workaround the issue. >> >> Matt Chen may be able to explain this issue with more detail. >> >> [1] https://lkml.org/lkml/2018/10/3/322 >> > > I just read the code of this patch and I don't quite understand. > You have a function that is declared as a non-inline function in two different header files? > btintel_firmware_lock is declared here: > > --- /dev/null > +++ b/include/linux/intel-wifi-bt.h > @@ -0,0 +1,8 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __INTEL_WIFI_BT_H__ > +#define __INTEL_WIFI_BT_H__ > + > +void btintel_firmware_lock(void); > > And ... > > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h > index 41c642cc523f..1373ffc2b575 100644 > --- a/drivers/bluetooth/btintel.h > +++ b/drivers/bluetooth/btintel.h > @@ -102,6 +102,8 @@ int btintel_read_boot_params(struct hci_dev *hdev, > struct intel_boot_params *params); > int btintel_download_firmware(struct hci_dev *dev, const struct firmware *fw, > u32 *boot_param); > +void btintel_firmware_lock(void); > > > This can't be right.
diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c index cc8c53dc0ab6..bbd7e199e968 100644 --- a/drivers/net/wireless/intel/iwlwifi/pcie/trans.c +++ b/drivers/net/wireless/intel/iwlwifi/pcie/trans.c @@ -72,6 +72,10 @@ #include <linux/pm_runtime.h> #include <linux/module.h> +#if IS_ENABLED(CONFIG_BT_INTEL) +#include <linux/intel-wifi-bt.h> +#endif + #include "iwl-drv.h" #include "iwl-trans.h" #include "iwl-csr.h" @@ -1335,6 +1339,10 @@ static int iwl_trans_pcie_start_fw(struct iwl_trans *trans, bool hw_rfkill; int ret; +#if IS_ENABLED(CONFIG_BT_INTEL) + void (*firmware_lock_func)(void); + void (*firmware_unlock_func)(void); +#endif /* This may fail if AMT took ownership of the device */ if (iwl_pcie_prepare_card_hw(trans)) { IWL_WARN(trans, "Exit HW not ready\n"); @@ -1401,8 +1409,37 @@ static int iwl_trans_pcie_start_fw(struct iwl_trans *trans, iwl_write32(trans, CSR_UCODE_DRV_GP1_CLR, CSR_UCODE_SW_BIT_RFKILL); /* Load the given image to the HW */ - if (trans->cfg->device_family >= IWL_DEVICE_FAMILY_8000) + if (trans->cfg->device_family >= IWL_DEVICE_FAMILY_8000) { +#if IS_ENABLED(CONFIG_BT_INTEL) + firmware_lock_func = symbol_request(btintel_firmware_lock); + firmware_unlock_func = symbol_request(btintel_firmware_unlock); + if (!firmware_lock_func || !firmware_unlock_func) { + if (firmware_lock_func) { + symbol_put(btintel_firmware_lock); + firmware_lock_func = NULL; + } + + if (firmware_unlock_func) { + symbol_put(btintel_firmware_unlock); + firmware_unlock_func = NULL; + } + } + + if (firmware_lock_func) + firmware_lock_func(); +#endif ret = iwl_pcie_load_given_ucode_8000(trans, fw); + +#if IS_ENABLED(CONFIG_BT_INTEL) + if (firmware_unlock_func) { + firmware_unlock_func(); + symbol_put(btintel_firmware_lock); + firmware_lock_func = NULL; + symbol_put(btintel_firmware_unlock); + firmware_unlock_func = NULL; + } +#endif + } else ret = iwl_pcie_load_given_ucode(trans, fw);
To avoid the firmware loading race between Bluetooth and WiFi on Intel 8260, load firmware exclusively when BT_INTEL is enabled. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- .../net/wireless/intel/iwlwifi/pcie/trans.c | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)