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]
Date:   Thu, 3 Dec 2020 13:44:21 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <agross@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Srinivas Rao L <lsrao@...eaurora.org>,
        Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH v2] soc: qcom: rpmh: Remove serialization of TCS commands

Hi,

On Mon, Nov 23, 2020 at 11:32 PM Maulik Shah <mkshah@...eaurora.org> wrote:
>
> @@ -423,8 +422,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
>                         cmd = &req->cmds[j];
>                         sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
>                         if (!(sts & CMD_STATUS_ISSUED) ||
> -                          ((req->wait_for_compl || cmd->wait) &&
> -                          !(sts & CMD_STATUS_COMPL))) {
> +                          (cmd->wait && !(sts & CMD_STATUS_COMPL))) {
>                                 pr_err("Incomplete request: %s: addr=%#x data=%#x",
>                                        drv->name, cmd->addr, cmd->data);
>                                 err = -EIO;

In my review of v1 all those months ago, the way we left things was
that I disagreed with this part of the patch, and I still do.  I think
you should leave things the way they were in tcs_tx_done().  Copying
my un-responded-to comments from v1 here for you:

In your patch in __tcs_buffer_write(), if "wait_for_compl" is set then
"CMD_MSGID_RESP_REQ" will be added for every message in the request,
right?  That's because you have this bit of code:

/* Convert all commands to RR when the request has wait_for_compl set */
cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;

That means that if _either_ "cmd->wait" or "req->wait_for_compl" is
set then you expect the "sts" to have "CMD_STATUS_COMPL", right?
That's exactly the code that used to be there.

Said another way, if "req->wait_for_compl" was set then it's an error
if any of our commands are missing the "CMD_STATUS_COMPL" bit, right?


> @@ -30,7 +30,7 @@ enum rpmh_state {
>   *
>   * @addr: the address of the resource slv_id:18:16 | offset:0:15
>   * @data: the resource state request
> - * @wait: wait for this request to be complete before sending the next
> + * @wait: ensure that this command is complete before returning

In my response to v1 I suggested that a comment would be nice here.
Something akin to:

Setting "wait" here only makes sense in the batch case for active-only
transfers.

This is because:
* rpmh_write_async() - There's no callback and rpmh_write_async()
doesn't set the "completion" to anything so there's nobody that cares
at all

* DEFINE_RPMH_MSG_ONSTACK - always sets wait_for_compl.

-Doug

Powered by blists - more mailing lists