diff mbox series

[v3,08/10] drivers: qcom: rpmh-rsc: Don't double-check rpmh

Message ID 20200407164915.v3.8.I8e187cdfb7a31f5bb7724f1f937f2862ee464a35@changeid (mailing list archive)
State Superseded
Headers show
Series drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand

Commit Message

Doug Anderson April 7, 2020, 11:50 p.m. UTC
The calls rpmh_rsc_write_ctrl_data() and rpmh_rsc_send_data() are only
ever called from rpmh.c.  We know that rpmh.c already error checked
the message.  There's no reason to do it again in rpmh-rsc.

Suggested-by: Maulik Shah <mkshah@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- ("Don't double-check rpmh") replaces ("Warning if tcs_write...")

Changes in v2: None

 drivers/soc/qcom/rpmh-rsc.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

Comments

Maulik Shah April 8, 2020, 12:23 p.m. UTC | #1
Hi,

In rpmh.c, rpmh_write_async() and rpmh_write_batch() uses 
__fill_rpmh_msg() which already checks for below payload conditions.

so i am ok to remove duplicate checks from rpmh-rsc.c

can you please add payload at the end of subject.

drivers: qcom: rpmh-rsc: Don't double-check rpmh payload

Other than this.

Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
Tested-by: Maulik Shah <mkshah@codeaurora.org>

Note:

rpmh_write() is not using __fill_rpmh_msg() and have replica as below, 
probably since it was declares message on stack instead of using malloc()

         if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
                 return -EINVAL;

         memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
         rpm_msg.msg.num_cmds = n;

Making a note to remove above if check and start using __fill_rpmh_msg() 
here as well to do memcpy() and num_cmds initilization.

Although it may end up writing msg.state and msg.cmd twice (once during 
defining msg on stack and then during fill rpmh msg) but it should be ok.

Below two lines from __rpmh_write() can be removed as well.

         rpm_msg->msg.state = state;

DEFINE_RPMH_MSG_ONSTACK() and __fill_rpmh_msg() seems taking care of 
initializing msg.state already, so we should be good.

if you are spinning a new version and want to include above change as 
well, i am ok.

if not, i can push separate patch to update this as well once my series 
to invoke rpmh_flush() gets picked up.

Thanks,
Maulik

On 4/8/2020 5:20 AM, Douglas Anderson wrote:
> The calls rpmh_rsc_write_ctrl_data() and rpmh_rsc_send_data() are only
> ever called from rpmh.c.  We know that rpmh.c already error checked
> the message.  There's no reason to do it again in rpmh-rsc.
>
> Suggested-by: Maulik Shah <mkshah@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v3:
> - ("Don't double-check rpmh") replaces ("Warning if tcs_write...")
>
> Changes in v2: None
>
>   drivers/soc/qcom/rpmh-rsc.c | 18 +-----------------
>   1 file changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 9502e7ea96be..10c026b2e1bc 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -633,7 +633,7 @@ static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
>   }
>   
>   /**
> - * rpmh_rsc_send_data() - Validate the incoming message + write to TCS block.
> + * rpmh_rsc_send_data() - Write / trigger active-only message.
>    * @drv: The controller.
>    * @msg: The data to be sent.
>    *
> @@ -658,12 +658,6 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   {
>   	int ret;
>   
> -	if (!msg || !msg->cmds || !msg->num_cmds ||
> -	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
> -		WARN_ON(1);
> -		return -EINVAL;
> -	}
> -
>   	do {
>   		ret = tcs_write(drv, msg);
>   		if (ret == -EBUSY) {
> @@ -734,16 +728,6 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>   	unsigned long flags;
>   	int ret;
>   
> -	if (!msg || !msg->cmds || !msg->num_cmds ||
> -	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
> -		pr_err("Payload error\n");
> -		return -EINVAL;
> -	}
> -
> -	/* Data sent to this API will not be sent immediately */
> -	if (msg->state == RPMH_ACTIVE_ONLY_STATE)
> -		return -EINVAL;
> -
>   	tcs = get_tcs_for_msg(drv, msg);
>   	if (IS_ERR(tcs))
>   		return PTR_ERR(tcs);
Doug Anderson April 9, 2020, 12:10 a.m. UTC | #2
Hi,

On Wed, Apr 8, 2020 at 5:24 AM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> Hi,
>
> In rpmh.c, rpmh_write_async() and rpmh_write_batch() uses
> __fill_rpmh_msg() which already checks for below payload conditions.
>
> so i am ok to remove duplicate checks from rpmh-rsc.c
>
> can you please add payload at the end of subject.
>
> drivers: qcom: rpmh-rsc: Don't double-check rpmh payload
>
> Other than this.
>
> Reviewed-by: Maulik Shah <mkshah@codeaurora.org>
> Tested-by: Maulik Shah <mkshah@codeaurora.org>

Thanks!  Bjorn / Andy: if you want me to spin my series I'm happy to.
I'm also happy to just let you fix this nit in the commit message and
the other one Maulik had when applying.  Just let me know.


> Note:
>
> rpmh_write() is not using __fill_rpmh_msg() and have replica as below,
> probably since it was declares message on stack instead of using malloc()
>
>          if (!cmd || !n || n > MAX_RPMH_PAYLOAD)
>                  return -EINVAL;
>
>          memcpy(rpm_msg.cmd, cmd, n * sizeof(*cmd));
>          rpm_msg.msg.num_cmds = n;
>
> Making a note to remove above if check and start using __fill_rpmh_msg()
> here as well to do memcpy() and num_cmds initilization.
>
> Although it may end up writing msg.state and msg.cmd twice (once during
> defining msg on stack and then during fill rpmh msg) but it should be ok.
>
> Below two lines from __rpmh_write() can be removed as well.
>
>          rpm_msg->msg.state = state;
>
> DEFINE_RPMH_MSG_ONSTACK() and __fill_rpmh_msg() seems taking care of
> initializing msg.state already, so we should be good.
>
> if you are spinning a new version and want to include above change as
> well, i am ok.
>
> if not, i can push separate patch to update this as well once my series
> to invoke rpmh_flush() gets picked up.

I'd sorta be inclined to wait and do this later just because it seems
like we've got enough changes stacked together right now...

-Doug
diff mbox series

Patch

diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 9502e7ea96be..10c026b2e1bc 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -633,7 +633,7 @@  static int tcs_write(struct rsc_drv *drv, const struct tcs_request *msg)
 }
 
 /**
- * rpmh_rsc_send_data() - Validate the incoming message + write to TCS block.
+ * rpmh_rsc_send_data() - Write / trigger active-only message.
  * @drv: The controller.
  * @msg: The data to be sent.
  *
@@ -658,12 +658,6 @@  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
 {
 	int ret;
 
-	if (!msg || !msg->cmds || !msg->num_cmds ||
-	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
-		WARN_ON(1);
-		return -EINVAL;
-	}
-
 	do {
 		ret = tcs_write(drv, msg);
 		if (ret == -EBUSY) {
@@ -734,16 +728,6 @@  int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
 	unsigned long flags;
 	int ret;
 
-	if (!msg || !msg->cmds || !msg->num_cmds ||
-	    msg->num_cmds > MAX_RPMH_PAYLOAD) {
-		pr_err("Payload error\n");
-		return -EINVAL;
-	}
-
-	/* Data sent to this API will not be sent immediately */
-	if (msg->state == RPMH_ACTIVE_ONLY_STATE)
-		return -EINVAL;
-
 	tcs = get_tcs_for_msg(drv, msg);
 	if (IS_ERR(tcs))
 		return PTR_ERR(tcs);