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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bb87bd34-ab81-e297-3bd4-3e52f6859e34@linaro.org>
Date:   Tue, 6 Mar 2018 09:26:42 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Mark Brown <broonie@...nel.org>
Cc:     andy.gross@...aro.org, linux-arm-msm@...r.kernel.org,
        alsa-devel@...a-project.org, david.brown@...aro.org,
        robh+dt@...nel.org, mark.rutland@....com, lgirdwood@...il.com,
        plai@...eaurora.org, bgoswami@...eaurora.org, perex@...ex.cz,
        tiwai@...e.com, linux-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, rohkumar@....qualcomm.com,
        spatakok@....qualcomm.com
Subject: Re: [PATCH v3 07/25] ASoC: qcom: qdsp6: Add support to Q6ADM

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