On Tue, Feb 13, 2018 at 04:58:19PM +0000, srinivas.kandagatla@linaro.org wrote:
+static struct copp *adm_find_copp(struct q6adm *adm, int port_idx,
int copp_idx)+{
- struct copp *c;
- spin_lock(&adm->copps_list_lock);
- list_for_each_entry(c, &adm->copps_list, node) {
if ((port_idx == c->afe_port) && (copp_idx == c->copp_idx)) {spin_unlock(&adm->copps_list_lock);return c;}- }
- spin_unlock(&adm->copps_list_lock);
We've again got this use of spinlocks here but no IRQ safety - what exactly is going on with the locking? In general all of the locking in this stuff is raising very serious alarm bells with me, I don't understand what is being protected against what and there's some very obvious bugs. We could probably use some documentation about what the locking is supposed to be doing.
- case ADM_CMDRSP_DEVICE_OPEN_V5: {
copp->id = open->copp_id;wake_up(&copp->wait);- }
- break;
- default:
This indentation is confusing.
+static struct copp *adm_find_matching_copp(struct q6adm *adm,
int port_id, int topology,int mode, int rate, int channel_mode,int bit_width, int app_type)+{
- struct copp *c;
- spin_lock(&adm->copps_list_lock);
- list_for_each_entry(c, &adm->copps_list, node) {
if ((port_id == c->afe_port) && (topology == c->topology) &&(mode == c->mode) && (rate == c->rate) &&(bit_width == c->bit_width) && (app_type == c->app_type)) {spin_unlock(&adm->copps_list_lock);return c;}- }
- spin_unlock(&adm->copps_list_lock);
- c = adm_alloc_copp(adm, port_id);
So really this is a find or allocate operation...
- if (IS_ERR_OR_NULL(c))
return ERR_CAST(c);- mutex_lock(&c->lock);
- c->refcnt = 0;
Why do we need to lock the thing we just allocated but didn't yet initialize, and surely if something can find it before we finished initializing we have a race condition?
- copp = adm_find_matching_copp(adm, port_id, topology, perf_mode,
rate, channel_mode, bit_width, app_type);- /* Create a COPP if port id are not enabled */
- if (copp->refcnt == 0) {
ret = q6adm_device_open(adm, copp, port_id, path, topology,channel_mode, bit_width, rate);if (ret < 0)return ret;- }
- mutex_lock(&copp->lock);
- copp->refcnt++;
- mutex_unlock(&copp->lock);
There's an obvious race here between checking the reference count and incrementing it - something might drop a reference before we increment it which would be bad. I'm also not clear when we'd want multiple things using a single COPP.
- mutex_lock(&copp->lock);
- copp->refcnt--;
- mutex_unlock(&copp->lock);
- if (!copp->refcnt) {
This locking is also broken.