Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <bb2a64ce-33f9-154c-faad-c5fbc70cd91c@quicinc.com>
Date:   Mon, 14 Mar 2022 18:35:12 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH v5 2/2] ath11k: add read variant from SMBIOS for download
 board data
Content-Language: en-US
To:     Kalle Valo <kvalo@kernel.org>
CC:     <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20211220064829.17557-1-quic_wgong@quicinc.com>
 <20211220064829.17557-3-quic_wgong@quicinc.com> <87wnh2ql8n.fsf@kernel.org>
From:   Wen Gong <quic_wgong@quicinc.com>
In-Reply-To: <87wnh2ql8n.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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.