diff mbox series

[v2] wifi: ath10k: Add WLAN firmware image version info into smem

Message ID 20221104082828.14386-1-quic_youghand@quicinc.com
State New
Headers
Series [v2] wifi: ath10k: Add WLAN firmware image version info into smem |

Commit Message

Youghandhar Chintala Nov. 4, 2022, 8:28 a.m. UTC 
  In a SoC based solution, it would be useful to know the versions of the
various binary firmware blobs the system is running on. On a QCOM based
SoC, this info can be obtained from socinfo debugfs infrastructure. For
this to work, respective subsystem drivers have to export the firmware
version information to an SMEM based version information table.

Having firmware version information at one place will help quickly
figure out the firmware versions of various subsystems on the device
instead of going through builds/logs in an event of a system crash.

Fill WLAN firmware version information in SMEM version table to be
printed as part of socinfo debugfs infrastructure on a Qualcomm based
SoC.

This change is applicable only for WCN399X targets.

Reported-by: kernel test robot <lkp@intel.com>

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1

Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>

---
Changes from v1:
 - Changed print format specifier to %zu from %i
 - Changed ath10k_qmi_add_wlan_ver_smem() API argument
	  to const char *fw_build_id from char *fw_build_id
 - Changed version_string_size with MACRO
---
 drivers/net/wireless/ath/ath10k/qmi.c | 28 +++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Comments

Stephen Boyd Nov. 7, 2022, 5:35 p.m. UTC | #1 
Quoting Youghandhar Chintala (2022-11-04 01:28:28)
> In a SoC based solution, it would be useful to know the versions of the
> various binary firmware blobs the system is running on. On a QCOM based
> SoC, this info can be obtained from socinfo debugfs infrastructure. For
> this to work, respective subsystem drivers have to export the firmware
> version information to an SMEM based version information table.
>
> Having firmware version information at one place will help quickly
> figure out the firmware versions of various subsystems on the device
> instead of going through builds/logs in an event of a system crash.
>
> Fill WLAN firmware version information in SMEM version table to be
> printed as part of socinfo debugfs infrastructure on a Qualcomm based
> SoC.
>
> This change is applicable only for WCN399X targets.
>
> Reported-by: kernel test robot <lkp@intel.com>
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1
>
> Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>

The trailers go together, no blank lines between them.

