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