Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201016192654.32610-1-evgreen@chromium.org> <20201016122559.v2.3.Ibb3eedcd634298b039e3af2ec43c7860ae947916@changeid>
In-Reply-To: <20201016122559.v2.3.Ibb3eedcd634298b039e3af2ec43c7860ae947916@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Wed, 21 Oct 2020 14:41:26 -0700
Message-ID: <CAD=FV=WjMi6BdoEjDoM=U=ZHDMsFdLQSU5q20HGjc+DyfnrJEg@mail.gmail.com>
Subject: Re: [PATCH v2 3/4] nvmem: core: Add support for keepout regions
To:     Evan Green <evgreen@chromium.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Stephen Boyd <swboyd@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
>
> Introduce support into the nvmem core for arrays of register ranges
> that should not result in actual device access. For these regions a
> constant byte (repeated) is returned instead on read, and writes are
> quietly ignored and returned as successful.
>
> This is useful for instance if certain efuse regions are protected
> from access by Linux because they contain secret info to another part
> of the system (like an integrated modem).
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
>  - Introduced keepout regions into the core (Srini)
>
>  drivers/nvmem/core.c           | 95 ++++++++++++++++++++++++++++++++--
>  include/linux/nvmem-provider.h | 17 ++++++
>  2 files changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index a09ff8409f600..f7819c57f8828 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -19,6 +19,9 @@
>  #include <linux/of.h>
>  #include <linux/slab.h>
>
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))

Why not use min() / max() macros from include/linux/kernel.h?


> +static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
> +                                     unsigned int offset, void *val,
> +                                     size_t bytes, int write)
> +{
> +
> +       unsigned int end = offset + bytes;
> +       unsigned int kend, ksize;
> +       const struct nvmem_keepout *keepout = nvmem->keepout;
> +       const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
> +       int rc;
> +
> +       /* Skip everything before the range being accessed */

nit: "Skip everything" => "Skip all keepouts"

...might not hurt to remind here that keepouts are sorted?


> +       while ((keepout < keepoutend) && (keepout->end <= offset))
> +               keepout++;
> +
> +       while ((offset < end) && (keepout < keepoutend)) {
> +

nit: remove blank line?


> @@ -647,6 +732,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>         nvmem->type = config->type;
>         nvmem->reg_read = config->reg_read;
>         nvmem->reg_write = config->reg_write;
> +       nvmem->keepout = config->keepout;
> +       nvmem->nkeepout = config->nkeepout;

It seems like it might be worth adding something to validate that the
ranges are sorted and return an error if they're not.

Maybe worth validating (and documenting) that the keepouts won't cause
us to violate "stride" and/or "word_size" ?


Everything above is just nits and other than them this looks like a
nice change.  BTW: this is the kind of thing that screams for unit
testing, though that might be a bit too much of a yak to shave here?

-Doug