linux-kernel - Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update

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: <20230320041019.5qs6qbztvv45pacs@ripper>
Date:   Sun, 19 Mar 2023 21:10:19 -0700
From:   Bjorn Andersson <andersson@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Mukesh Ojha <quic_mojha@...cinc.com>, agross@...nel.org,
        konrad.dybcio@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v3 2/5] pinctrl: qcom: Use qcom_scm_io_update_field()

On Fri, Mar 17, 2023 at 09:58:04PM +0100, Linus Walleij wrote:
> On Fri, Mar 17, 2023 at 5:28 PM Mukesh Ojha <quic_mojha@...cinc.com> wrote:
> 
> > Use qcom_scm_io_update_field() exported function introduced
> > in last commit.
> >
> > Signed-off-by: Mukesh Ojha <quic_mojha@...cinc.com>
> 
> Fine by me, but I want you to first consider switching the
> custom register accessors to regmap.
> 

I took a quick look at it and there seem to be two ways that it can be
done.

We can retain the MSM_ACCESSOR() macros that generates the custom
register accessors, but plug in a regmap between these accessors and the
mmio operations. But this just adds a few extra hops inbetween the
driver and the volatile read/write, with a slight increase of memory,
without any obvious benefits.

The more alluring alternative is to replace the custom accessors with
reg_fields. This would allow us to replace some (perhaps many) of the
bit-manipulation with regmap_update_bits().

But at minimum we'd need one reg_field per register, per pin, so that's
5 reg_fields per pin which adds up to ~10-24kb extra space, depending on
platform.

Even more alluring would be to have reg_fields describing the actual
fields in the registers, which would allow us to better utilize the
regmap API directly. This would cost us 35-75kb of heap.

IMHO this is quite a significant effort, and given that the driver seems
to be doing its job I'd rather see such efforts being focused elsewhere.

Regards,
Bjorn