Marc Gonzalez <mgonzalez@xxxxxxxxxx> writes: > On 01/03/2024 09:10, Kalle Valo wrote: > >> Marc Gonzalez wrote: >> >>> Kalle Valo wrote: >>> >>>> Here's one example where in ath10k we use a feature bit as a workaround: >>>> >>>> /* Don't trust error code from otp.bin */ >>>> ATH10K_FW_FEATURE_IGNORE_OTP_RESULT = 7, >>>> >>>> .... >>>> >>>> if (!(skip_otp || test_bit(ATH10K_FW_FEATURE_IGNORE_OTP_RESULT, >>>> ar->running_fw->fw_file.fw_features)) && >>>> result != 0) { >>>> ath10k_err(ar, "otp calibration failed: %d", result); >>>> return -EINVAL; >>>> } >>>> >>>> BTW for modifying firmware-N.bin files we have a script here: >>>> >>>> https://github.com/qca/qca-swiss-army-knife/blob/master/tools/scripts/ath10k/ath10k-fwencoder >>> >>> If I understand correctly, you are saying that there is >>> (maybe... probably) a bug in the FW, so it makes sense to >>> tag that specific FW file with a special bit which the kernel >>> will interpret as "this FW is broken in a specific way; >>> and here's how to work around the issue." >>> >>> So this bit would serve the same purpose as my proposed >>> "qcom,no-msa-ready-indicator" bit (that bit existed instead >>> in my board's device tree). >>> >>> The problem I see is that the firmware files are signed. >>> Thus, changing a single bit breaks the verification... >>> UNLESS the FW format allows for a signed section ALONG-SIDE >>> an unsigned section? >> >> firmware-N.bin is ath10k specific container file format and we (the >> Linux community) have full access to it using ath10k-fwencoder, there's >> no signing or anything like that. One of the major reasons why it was >> designed was to handle differences between firmware branches, just like >> in this case. >> >> Of course plan A should be to fix the firmware but if that doesn't work >> out then plan B could be using the feature bit in firmware-N.bin. >> >> BTW related to this Dmitry is extending firmware-N.bin handling for >> WCN3990, you will most likely need to use that: >> >> https://patchwork.kernel.org/project/linux-wireless/cover/20240130-wcn3990-firmware-path-v1-0-826b93202964@xxxxxxxxxx/ > > > If I understand correctly (happy to have anyone correct any > misunderstandings), if the FW cannot be fixed (for any reason), > then we would have to do something like this: Thanks, this is exactly what I'm proposing. > diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c > index 0032f8aa892ff..c8778ebe922af 100644 > --- a/drivers/net/wireless/ath/ath10k/core.c > +++ b/drivers/net/wireless/ath/ath10k/core.c > @@ -769,6 +769,7 @@ static const char *const ath10k_core_fw_feature_str[] = { > [ATH10K_FW_FEATURE_SINGLE_CHAN_INFO_PER_CHANNEL] = "single-chan-info-per-channel", > [ATH10K_FW_FEATURE_PEER_FIXED_RATE] = "peer-fixed-rate", > [ATH10K_FW_FEATURE_IRAM_RECOVERY] = "iram-recovery", > + [ATH10K_FW_FEATURE_NO_MSA_READY] = "no-msa-ready-indicator", For consistency I would have just "no-msa-ready". > @@ -1151,6 +1154,9 @@ struct ath10k { > u8 cfg_tx_chainmask; > u8 cfg_rx_chainmask; > > + /* FW does not send MSA_READY indicator. Fake it */ > + bool fake_msa_ready; /* bool or u8? or s8? or bitfield? */ Hopefully not needed, see below. > struct completion install_key_done; > > int last_wmi_vdev_start_status; > diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c > index 38e939f572a9e..0776e79b25f3a 100644 > --- a/drivers/net/wireless/ath/ath10k/qmi.c > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > @@ -1040,6 +1040,8 @@ static void ath10k_qmi_driver_event_work(struct work_struct *work) > switch (event->type) { > case ATH10K_QMI_EVENT_SERVER_ARRIVE: > ath10k_qmi_event_server_arrive(qmi); > + if (ar->fake_msa_ready) > + ath10k_qmi_event_msa_ready(qmi); Unless I'm missing something I would use here test_bit() directly: if (test_bit(ATH10K_FW_FEATURE_NO_MSA_READY, ar->running_fw->fw_file.fw_features)) ath10k_qmi_event_msa_ready(qmi); -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches