linux-kernel - Re: [PATCH v4 06/10] drivers: qcom: rpmh-rsc: A lot of comments

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, 13 Apr 2020 14:58:32 -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 06/10] drivers: qcom: rpmh-rsc: A lot of comments

Quoting Douglas Anderson (2020-04-13 10:04:11)
> I've been pouring through the rpmh-rsc code and trying to understand
> it.  Document everything to the best of my ability.  All documentation
> here is strictly from code analysis--no actual knowledge of the
> hardware was used.  If something is wrong in here I either
> misunderstood the code, had a typo, or the code has a bug in it
> leading to my incorrect understanding.
> 
> In a few places here I have documented things that don't make tons of
> sense.  A future patch will try to address this.  While this means I'm
> adding comments / todos and then later fixing them in the series, it
> seemed more urgent to get things documented first so that people could
> understand the later patches.
> 
> Any comments I adjusted I also tried to make match kernel-doc better.
> Specifically:
> - kernel-doc says do not leave a blank line between the function
>   description and the arguments
> - kernel-doc examples always have things starting w/ a capital and
>   ending with a period.
> 
> This should be a no-op.  It's just comment changes.
> 
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> Reviewed-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 c9e5cddbc099..f0a7ada0c16f 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -171,12 +171,38 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
>         }
>  }
>  
> +/**
> + * tcs_is_free() - Return if a TCS is totally free.
> + * @drv:    The RSC controller.
> + * @tcs_id: The global ID of this TCS.
> + *
> + * Returns true if nobody has claimed this TCS (by setting tcs_in_use).
> + * If the TCS looks free, checks that the hardware agrees.
> + *
> + * Must be called with the drv->lock held or the tcs_lock for the TCS being

I think we have 'Context:' for these sorts of things.

> + * tested. If only the tcs_lock is held then it is possible that this
> + * function will return that a tcs is still busy when it has been recently
> + * been freed but it will never return free when a TCS is actually in use.
> + *
> + * Return: true if the given TCS is free.
> + */
>  static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
>  {
>         return !test_bit(tcs_id, drv->tcs_in_use) &&
>                read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
>  }
>  
> +/**
> + * tcs_invalidate() - Invalidate all TCSes of the given type (sleep or wake).
> + * @drv:  The RSC controller.
> + * @type: SLEEP_TCS or WAKE_TCS
> + *
> + * This will clear the "slots" variable of the given tcs_group and also
> + * tell the hardware to forget about all entries.
> + *
> + * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
> + *         bit. Caller should make sure to enable interrupts between tries.
> + */
>  static int tcs_invalidate(struct rsc_drv *drv, int type)
>  {
>         int m;
> @@ -624,6 +798,23 @@ static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
>         return false;
>  }
>  
> +/**
> + * rpmh_rsc_cpu_pm_callback() - Check if any of the AMCs are busy.
> + * @nfb:    Pointer to the notifier block in struct rsc_drv.
> + * @action: CPU_PM_ENTER, CPU_PM_ENTER_FAILED, or CPU_PM_EXIT.
> + * @v:      Unused
> + *
> + * This function is given to cpu_pm_register_notifier so we can be informed
> + * about when CPUs go down. When all CPUs go down we know no more active
> + * transfers will be started so we write sleep/wake sets. This function gets
> + * called from cpuidle code paths and also at system suspend time.
> + *
> + * If its last CPU going down and AMCs are not busy then writes cached sleep
> + * and wake messages to TCSes. The firmware then takes care of triggering
> + * them when entering deepest low power modes.
> + *
> + * Return: See cpu_pm_register_notifier.

cpu_pm_register_notifier()

> + */
>  static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
>                                     unsigned long action, void *v)
>  {