Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 05/16] ASoC: qcom: audioreach: add basic pkt alloc
 support
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        bjorn.andersson@linaro.org, broonie@kernel.org, robh@kernel.org
Cc:     devicetree@vger.kernel.org, alsa-devel@alsa-project.org,
        bgoswami@codeaurora.org, lgirdwood@gmail.com, tiwai@suse.de,
        plai@codeaurora.org, linux-kernel@vger.kernel.org
References: <20210714153039.28373-1-srinivas.kandagatla@linaro.org>
 <20210714153039.28373-6-srinivas.kandagatla@linaro.org>
 <58d6df8d-83ed-c716-81b2-e0927f58da6f@linux.intel.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <e581be78-10cd-12e7-17df-c53c1b680473@linaro.org>
Date:   Thu, 15 Jul 2021 11:31:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <58d6df8d-83ed-c716-81b2-e0927f58da6f@linux.intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks for review Pierre,

On 14/07/2021 17:30, Pierre-Louis Bossart wrote:
> 
> 
> 
>> diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
>> index cc7c1de2f1d9..721ea56b2cb5 100644
>> --- a/sound/soc/qcom/Kconfig
>> +++ b/sound/soc/qcom/Kconfig
>> @@ -103,6 +103,12 @@ config SND_SOC_QDSP6
>>   	 audio drivers. This includes q6asm, q6adm,
>>   	 q6afe interfaces to DSP using apr.
>>   
>> +config SND_SOC_QCOM_AUDIOREACH
>> +	tristate "SoC ALSA audio drives for Qualcomm AUDIOREACH"
> 
> typo: drivers?
> 
Will fix all the typos in next spin.


>> +static void *__audioreach_alloc_pkt(int payload_size, uint32_t opcode,
>> +				     uint32_t token, uint32_t src_port,
>> +				     uint32_t dest_port, bool has_cmd_hdr)
>> +{
>> +	struct apm_cmd_header *cmd_header;
>> +	struct gpr_pkt *pkt;
>> +	void *p;
>> +	int pkt_size = GPR_HDR_SIZE + payload_size;
>> +
>> +	if (has_cmd_hdr)
>> +		pkt_size += APM_CMD_HDR_SIZE;
>> +
>> +	p = kzalloc(pkt_size, GFP_ATOMIC);
> 
> is GFP_ATOMIC required? it's the same question really than my earlier one on spinlock_irqsave, it's rather hard to figure out in what context this code will run.

I had some spinlocks in this patch for compress offload cases, so had to 
make it ATOMIC. having said that we could avoid ATOMIC here.

> 
>> +	if (!p)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pkt = p;
>> +	pkt->hdr.version = GPR_PKT_VER;
>> +	pkt->hdr.hdr_size = 6;
> 
> magic number. looks like a missing macro...

There is already a macro for this, GPR_PKT_HEADER_WORD_SIZE I should 
have used it.
> 
>> +	pkt->hdr.pkt_size = pkt_size;
>> +	pkt->hdr.dest_port = dest_port;
>> +	pkt->hdr.src_port = src_port;
>> +
>> +	pkt->hdr.dest_domain = GPR_DOMAIN_ID_ADSP;
>> +	pkt->hdr.src_domain = GPR_DOMAIN_ID_APPS;
>> +	pkt->hdr.token = token;
>> +	pkt->hdr.opcode = opcode;
>> +
>> +	if (has_cmd_hdr) {
>> +		p = p + GPR_HDR_SIZE;
>> +		cmd_header = p;
>> +		cmd_header->payload_size = payload_size;
>> +	}
>> +
>> +	return pkt;
>> +}
>