>
> ---
> Changes from v1:
>  - Changed print format specifier to %zu from %i
>  - Changed ath10k_qmi_add_wlan_ver_smem() API argument
>           to const char *fw_build_id from char *fw_build_id
>  - Changed version_string_size with MACRO
> ---
>  drivers/net/wireless/ath/ath10k/qmi.c | 28 +++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
> index 66cb7a1e628a..928d78f6d494 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.c
> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
> @@ -14,6 +14,7 @@
>  #include <linux/net.h>
>  #include <linux/platform_device.h>
>  #include <linux/qcom_scm.h>
> +#include <linux/soc/qcom/smem.h>
>  #include <linux/string.h>
>  #include <net/sock.h>
>
> @@ -22,6 +23,8 @@
>
>  #define ATH10K_QMI_CLIENT_ID           0x4b4e454c
>  #define ATH10K_QMI_TIMEOUT             30
> +#define ATH10K_SMEM_IMAGE_VERSION_TABLE       469
> +#define ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX     13
>
>  static int ath10k_qmi_map_msa_permission(struct ath10k_qmi *qmi,
>                                          struct ath10k_msa_mem_info *mem_info)
> @@ -536,6 +539,29 @@ int ath10k_qmi_wlan_disable(struct ath10k *ar)
>         return ath10k_qmi_mode_send_sync_msg(ar, QMI_WLFW_OFF_V01);
>  }
>
> +static void ath10k_qmi_add_wlan_ver_smem(struct ath10k *ar, const char *fw_build_id)
> +{
> +       u8 *smem_table_ptr;
> +       size_t smem_block_size;
> +       const u32 version_string_size = MAX_BUILD_ID_LEN;

Why not make this size_t as well so the type is the same for the
comparison with smem_block_size?

> +       const u32 smem_img_idx_wlan = ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX * 128;

> +
> +       smem_table_ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> +                                      ATH10K_SMEM_IMAGE_VERSION_TABLE,
> +                                      &smem_block_size);
> +       if (IS_ERR(smem_table_ptr)) {
> +               ath10k_dbg(ar, ATH10K_DBG_QMI, "smem image version table not found");

Is this missing a newline?

> +               return;
> +       }
> +       if (smem_img_idx_wlan + version_string_size > smem_block_size) {
> +               ath10k_dbg(ar, ATH10K_DBG_QMI, "smem block size too small: %zu",

Same newline question.

> +                          smem_block_size);
> +               return;
> +       }
> +       memcpy(smem_table_ptr + smem_img_idx_wlan, fw_build_id,

Is it a string? Does it need to be NUL terminated? Should this use some
sort of strcpy()? Does the comparison above need to leave a space for
the NUL terminator?

> +              version_string_size);
> +}
> +
>  static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi)
>  {
>         struct wlfw_cap_resp_msg_v01 *resp;
Kalle Valo Nov. 8, 2022, 10:30 a.m. UTC | #2 
Youghandhar Chintala <quic_youghand@quicinc.com> wrote:

> In a SoC based solution, it would be useful to know the versions of the
> various binary firmware blobs the system is running on. On a QCOM based
> SoC, this info can be obtained from socinfo debugfs infrastructure. For
> this to work, respective subsystem drivers have to export the firmware
> version information to an SMEM based version information table.
> 
> Having firmware version information at one place will help quickly
> figure out the firmware versions of various subsystems on the device
> instead of going through builds/logs in an event of a system crash.
> 
> Fill WLAN firmware version information in SMEM version table to be
> printed as part of socinfo debugfs infrastructure on a Qualcomm based
> SoC.
> 
> This change is applicable only for WCN399X targets.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> 
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1
> 
> Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

This doesn't compile unless QCOM_SMEM is enabled in Kconfig. So should we add
"select QCOM_SMEM" in Kconfig for ATH10K_SNOC?
Youghandhar Chintala Nov. 8, 2022, 12:04 p.m. UTC | #3 
On 11/8/2022 4:00 PM, Kalle Valo wrote:
> Youghandhar Chintala <quic_youghand@quicinc.com> wrote:
>
>> In a SoC based solution, it would be useful to know the versions of the
>> various binary firmware blobs the system is running on. On a QCOM based
>> SoC, this info can be obtained from socinfo debugfs infrastructure. For
>> this to work, respective subsystem drivers have to export the firmware
>> version information to an SMEM based version information table.
>>
>> Having firmware version information at one place will help quickly
>> figure out the firmware versions of various subsystems on the device
>> instead of going through builds/logs in an event of a system crash.
>>
>> Fill WLAN firmware version information in SMEM version table to be
>> printed as part of socinfo debugfs infrastructure on a Qualcomm based
>> SoC.
>>
>> This change is applicable only for WCN399X targets.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1
>>
>> Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>
>> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> This doesn't compile unless QCOM_SMEM is enabled in Kconfig. So should we add
> "select QCOM_SMEM" in Kconfig for ATH10K_SNOC?

Yes Kalle. Thank you.

Regards,

Youghandhar
Youghandhar Chintala Nov. 11, 2022, 11:37 a.m. UTC | #4 
On 11/7/2022 11:05 PM, Stephen Boyd wrote:
> Quoting Youghandhar Chintala (2022-11-04 01:28:28)
>> In a SoC based solution, it would be useful to know the versions of the
>> various binary firmware blobs the system is running on. On a QCOM based
>> SoC, this info can be obtained from socinfo debugfs infrastructure. For
>> this to work, respective subsystem drivers have to export the firmware
>> version information to an SMEM based version information table.
>>
>> Having firmware version information at one place will help quickly
>> figure out the firmware versions of various subsystems on the device
>> instead of going through builds/logs in an event of a system crash.
>>
>> Fill WLAN firmware version information in SMEM version table to be
>> printed as part of socinfo debugfs infrastructure on a Qualcomm based
>> SoC.
>>
>> This change is applicable only for WCN399X targets.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1
>>
>> Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>
> The trailers go together, no blank lines between them.
>
>> ---
>> Changes from v1:
>>   - Changed print format specifier to %zu from %i
>>   - Changed ath10k_qmi_add_wlan_ver_smem() API argument
>>            to const char *fw_build_id from char *fw_build_id
>>   - Changed version_string_size with MACRO
>> ---
>>   drivers/net/wireless/ath/ath10k/qmi.c | 28 +++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
>> index 66cb7a1e628a..928d78f6d494 100644
>> --- a/drivers/net/wireless/ath/ath10k/qmi.c
>> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/net.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/qcom_scm.h>
>> +#include <linux/soc/qcom/smem.h>
>>   #include <linux/string.h>
>>   #include <net/sock.h>
>>
>> @@ -22,6 +23,8 @@
>>
>>   #define ATH10K_QMI_CLIENT_ID           0x4b4e454c
>>   #define ATH10K_QMI_TIMEOUT             30
>> +#define ATH10K_SMEM_IMAGE_VERSION_TABLE       469
>> +#define ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX     13
>>
>>   static int ath10k_qmi_map_msa_permission(struct ath10k_qmi *qmi,
>>                                           struct ath10k_msa_mem_info *mem_info)
>> @@ -536,6 +539,29 @@ int ath10k_qmi_wlan_disable(struct ath10k *ar)
>>          return ath10k_qmi_mode_send_sync_msg(ar, QMI_WLFW_OFF_V01);
>>   }
>>
>> +static void ath10k_qmi_add_wlan_ver_smem(struct ath10k *ar, const char *fw_build_id)
>> +{
>> +       u8 *smem_table_ptr;
>> +       size_t smem_block_size;
>> +       const u32 version_string_size = MAX_BUILD_ID_LEN;
> Why not make this size_t as well so the type is the same for the
> comparison with smem_block_size?
>
>> +       const u32 smem_img_idx_wlan = ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX * 128;
>> +
>> +       smem_table_ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY,
>> +                                      ATH10K_SMEM_IMAGE_VERSION_TABLE,
>> +                                      &smem_block_size);
>> +       if (IS_ERR(smem_table_ptr)) {
>> +               ath10k_dbg(ar, ATH10K_DBG_QMI, "smem image version table not found");
> Is this missing a newline?
>
>> +               return;
>> +       }
>> +       if (smem_img_idx_wlan + version_string_size > smem_block_size) {
>> +               ath10k_dbg(ar, ATH10K_DBG_QMI, "smem block size too small: %zu",
> Same newline question.
>
>> +                          smem_block_size);
>> +               return;
>> +       }
>> +       memcpy(smem_table_ptr + smem_img_idx_wlan, fw_build_id,
> Is it a string? Does it need to be NUL terminated? Should this use some
> sort of strcpy()? Does the comparison above need to leave a space for
> the NUL terminator?
>
>> +              version_string_size);
>> +}
>> +
>>   static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi)
>>   {
>>          struct wlfw_cap_resp_msg_v01 *resp;


Thank you Stephen.

I will address all your comments in next version of patch.

Regards,

Youghandhar
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
index 66cb7a1e628a..928d78f6d494 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -14,6 +14,7 @@ 
 #include <linux/net.h>
 #include <linux/platform_device.h>
 #include <linux/qcom_scm.h>
+#include <linux/soc/qcom/smem.h>
 #include <linux/string.h>
 #include <net/sock.h>
 
@@ -22,6 +23,8 @@ 
 
 #define ATH10K_QMI_CLIENT_ID		0x4b4e454c
 #define ATH10K_QMI_TIMEOUT		30
+#define ATH10K_SMEM_IMAGE_VERSION_TABLE       469
+#define ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX     13
 
 static int ath10k_qmi_map_msa_permission(struct ath10k_qmi *qmi,
 					 struct ath10k_msa_mem_info *mem_info)
@@ -536,6 +539,29 @@  int ath10k_qmi_wlan_disable(struct ath10k *ar)
 	return ath10k_qmi_mode_send_sync_msg(ar, QMI_WLFW_OFF_V01);
 }
 
