Re: [PATCH v5 2/2] ath11k: add read variant from SMBIOS for download board data

[Wen Gong <quic_wgong@quicinc.com>]

On 3/10/2022 4:12 PM, Kalle Valo wrote:
> Wen Gong <quic_wgong@quicinc.com> writes:
>
>> This is to read variant from SMBIOS such as read from DT, the variant
>> string will be used to one part of string which used to search board
>> data from board-2.bin.
>>
>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
>>
>> Signed-off-by: Wen Gong <quic_wgong@quicinc.com>
> [...]
>
>> +static void ath11k_core_check_bdfext(const struct dmi_header *hdr, void *data)
>> +{
>> +	struct ath11k_base *ab = data;
>> +	const char *bdf_ext;
>> +	const char *magic = ATH11K_SMBIOS_BDF_EXT_MAGIC;
>> +	u8 bdf_enabled;
>> +	int i;
>> +	size_t len;
>> +
>> +	if (ab->qmi.target.bdf_ext[0] != '\0')
>> +		return;
>> +
>> +	if (hdr->type != ATH11K_SMBIOS_BDF_EXT_TYPE)
>> +		return;
>> +
>> +	if (hdr->length != ATH11K_SMBIOS_BDF_EXT_LENGTH) {
>> +		ath11k_dbg(ab, ATH11K_DBG_BOOT,
>> +			   "wrong smbios bdf ext type length (%d).\n",
>> +			   hdr->length);
>> +		return;
>> +	}
>> +
>> +	bdf_enabled = *((u8 *)hdr + ATH11K_SMBIOS_BDF_EXT_OFFSET);
>> +	if (!bdf_enabled) {
>> +		ath11k_dbg(ab, ATH11K_DBG_BOOT, "bdf variant name not found.\n");
>> +		return;
>> +	}
>> +
>> +	/* Only one string exists (per spec) */
>> +	bdf_ext = (char *)hdr + hdr->length;
> A proper struct is preferred over pointer arithmetic. For example
> something like this:
>
> struct ath11k_smbios_bdf {
>          struct dmi_header hdr;
>          u32 padding;
>          u8 bdf_enabled;
>          u8 bdf_ext[ATH11K_SMBIOS_BDF_EXT_MAX_LEN];
> }
>
> I'm not sure if I got the offsets right, but I hope you get the idea
> anyway.
Will change it.
>> +
>> +	if (memcmp(bdf_ext, magic, strlen(magic)) != 0) {
>> +		ath11k_dbg(ab, ATH11K_DBG_BOOT,
>> +			   "bdf variant magic does not match.\n");
>> +		return;
>> +	}
>> +
>> +	len = strlen(bdf_ext);
> What if bdf_ext is not null terminated? Wouldn't strnlen() with
> ATH11K_SMBIOS_BDF_EXT_MAX_LEN would be safer?
Yes, will change it.
>
>> --- a/drivers/net/wireless/ath/ath11k/core.h
>> +++ b/drivers/net/wireless/ath/ath11k/core.h
>> @@ -971,7 +971,18 @@ int ath11k_core_fetch_bdf(struct ath11k_base *ath11k,
>>   			  struct ath11k_board_data *bd);
>>   void ath11k_core_free_bdf(struct ath11k_base *ab, struct ath11k_board_data *bd);
>>   int ath11k_core_check_dt(struct ath11k_base *ath11k);
>> +/* SMBIOS type containing Board Data File Name Extension */
>> +#define ATH11K_SMBIOS_BDF_EXT_TYPE 0xF8
>>   
>> +/* SMBIOS type structure length (excluding strings-set) */
>> +#define ATH11K_SMBIOS_BDF_EXT_LENGTH 0x9
>> +
>> +/* Offset pointing to Board Data File Name Extension */
>> +#define ATH11K_SMBIOS_BDF_EXT_OFFSET 0x8
>> +
>> +/* The magic used by QCA spec */
>> +#define ATH11K_SMBIOS_BDF_EXT_MAGIC "BDF_"
>> +int ath11k_core_check_smbios(struct ath11k_base *ab);
>>   void ath11k_core_halt(struct ath11k *ar);
>>   int ath11k_core_resume(struct ath11k_base *ab);
>>   int ath11k_core_suspend(struct ath11k_base *ab);
> Please don't mix defines and function declarations, so move defines up
> in the file.
Yes, will change it.

-- 
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k