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

[Srinivas Kandagatla]

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...