| Message ID | 1508937247-11890-4-git-send-email-qca_merez@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Maya Erez <qca_merez@qca.qualcomm.com> writes: > From: Lior David <qca_liord@qca.qualcomm.com> > > FW capabilities are currently retrieved only during module > initialization, but userspace can replace the firmware while > interface is down, so refresh the FW capabilities when > interface is up (after FW is loaded) to ensure driver > functionality matches the loaded FW. I think usually the firmware is loaded only once during probe() and I think it's quite special that you retrieve it during interface up. Being able to change the firmware version runtime like that can lead to problems eventually, for example cfg80211 might not allow changing already registered configuration etc.
On 10/28/2017 6:01 PM, Kalle Valo wrote: > Maya Erez <qca_merez@qca.qualcomm.com> writes: > >> From: Lior David <qca_liord@qca.qualcomm.com> >> >> FW capabilities are currently retrieved only during module >> initialization, but userspace can replace the firmware while >> interface is down, so refresh the FW capabilities when >> interface is up (after FW is loaded) to ensure driver >> functionality matches the loaded FW. > > I think usually the firmware is loaded only once during probe() and I > think it's quite special that you retrieve it during interface up. Being > able to change the firmware version runtime like that can lead to > problems eventually, for example cfg80211 might not allow changing > already registered configuration etc. > Yes you are right, it is not perfect. We shutdown our chip in interface down so the FW is lost, we have to load it every interface up. We could request_firmware only once at insmod and store it so the same FW will be used every time but this is a big waste of memory (FW size is ~400KB and may be larger with future chips). We only touch one or two very simple fields in struct wiphy that are not validated in wiphy_register. The behavior is not 100% proof but good enough, and we recommend to our users to always rmmod/insmod when replacing FW (replacing FW at runtime is usually not done in production systems, this is only for debugging). However, if we do not refresh the capabilities we will have larger problems - the driver can try to access features not supported by FW and cause FW/host crashes.
Lior David <liord@codeaurora.org> writes: > On 10/28/2017 6:01 PM, Kalle Valo wrote: >> Maya Erez <qca_merez@qca.qualcomm.com> writes: >> >>> From: Lior David <qca_liord@qca.qualcomm.com> >>> >>> FW capabilities are currently retrieved only during module >>> initialization, but userspace can replace the firmware while >>> interface is down, so refresh the FW capabilities when >>> interface is up (after FW is loaded) to ensure driver >>> functionality matches the loaded FW. >> >> I think usually the firmware is loaded only once during probe() and I >> think it's quite special that you retrieve it during interface up. Being >> able to change the firmware version runtime like that can lead to >> problems eventually, for example cfg80211 might not allow changing >> already registered configuration etc. >> > Yes you are right, it is not perfect. > We shutdown our chip in interface down so the FW is lost, we have to load it > every interface up. We could request_firmware only once at insmod and store it > so the same FW will be used every time but this is a big waste of memory (FW > size is ~400KB and may be larger with future chips). We only touch one or two > very simple fields in struct wiphy that are not validated in wiphy_register. The > behavior is not 100% proof but good enough, and we recommend to our users to > always rmmod/insmod when replacing FW (replacing FW at runtime is usually not > done in production systems, this is only for debugging). > However, if we do not refresh the capabilities we will have larger problems - > the driver can try to access features not supported by FW and cause FW/host crashes. Yes, I agree that this patch does improve things and should be applied. But I think in the long run you should consider changing this and do the same as other drivers do (load the firmware only on probe()).
diff --git a/drivers/net/wireless/ath/wil6210/fw_inc.c b/drivers/net/wireless/ath/wil6210/fw_inc.c index e01acac..7a33792 100644 --- a/drivers/net/wireless/ath/wil6210/fw_inc.c +++ b/drivers/net/wireless/ath/wil6210/fw_inc.c @@ -124,24 +124,19 @@ static int fw_ignore_section(struct wil6210_priv *wil, const void *data, return 0; } -static int fw_handle_comment(struct wil6210_priv *wil, const void *data, - size_t size) -{ - wil_hex_dump_fw("", DUMP_PREFIX_OFFSET, 16, 1, data, size, true); - - return 0; -} - static int -fw_handle_capabilities(struct wil6210_priv *wil, const void *data, - size_t size) +fw_handle_comment(struct wil6210_priv *wil, const void *data, + size_t size) { const struct wil_fw_record_capabilities *rec = data; size_t capa_size; if (size < sizeof(*rec) || - le32_to_cpu(rec->magic) != WIL_FW_CAPABILITIES_MAGIC) + le32_to_cpu(rec->magic) != WIL_FW_CAPABILITIES_MAGIC) { + wil_hex_dump_fw("", DUMP_PREFIX_OFFSET, 16, 1, + data, size, true); return 0; + } capa_size = size - offsetof(struct wil_fw_record_capabilities, capabilities); @@ -422,7 +417,7 @@ static int fw_handle_gateway_data4(struct wil6210_priv *wil, const void *data, int (*parse_handler)(struct wil6210_priv *wil, const void *data, size_t size); } wil_fw_handlers[] = { - {wil_fw_type_comment, fw_handle_comment, fw_handle_capabilities}, + {wil_fw_type_comment, fw_handle_comment, fw_handle_comment}, {wil_fw_type_data, fw_handle_data, fw_ignore_section}, {wil_fw_type_fill, fw_handle_fill, fw_ignore_section}, /* wil_fw_type_action */ @@ -517,7 +512,7 @@ int wil_request_firmware(struct wil6210_priv *wil, const char *name, rc = request_firmware(&fw, name, wil_to_dev(wil)); if (rc) { - wil_err_fw(wil, "Failed to load firmware %s\n", name); + wil_err_fw(wil, "Failed to load firmware %s rc %d\n", name, rc); return rc; } wil_dbg_fw(wil, "Loading <%s>, %zu bytes\n", name, fw->size); diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c index bac829a..ad1f9f8 100644 --- a/drivers/net/wireless/ath/wil6210/main.c +++ b/drivers/net/wireless/ath/wil6210/main.c @@ -761,6 +761,8 @@ static void wil_collect_fw_info(struct wil6210_priv *wil) u8 retry_short; int rc; + wil_refresh_fw_capabilities(wil); + rc = wmi_get_mgmt_retry(wil, &retry_short); if (!rc) { wiphy->retry_short = retry_short; @@ -768,6 +770,25 @@ static void wil_collect_fw_info(struct wil6210_priv *wil) } } +void wil_refresh_fw_capabilities(struct wil6210_priv *wil) +{ + struct wiphy *wiphy = wil_to_wiphy(wil); + + wil->keep_radio_on_during_sleep = + wil->platform_ops.keep_radio_on_during_sleep && + wil->platform_ops.keep_radio_on_during_sleep( + wil->platform_handle) && + test_bit(WMI_FW_CAPABILITY_D3_SUSPEND, wil->fw_capabilities); + + wil_info(wil, "keep_radio_on_during_sleep (%d)\n", + wil->keep_radio_on_during_sleep); + + if (test_bit(WMI_FW_CAPABILITY_RSSI_REPORTING, wil->fw_capabilities)) + wiphy->signal_type = CFG80211_SIGNAL_TYPE_MBM; + else + wiphy->signal_type = CFG80211_SIGNAL_TYPE_UNSPEC; +} + void wil_mbox_ring_le2cpus(struct wil6210_mbox_ring *r) { le32_to_cpus(&r->base); @@ -1072,11 +1093,11 @@ int wil_reset(struct wil6210_priv *wil, bool load_fw) return rc; } + wil_collect_fw_info(wil); + if (wil->ps_profile != WMI_PS_PROFILE_TYPE_DEFAULT) wil_ps_update(wil, wil->ps_profile); - wil_collect_fw_info(wil); - if (wil->platform_ops.notify) { rc = wil->platform_ops.notify(wil->platform_handle, WIL_PLATFORM_EVT_FW_RDY); diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c b/drivers/net/wireless/ath/wil6210/pcie_bus.c index 9cc97da..659c32c 100644 --- a/drivers/net/wireless/ath/wil6210/pcie_bus.c +++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c @@ -85,9 +85,7 @@ void wil_set_capabilities(struct wil6210_priv *wil) /* extract FW capabilities from file without loading the FW */ wil_request_firmware(wil, wil->wil_fw_name, false); - - if (test_bit(WMI_FW_CAPABILITY_RSSI_REPORTING, wil->fw_capabilities)) - wil_to_wiphy(wil)->signal_type = CFG80211_SIGNAL_TYPE_MBM; + wil_refresh_fw_capabilities(wil); } void wil_disable_irq(struct wil6210_priv *wil) @@ -297,15 +295,6 @@ static int wil_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id) wil_set_capabilities(wil); wil6210_clear_irq(wil); - wil->keep_radio_on_during_sleep = - wil->platform_ops.keep_radio_on_during_sleep && - wil->platform_ops.keep_radio_on_during_sleep( - wil->platform_handle) && - test_bit(WMI_FW_CAPABILITY_D3_SUSPEND, wil->fw_capabilities); - - wil_info(wil, "keep_radio_on_during_sleep (%d)\n", - wil->keep_radio_on_during_sleep); - /* FW should raise IRQ when ready */ rc = wil_if_pcie_enable(wil); if (rc) { diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h index ad40a5a..8dfb4a7 100644 --- a/drivers/net/wireless/ath/wil6210/wil6210.h +++ b/drivers/net/wireless/ath/wil6210/wil6210.h @@ -872,6 +872,7 @@ int wil_ps_update(struct wil6210_priv *wil, int __wil_up(struct wil6210_priv *wil); int wil_down(struct wil6210_priv *wil); int __wil_down(struct wil6210_priv *wil); +void wil_refresh_fw_capabilities(struct wil6210_priv *wil); void wil_mbox_ring_le2cpus(struct wil6210_mbox_ring *r); int wil_find_cid(struct wil6210_priv *wil, const u8 *mac); void wil_set_ethtoolops(struct net_device *ndev);