linux-kernel - Re: [PATCH v4 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots 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]
Message-ID: <20200414052851.GI20625@builder.lan>
Date:   Mon, 13 Apr 2020 22:28:51 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Maulik Shah <mkshah@...eaurora.org>, swboyd@...omium.org,
        mka@...omium.org, Rajendra Nayak <rnayak@...eaurora.org>,
        evgreen@...omium.org, Lina Iyer <ilina@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 00/10] drivers: qcom: rpmh-rsc: Cleanup / add lots of
 comments

On Mon 13 Apr 10:04 PDT 2020, Douglas Anderson wrote:

> In order to review Maulik's latest "rpmh_flush for non OSI targets"
> patch series I've found myself trying to understand rpmh-rsc better.
> To make it easier for others to do this in the future, add a whole lot
> of comments / documentation.
> 
> As part of this there are a very small number of functional changes.
> - We'll get a tiny performance boost by getting rid of the "cmd_cache"
>   which I believe was unnecessary.
> - We now assume someone else is in charge of exclusivity for
>   tcs_invalidate() and have removed a lock in there. As per the
>   comments in the patch, this isn't expected to cause problems.
> - tcs_is_free() no longer checks hardware state, but we think it
>   didn't need to.
> 
> These changes touch a lot of code in rpmh-rsc.  Luckily Maulik has
> reported that he's tested them and they work fine for him.  They
> should be ready to go.
> 
> I've tried to structure the patches so that simpler / less
> controversial patches are first. Those could certainly land on their
> own without later patches. Many of the patches could also be dropped
> and the others would still apply if they are controversial.  If you
> need help doing this then please yell.
> 
> These patches are based on Maulik's v17 series, AKA:
> https://lore.kernel.org/r/1586703004-13674-1-git-send-email-mkshah@codeaurora.org
> 
> There are still more cleanups that we need to do, but to avoid having
> too many patches flying through the air at once we'll do them after
> Maulik's v17 and this series lands.
> 
> With all that, enjoy.
> 

Applied the series, with the typos pointed out by Stephen fixed.
Please follow up on the additional suggestions with incremental patches.

Regards,
Bjorn

> Changes in v4:
> - Add "payload" to end of ("Don't double-check rpmh") patch subject
> - Removed extra "make sure" in commit message.
> 
> Changes in v3:
> - ("...are not for IRQ") is new for v3.
> - ("Don't double-check rpmh") replaces ("Warning if tcs_write...")
> - Add "TCS" in title (Maulik).
> - Adjusted comments for rpmh_rsc_write_ctrl_data().
> - Comments for new enable_tcs_irq() function.
> - Comments for new rpmh_rsc_cpu_pm_callback() function.
> - Extra blank line removed (Maulik).
> - IRQ registers aren't in TCS0 (Maulik).
> - Kill find_match moves from patch #9 to patch #5 (Maulik).
> - Mention in message that I also fixed up kernel-doc stuff.
> - Moved comments patch after ("Kill cmd_cache and find_match...").
> - One space after a period now (Maulik).
> - Plural of TCS fixed to TCSes following Maulik's example.
> - Re-added comment in tcs_write() about checking for same address.
> - Rebased atop v16 ('Invoke rpmh_flush...') series.
> - Replace ("...warn if state mismatch") w/ ("...just check tcs_in_use")
> - Replaced ("irqsave()...") + ("...never -EBUSY") w/ ("Caller handles...")
> - Rewrote commit message to adjust for patch order.
> - __tcs_set_trigger() comments adjusted now that it can set or unset.
> - get_tcs_for_msg() documents why it's safe to borrow the wake TCS.
> - get_tcs_for_msg() no longer returns -EAGAIN.
> 
> Changes in v2:
> - Comment tcs_is_free() new for v2; replaces old patch 6.
> - Document bug of tcs_write() not handling -EAGAIN.
> - Document get_tcs_for_msg() => -EAGAIN only for ACTIVE_ONLY.
> - Document locks for updating "tcs_in_use" more.
> - Document tcs_is_free() without drv->lock OK for tcs_invalidate().
> - Document that rpmh_rsc_send_data() can be an implicit invalidate.
> - Document two get_tcs_for_msg() issues if zero-active TCS.
> - Fixed documentation of "tcs" param in find_slots().
> - Got rid of useless "if (x) continue" at end of for loop.
> - More clear that active-only xfers can happen on wake TCS sometimes.
> - Now prose in comments instead of struct definitions.
> - Pretty ASCII art from Stephen.
> - Reword tcs_write() doc a bit.
> 
> Douglas Anderson (10):
>   drivers: qcom: rpmh-rsc: Clean code reading/writing TCS regs/cmds
>   drivers: qcom: rpmh-rsc: Document the register layout better
>   drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
>   drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction
>   drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
>   drivers: qcom: rpmh-rsc: A lot of comments
>   drivers: qcom: rpmh-rsc: tcs_is_free() can just check tcs_in_use
>   drivers: qcom: rpmh-rsc: Don't double-check rpmh payload
>   drivers: qcom: rpmh-rsc: Caller handles tcs_invalidate() exclusivity
>   drivers: qcom: rpmh-rsc: read_tcs_reg()/write_tcs_reg() are not for
>     IRQ
> 
>  drivers/soc/qcom/rpmh-internal.h |  66 +++--
>  drivers/soc/qcom/rpmh-rsc.c      | 465 +++++++++++++++++++++----------
>  drivers/soc/qcom/rpmh.c          |   5 +-
>  3 files changed, 363 insertions(+), 173 deletions(-)
> 
> -- 
> 2.26.0.110.g2183baf09c-goog
>