Re: [PATCH v2 3/4] nvmem: core: Add support for keepout regions

[Evan Green <evgreen@chromium.org>]

On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson <dianders@chromium.org> wrote:
>
> 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?
>

Done

>
> > +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?

Done

>
>
> > +       while ((keepout < keepoutend) && (keepout->end <= offset))
> > +               keepout++;
> > +
> > +       while ((offset < end) && (keepout < keepoutend)) {
> > +
>
> nit: remove blank line?

Done

>
>
> > @@ -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" ?

Done

>
>
> 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?

It would be cool, but I'll leave that for another time. Thanks for the
review, Doug!

>
> -Doug