Re: [PATCH v2 06/16] ASoC: qcom: audioreach: add q6apm support

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
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
Subject: Re: [PATCH v2 06/16] ASoC: qcom: audioreach: add q6apm support
Date: Thu, 15 Jul 2021 11:31:54 +0100	[thread overview]
Message-ID: <d52fa4c9-d3c7-7501-0bcb-2c7ff997aa55@linaro.org> (raw)
In-Reply-To: <77b9d17d-33b6-8a7a-bb1d-e32543d9da79@linux.intel.com>

Thanks Pierre for review,

On 14/07/2021 17:40, Pierre-Louis Bossart wrote:
> 
>>   /* SubGraph Config */
>> @@ -32,7 +33,7 @@ struct apm_sub_graph_params  {
>>   /* container config */
>>   struct apm_container_obj  {
>>   	struct apm_container_cfg container_cfg;
>> -	/* Capablity ID list */
>> +	/* Capability ID list */
> 
> squash in wrong patch, this should have been included in the previous patch.

My bad.. will fix such instances in next spin.

> 
>>   	struct apm_prop_data cap_data;
>>   	uint32_t num_capablity_id;

...

> 
>> +static struct audioreach_graph *q6apm_get_audioreach_graph(struct q6apm *apm,
>> +						      uint32_t graph_id)
>> +{
>> +	struct audioreach_graph *graph = NULL;
> 
> useless init
> 
>> +	struct audioreach_graph_info *info;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&apm->lock, flags);
>> +	graph = idr_find(&apm->graph_idr, graph_id);
>> +	spin_unlock_irqrestore(&apm->lock, flags);
>> +
>> +	if (graph) {
>> +		kref_get(&graph->refcount);
>> +		return graph;
>> +	}
>> +
>> +	info = idr_find(&apm->graph_info_idr, graph_id);
>> +
>> +	if (!info)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	graph = kzalloc(sizeof(*graph), GFP_KERNEL);
>> +	if (!graph)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	graph->apm = apm;
>> +	graph->info = info;
>> +	graph->id = graph_id;
>> +
>> +	/* Assuming Linear Graphs only for now! */
>> +	graph->graph = audioreach_alloc_graph_pkt(apm, &info->sg_list, graph_id);
>> +	if (IS_ERR(graph->graph))
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	spin_lock(&apm->lock);
>> +	idr_alloc(&apm->graph_idr, graph, graph_id,
>> +		  graph_id + 1, GFP_ATOMIC);
> 
> ATOMIC?
we are under spinlock here, so we need this.

> 
>> +	spin_unlock(&apm->lock);
>> +
>> +	kref_init(&graph->refcount);
>> +
>> +	q6apm_send_cmd_sync(apm, graph->graph, 0);
>> +
>> +	return graph;
>> +}
>> +
>> +static int audioreach_graph_mgmt_cmd(struct audioreach_graph *graph,
>> +				      uint32_t opcode)
>> +{
>> +	struct gpr_pkt *pkt;
>> +	void *p;
>> +	int i = 0, rc, payload_size;
>> +	struct q6apm *apm = graph->apm;
>> +	struct audioreach_graph_info *info = graph->info;
>> +	int num_sub_graphs = info->num_sub_graphs;
>> +	struct apm_graph_mgmt_cmd *mgmt_cmd;
>> +	struct apm_module_param_data *param_data;
>> +	struct audioreach_sub_graph *sg;
>> +
>> +	payload_size = APM_GRAPH_MGMT_PSIZE(num_sub_graphs);
>> +
>> +	p = audioreach_alloc_apm_cmd_pkt(payload_size, opcode, 0);
>> +	if (IS_ERR(p))
>> +		return -ENOMEM;
>> +
>> +	pkt = p;
>> +	p = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
>> +
>> +	mgmt_cmd = p;
>> +	mgmt_cmd->num_sub_graphs = num_sub_graphs;
>> +
>> +	param_data = &mgmt_cmd->param_data;
>> +	param_data->module_instance_id = APM_MODULE_INSTANCE_ID;
>> +	param_data->param_id = APM_PARAM_ID_SUB_GRAPH_LIST;
>> +	param_data->param_size = payload_size - APM_MODULE_PARAM_DATA_SIZE;
>> +
>> +	list_for_each_entry(sg, &info->sg_list, node) {
>> +		mgmt_cmd->sub_graph_id_list[i++] = sg->sub_graph_id;
>> +	}
>> +
>> +	rc = q6apm_send_cmd_sync(apm, pkt, 0);
>> +
>> +	kfree(pkt);
>> +
>> +	return rc;
>> +}
>> +
>> +static void q6apm_put_audioreach_graph(struct kref *ref)
>> +{
>> +	struct audioreach_graph *graph;
>> +	struct q6apm *apm;
>> +	unsigned long flags;
>> +
>> +	graph = container_of(ref, struct audioreach_graph, refcount);
>> +	apm = graph->apm;
>> +
>> +	audioreach_graph_mgmt_cmd(graph, APM_CMD_GRAPH_CLOSE);
>> +
>> +	spin_lock_irqsave(&apm->lock, flags);
>> +	graph = idr_remove(&apm->graph_idr, graph->id);
>> +	spin_unlock_irqrestore(&apm->lock, flags);
>> +
>> +	kfree(graph->graph);
>> +	kfree(graph);
> 
> earlier in the _get routine, you had a kref_get(&graph->refcount)
> 
This is a release callback for kref. Probably I should rename this to 
q6apm_release_audioreach_graph().

> is it intentional that there's kref_put?

kref_put is called in q6apm_graph_close()

q6apm_graph_open() calls q6apm_get_audioreach_graph() which will do 
kref_get.

q6apm_graph_close() will call kref_put().

when refcount is zero q6apm_put_audioreach_graph() callback will be invoked.

--srini
> 
> adding a comment on the refcounts might be useful...
> 