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

Message-ID: <7269521a-ac89-3856-c18c-ffaaf64c0806@linux.intel.com>
Date:   Mon, 10 Jun 2019 09:12:39 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        broonie@...nel.org, vkoul@...nel.org
Cc:     mark.rutland@....com, devicetree@...r.kernel.org,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        robh+dt@...nel.org
Subject: Re: [alsa-devel] [RFC PATCH 6/6] soundwire: qcom: add support for
 SoundWire controller


>>> +#define SWRM_COMP_HW_VERSION                    0x00
>>
>> Can we please use SDW_ or QCOM_SDW_ as prefix?
>>
> 
> SWRM prefix is as per the data sheet register names, If it help am happy 
> to add QCOM_ prefix it.

That'd be fine. As long as there is no duplication and two 
terms/prefixes used for the same thing I am happy.

>>> +
>>> +    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr, cmd_id, reg_addr);
>>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>>> +    if (ret < 0) {
>>> +        dev_err(ctrl->dev, "%s: reg 0x%x write failed, err:%d\n",
>>> +            __func__, val, ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    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.

>>
>>> +
>>> +    /* Mask soundwire interrupts */
>>> +    ctrl->reg_write(ctrl, SWRM_INTERRUPT_MASK_ADDR,
>>> +                    SWRM_INTERRUPT_STATUS_RMSK);
>>> +
>>> +    /* Configure No pings */
>>> +    val = ctrl->reg_read(ctrl, SWRM_MCP_CFG_ADDR);
>>
>> If there is any sort of PREQ signaling for Slave-initiated interrupts, 
>> disabling PINGs is likely a non-conformant implementation since the 
>> master is required to issue a PING command within 32 frames. That's 
>> also the only way to know if a device is attached, so additional 
>> comments are likely required.
> This is the value of Maximum number of consiecutive read/write commands 
> without ping command in between. I will try to collect more details and 
> add some comments here.

Right, so it's probably the comment that's too strict. It's not no pings 
it's no pings during a sequence of up to N read/writes.