linux-kernel - Re: [PATCH v9 04/10] drivers: qcom: rpmh: add RPMH helper functions

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:   Wed, 30 May 2018 14:49:31 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Raju P L S S S N <rplsssn@...eaurora.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        linux-arm-msm@...r.kernel.org,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Evan Green <evgreen@...omium.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Lina Iyer <ilina@...eaurora.org>
Subject: Re: [PATCH v9 04/10] drivers: qcom: rpmh: add RPMH helper functions

Hi,

On Thu, May 24, 2018 at 3:45 AM, Raju P L S S S N
<rplsssn@...eaurora.org> wrote:
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -61,6 +61,8 @@
>  #define CMD_STATUS_ISSUED              BIT(8)
>  #define CMD_STATUS_COMPL               BIT(16)
>
> +LIST_HEAD(rsc_drv_list);

I still see no point of rsc_drv_list.  Please remove it, AKA squash in
<http://crosreview.com/1042883>.

I'm also still of the opinion that we should take something like
<http://crosreview.com/1054646>, AKA "Get rid of the global array
rpmh_rsc".

> +/**
> + * __rpmh_write: send the RPMH request
> + *
> + * @dev: The device making the request
> + * @state: Active/Sleep request type
> + * @rpm_msg: The data that needs to be sent (cmds).
> + */
> +static int __rpmh_write(const struct device *dev, enum rpmh_state state,
> +                       struct rpmh_request *rpm_msg)
> +{
> +       struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
> +
> +       if (IS_ERR(ctrlr))
> +               return PTR_ERR(ctrlr);
> +
> +       rpm_msg->msg.state = state;
> +
> +       if (state != RPMH_ACTIVE_ONLY_STATE)
> +               return -EINVAL;
> +
> +       WARN_ON(irqs_disabled());
> +
> +       return rpmh_rsc_send_data(ctrlr->drv, &rpm_msg->msg);
> +}

You went too far in the removal of EXPORT_SYMBOL I think.  This symbol
needs to be exported because other code that could be compiled as a
module might need to call into it.  To explain:

* If two files that are always built-in to Linux need to call into
each other: no need for EXPORT_SYMBOL.

* If two files that are always part of the same module need to call
into each other: no need for EXPORT_SYMBOL.

* If one file that might be built-into a module needs to call another
that's builtin to the kernel: need EXPORT_SYMBOL.

-Doug