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
| ||
|
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