+static void ath10k_qmi_add_wlan_ver_smem(struct ath10k *ar, const char *fw_build_id)
+{
+	u8 *smem_table_ptr;
+	size_t smem_block_size;
+	const u32 version_string_size = MAX_BUILD_ID_LEN;
+	const u32 smem_img_idx_wlan = ATH10K_SMEM_IMAGE_TABLE_CNSS_INDEX * 128;
+
+	smem_table_ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY,
+				       ATH10K_SMEM_IMAGE_VERSION_TABLE,
+				       &smem_block_size);
+	if (IS_ERR(smem_table_ptr)) {
+		ath10k_dbg(ar, ATH10K_DBG_QMI, "smem image version table not found");
+		return;
+	}
+	if (smem_img_idx_wlan + version_string_size > smem_block_size) {
+		ath10k_dbg(ar, ATH10K_DBG_QMI, "smem block size too small: %zu",
+			   smem_block_size);
+		return;
+	}
+	memcpy(smem_table_ptr + smem_img_idx_wlan, fw_build_id,
+	       version_string_size);
+}
+
 static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi)
 {
 	struct wlfw_cap_resp_msg_v01 *resp;
@@ -606,6 +632,8 @@  static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi)
 			    qmi->fw_version, qmi->fw_build_timestamp, qmi->fw_build_id);
 	}
 
+	ath10k_qmi_add_wlan_ver_smem(ar, qmi->fw_build_id);
+
 	kfree(resp);
 	return 0;