Re: [PATCH v7 07/13] wifi: ath12k: add support for fixed QMI firmware memory

[Raj Kumar Bhagat]

On 2/28/2025 9:08 PM, Jeff Johnson wrote:
> On 2/27/2025 11:10 AM, Raj Kumar Bhagat wrote:
> ...
>> +static int ath12k_qmi_assign_target_mem_chunk(struct ath12k_base *ab)
>> +{
>> +	struct reserved_mem *rmem;
>> +	phys_addr_t bdf_size;
>> +	int i, idx, ret;
>> +
>> +	for (i = 0, idx = 0; i < ab->qmi.mem_seg_count; i++) {
>> +		switch (ab->qmi.target_mem[i].type) {
>> +		case HOST_DDR_REGION_TYPE:
>> +			rmem = ath12k_core_get_reserved_mem(ab, 0);
>> +			if (!rmem) {
>> +				ret = -ENODEV;
>> +				goto out;
>> +			}
>> +
>> +			if (rmem->size < ab->qmi.target_mem[i].size) {
>> +				ath12k_dbg(ab, ATH12K_DBG_QMI,
>> +					   "failed to assign mem type %d req size %d avail size %lld\n",
>> +					   ab->qmi.target_mem[i].type,
>> +					   ab->qmi.target_mem[i].size,
>> +					   rmem->size);
> 
> The v6 version had a kernel test robot build warning here when building for
> MIPS and it looks like nothing has changed.
> 

The kernel test robot warning came just after I posted the v7. Thus, the v7
version also have the same warning. I will send v8 in sometime to address this.

> I don't know the history of why struct reserved_mem::size is of type
> phys_addr_t, but that type has a different size depending upon architecture,
> therefore you can't use %lld.
> 
> To print it correctly you either need to use the %paa format that is meant for
> that type, or probably better would be to assign it to a variable of type
> size_t and then use %zu (and use that variable in the size test as well)
> 

Sure will use variable to type size_t to address this.

> (also consider if the other %d formats should be %u instead)
> 

will update to %u.

>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +
>> +			ab->qmi.target_mem[idx].paddr = rmem->base;
>> +			ab->qmi.target_mem[idx].v.ioaddr =
>> +				ioremap(ab->qmi.target_mem[idx].paddr,
>> +					ab->qmi.target_mem[i].size);
>> +			if (!ab->qmi.target_mem[idx].v.ioaddr) {
>> +				ret = -EIO;
>> +				goto out;
>> +			}
>> +			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
>> +			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
>> +			idx++;
>> +			break;
>> +		case BDF_MEM_REGION_TYPE:
>> +			rmem = ath12k_core_get_reserved_mem(ab, 0);
>> +			if (!rmem) {
>> +				ret = -ENODEV;
>> +				goto out;
>> +			}
>> +
>> +			bdf_size = rmem->size - ab->hw_params->bdf_addr_offset;
>> +			if (bdf_size < ab->qmi.target_mem[i].size) {
>> +				ath12k_dbg(ab, ATH12K_DBG_QMI,
>> +					   "failed to assign mem type %d req size %d avail size %lld\n",
>> +					   ab->qmi.target_mem[i].type,
>> +					   ab->qmi.target_mem[i].size,
>> +					   bdf_size);
> 
> the same issue exists here.
> again this would be fixed by making bdf_size type size_t and using %zu
> 

Will use a common variable (size_t avail_size) to address this and the above.

>> +				ret = -EINVAL;
>> +				goto out;
>> +			}
>> +			ab->qmi.target_mem[idx].paddr =
>> +				rmem->base + ab->hw_params->bdf_addr_offset;
>> +			ab->qmi.target_mem[idx].v.ioaddr =
>> +				ioremap(ab->qmi.target_mem[idx].paddr,
>> +					ab->qmi.target_mem[i].size);
>> +			if (!ab->qmi.target_mem[idx].v.ioaddr) {
>> +				ret = -EIO;
>> +				goto out;
>> +			}
>> +			ab->qmi.target_mem[idx].size = ab->qmi.target_mem[i].size;
>> +			ab->qmi.target_mem[idx].type = ab->qmi.target_mem[i].type;
>> +			idx++;
>> +			break;