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: <1a1a4ac0-388c-8999-47a2-6d6f7471ab93@linux.intel.com>
Date:   Mon, 14 Oct 2019 10:43:54 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        robh@...nel.org, vkoul@...nel.org
Cc:     broonie@...nel.org, bgoswami@...eaurora.org,
        devicetree@...r.kernel.org, lgirdwood@...il.com,
        alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        spapothi@...eaurora.org
Subject: Re: [alsa-devel] [PATCH v3 2/2] soundwire: qcom: add support for
 SoundWire controller



On 10/14/19 4:04 AM, Srinivas Kandagatla wrote:
> Thanks Pierre for taking time to review the patch.
> 
> On 11/10/2019 18:50, Pierre-Louis Bossart wrote:
>>
>>> +static int qcom_swrm_cmd_fifo_wr_cmd(struct qcom_swrm_ctrl *ctrl, u8 
>>> cmd_data,
>>> +                     u8 dev_addr, u16 reg_addr)
>>> +{
>>> +    DECLARE_COMPLETION_ONSTACK(comp);
>>> +    unsigned long flags;
>>> +    u32 val;
>>> +    int ret;
>>> +
>>> +    spin_lock_irqsave(&ctrl->comp_lock, flags);
>>> +    ctrl->comp = &comp;
>>> +    spin_unlock_irqrestore(&ctrl->comp_lock, flags);
>>> +    val = SWRM_REG_VAL_PACK(cmd_data, dev_addr,
>>> +                SWRM_SPECIAL_CMD_ID, reg_addr);
>>> +    ret = ctrl->reg_write(ctrl, SWRM_CMD_FIFO_WR_CMD, val);
>>> +    if (ret)
>>> +        goto err;
>>> +
>>> +    ret = wait_for_completion_timeout(ctrl->comp,
>>> +                      msecs_to_jiffies(TIMEOUT_MS));
>>> +
>>> +    if (!ret)
>>> +        ret = SDW_CMD_IGNORED;
>>> +    else
>>> +        ret = SDW_CMD_OK;
>>
>> It's odd to report CMD_IGNORED on a timeout. CMD_IGNORED is a valid 
>> answer that should be retrieved immediately. You probably need to 
>> translate the soundwire errors into -ETIMEOUT or something.
> 
> In this controller we have no way to know if the command is ignored or 
> timedout, so All the commands that did not receive response either due 
> to ignore or timeout are currently detected with out any response 
> interrupt in given timeout.

ok. It's still very odd. a CMD_IGNORED is a required answer e.g. when 
there is no device0 left to enumerate, when a device has fallen off the 
bus or when accessing a non-implemented register.

>>> +static int qcom_swrm_register_dais(struct qcom_swrm_ctrl *ctrl)
>>> +{
>>> +    int num_dais = ctrl->num_dout_ports + ctrl->num_din_ports;
>>> +    struct snd_soc_dai_driver *dais;
>>> +    struct snd_soc_pcm_stream *stream;
>>> +    struct device *dev = ctrl->dev;
>>> +    int i;
>>> +
>>> +    /* PDM dais are only tested for now */
>>> +    dais = devm_kcalloc(dev, num_dais, sizeof(*dais), GFP_KERNEL);
>>> +    if (!dais)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < num_dais; i++) {
>>> +        dais[i].name = devm_kasprintf(dev, GFP_KERNEL, "SDW Pin%d", i);
>>> +        if (!dais[i].name)
>>> +            return -ENOMEM;
>>> +
>>> +        if (i < ctrl->num_dout_ports) {
>>> +            stream = &dais[i].playback;
>>> +            stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
>>> +                                 "SDW Tx%d", i);
>>> +        } else {
>>> +            stream = &dais[i].capture;
>>> +            stream->stream_name = devm_kasprintf(dev, GFP_KERNEL,
>>> +                                 "SDW Rx%d", i);
>>> +        }
>>
>> For the Intel stuff, we removed the stream_name assignment since it 
>> conflicted with the DAI widgets added by the topology. Since the code 
>> looks inspired by the Intel DAI handling, you should look into this.
> 
> Yes, this code was inspired by Intel's DAI handling, I will take a look 
> a look at latest code and update accordingly.


FWIW, the stream handling seems to have issues as well, specifically the 
'deprepare' part, we are currently working around errors with 
suspend-resume, see e.g. experimental branch at 
https://github.com/thesofproject/linux/pull/1314

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