linux-kernel - Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for SoundWire controller

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] [day] [month] [year] [list]

Message-ID: <d3ccd866-7bc5-9635-4bb4-6b0765f89835@linaro.org>
Date:   Sat, 15 Jun 2019 14:24:25 +0100
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org, vkoul@...nel.org
Cc:     mark.rutland@....com, devicetree@...r.kernel.org,
        alsa-devel@...a-project.org, robh+dt@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for
 SoundWire controller



On 11/06/2019 13:21, Pierre-Louis Bossart wrote:
> 
> 
> On 6/11/19 5:29 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 10/06/2019 15:12, Pierre-Louis Bossart wrote:
>>>>>> +
>>>>>> +    if (dev_addr == SDW_BROADCAST_DEV_NUM) {
>>>>>> +        ctrl->fifo_status = 0;
>>>>>> +        ret = wait_for_completion_timeout(&ctrl->sp_cmd_comp,
>>>>>> +                          msecs_to_jiffies(TIMEOUT_MS));
>>>>>
>>>>> This is odd. The SoundWire spec does not handle writes to a single 
>>>>> device or broadcast writes differently. I don't see a clear reason 
>>>>> why you would only timeout for a broadcast write.
>>>>>
>>>>
>>>> There is danger of blocking here without timeout.
>>>
>>> Right, and it's fine to add a timeout. The question is why add a 
>>> timeout *only* for a broadcast operation? It should be added for 
>>> every transaction IMO, unless you have a reason not to do so.
>>>
>>
>> I did try this before, the issue is when we read/write registers from 
>> interrupt handler, these can be deadlocked as we will be interrupt 
>> handler waiting for another completion interrupt, which will never 
>> happen unless we return from the first interrupt.
> 
> I don't quite get the issue. With the Intel hardware we only deal with 
> Master registers (some of which mirror the bus state) in the handler and 
> will only modify Slave registers in the thread. All changes to Slave 
> registers will be subject to a timeout as well as a check for no 
> response or NAK. Not sure what is specific about your solution that 
> requires a different handling of commands depending on which device 
> number is used. It could very well be that you've uncovered a flaw in 
> the bus design but I still don't see how it would be Qualcomm-specific?

Sorry It took bit more time for digging up the issue which I faced 
previously to answer this query. This is now fixed and v2 patchset has 
same handling for all the slave registers read/writes with no special 
casing.

Thanks,
srini