linux-kernel - Re: [PATCH v4 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl

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: <158681380955.84447.4248338073528400701@swboyd.mtv.corp.google.com>
Date:   Mon, 13 Apr 2020 14:36:49 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Douglas Anderson <dianders@...omium.org>,
        Maulik Shah <mkshah@...eaurora.org>
Cc:     mka@...omium.org, Rajendra Nayak <rnayak@...eaurora.org>,
        evgreen@...omium.org, Lina Iyer <ilina@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 03/10] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller

Quoting Douglas Anderson (2020-04-13 10:04:08)
> I was trying to write documentation for the functions in rpmh-rsc and
> I got to tcs_ctrl_write().  The documentation for the function would
> have been: "This is the core of rpmh_rsc_write_ctrl_data(); all the
> caller does is error-check and then call this".
> 
> Having the error checks in a separate function doesn't help for
> anything since:
> - There are no other callers that need to bypass the error checks.
> - It's less documenting.  When I read tcs_ctrl_write() I kept
>   wondering if I need to handle cases other than ACTIVE_ONLY or cases
>   with more commands than could fit in a TCS.  This is obvious when
>   the error checks and code are together.
> - The function just isn't that long, so there's no problem
>   understanding the combined function.
> 
> Things were even more confusing because the two functions names didn't
> make obvious (at least to me) their relationship.
> 
> Simplify by folding one function into the other.
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> Reviewed-by: Maulik Shah <mkshah@...eaurora.org>
> Tested-by: Maulik Shah <mkshah@...eaurora.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@...omium.org>

> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 439a0eadabf1..d9177324c6a2 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -628,7 +612,18 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>         if (msg->state == RPMH_ACTIVE_ONLY_STATE)
>                 return -EINVAL;
>  
> -       return tcs_ctrl_write(drv, msg);
> +       tcs = get_tcs_for_msg(drv, msg);
> +       if (IS_ERR(tcs))
> +               return PTR_ERR(tcs);
> +
> +       spin_lock_irqsave(&tcs->lock, flags);
> +       /* find the TCS id and the command in the TCS to write to */
> +       ret = find_slots(tcs, msg, &tcs_id, &cmd_id);

It may be better to inline find_slots() too. It's only used here and
that tcs_id = 0, cmd_id = 0 line at the top of this function is
annoying.