Re: [PATCH v3 07/25] ASoC: qcom: qdsp6: Add support to Q6ADM

[Srinivas Kandagatla]

Thanks for the review,

On 01/03/18 21:24, Mark Brown wrote:
> On Tue, Feb 13, 2018 at 04:58:19PM +0000, srinivas.kandagatla@xxxxxxxxxx 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.
I agree, there are locking issues here, Am revisiting them all before I 
send a next version.

> > +	case ADM_CMDRSP_DEVICE_OPEN_V5: {
>
> > +		copp->id = open->copp_id;
> > +		wake_up(&copp->wait);
> > +	}
> > +	break;
> > +	default:
>
> This indentation is confusing.
I agree, will fix such instances in next version.

thanks,
srini