linux-kernel - Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver

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:   Mon, 09 Mar 2020 11:19:53 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Maulik Shah <mkshah@...eaurora.org>
Cc:     mka@...omium.org, evgreen@...omium.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        agross@...nel.org, dianders@...omium.org, rnayak@...eaurora.org,
        ilina@...eaurora.org, lsrao@...eaurora.org,
        Mahesh Sivasubramanian <msivasub@...eaurora.org>
Subject: Re: [PATCH v3 2/4] soc: qcom: Add SoC sleep stats driver

Quoting Maulik Shah (2020-03-09 03:58:27)
> 
> On 3/7/2020 12:12 PM, Bjorn Andersson wrote:
> > On Thu 05 Mar 23:23 PST 2020, Maulik Shah wrote:
> >> diff --git a/drivers/soc/qcom/soc_sleep_stats.c b/drivers/soc/qcom/soc_sleep_stats.c
> >> new file mode 100644
> >> index 00000000..79a14d2
> >> --- /dev/null
> >> +++ b/drivers/soc/qcom/soc_sleep_stats.c
> >> @@ -0,0 +1,248 @@
[...]
> >> +    u32 pid;
> >> +};
> >> +
> >> +static struct subsystem_data subsystems[] = {
> >> +    { "modem", MODEM_ITEM_ID, PID_MODEM },
> >> +    { "adsp", ADSP_ITEM_ID, PID_ADSP },
> >> +    { "cdsp", CDSP_ITEM_ID, PID_CDSP },
> >> +    { "slpi", SLPI_ITEM_ID, PID_SLPI },
> >> +    { "gpu", GPU_ITEM_ID, PID_APSS },
> >> +    { "display", DISPLAY_ITEM_ID, PID_APSS },
> >> +};
> >> +
> >> +struct stats_config {
> >> +    unsigned int offset_addr;
> >> +    unsigned int num_records;
> >> +    bool appended_stats_avail;
> >> +};
> >> +
> >> +static const struct stats_config *config;
> > Add this to soc_sleep_stats_data and pass that as s->private instead of
> > just the reg, to avoid the global variable.
> 
> No, this is required to keep global. we are not passing just reg as s->private,
> we are passing "reg + offset" which differs for each stat.

Can you please stop sending these review comment replies and then
immediately turning around and sending the next revision of the patch
series. I doubt that the changes take less than an hour to write and it
would be helpful for everyone involved to have constructive discussions
about the code if there's ever a response besides "done" or "ok".

In this case it should be possible to get rid of the global 'config'.
Make a new container struct to hold the config and offset or figure out
what actually needs to be passed into the functions instead and do that
when the files are created.