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] [day] [month] [year] [list]

Message-ID: <ad874421-f197-174f-3d30-2037c7fb57a0@codeaurora.org>
Date:   Tue, 10 Mar 2020 21:08:41 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Stephen Boyd <swboyd@...omium.org>,
        Bjorn Andersson <bjorn.andersson@...aro.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


On 3/9/2020 11:49 PM, Stephen Boyd wrote:
> 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".
Sure, i be careful to wait for at least few days before spinning new one,
atleast wait till all discussion finish on current version.

Also will try to reply in more details for comments as much as possible.
>
> 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.

i will address this to get rid of global config.

Thanks,
Maulik

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation