On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson <diand...@chromium.org> wrote:
>
> Hi,
>
> On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgr...@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 <evgr...@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
