[PATCH v2 4/5] lib: sha256: Add support for hardware specific sha256_process

[Simon Glass]

Hi Tom,

On Tue, 7 Feb 2023 at 17:10, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Feb 07, 2023 at 03:25:16PM -0700, Simon Glass wrote:
> > Hi Loic,
> >
> > On Tue, 7 Feb 2023 at 14:47, Loic Poulain <loic.poulain at linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Tue, 7 Feb 2023 at 05:05, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Hi Loic,
> > > >
> > > > On Mon, 6 Feb 2023 at 15:12, Loic Poulain <loic.poulain at linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > Le lun. 6 févr. 2023 à 18:12, Simon Glass <sjg at chromium.org> a écrit :
> > > > >>
> > > > >> Hi Loic,
> > > > >>
> > > > >> On Wed, 1 Jun 2022 at 12:27, Loic Poulain <loic.poulain at linaro.org> wrote:
> > > > >> >
> > > > >> > Mark sha256_process as weak to allow hardware specific implementation.
> > > > >> > Add parameter for supporting multiple blocks processing.
> > > > >> >
> > > > >> > Signed-off-by: Loic Poulain <loic.poulain at linaro.org>
> > > > >> > ---
> > > > >> >  lib/sha256.c | 26 +++++++++++++++++++-------
> > > > >> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > > >> >
> > > [...]
> > > > >> > +__weak void sha256_process(sha256_context *ctx, const unsigned char *data,
> > > > >> > +                          unsigned int blocks)
> > > > >> > +{
> > > > >> > +       if (!blocks)
> > > > >> > +               return;
> > > > >> > +
> > > > >> > +       while (blocks--) {
> > > > >> > +               sha256_process_one(ctx, data);
> > > > >> > +               data += 64;
> > > > >> > +       }
> > > > >> > +}
> > > > >> > +
> > > > >> >  void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length)
> > > > >> >  {
> > > > >> >         uint32_t left, fill;
> > > > >> > @@ -204,17 +218,15 @@ void sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length)
> > > > >> >
> > > > >> >         if (left && length >= fill) {
> > > > >> >                 memcpy((void *) (ctx->buffer + left), (void *) input, fill);
> > > > >> > -               sha256_process(ctx, ctx->buffer);
> > > > >> > +               sha256_process(ctx, ctx->buffer, 1);
> > > > >> >                 length -= fill;
> > > > >> >                 input += fill;
> > > > >> >                 left = 0;
> > > > >> >         }
> > > > >> >
> > > > >> > -       while (length >= 64) {
> > > > >> > -               sha256_process(ctx, input);
> > > > >> > -               length -= 64;
> > > > >> > -               input += 64;
> > > > >> > -       }
> > > > >> > +       sha256_process(ctx, input, length / 64);
> > > > >> > +       input += length / 64 * 64;
> > > > >> > +       length = length % 64;
> > > > >> >
> > > > >> >         if (length)
> > > > >> >                 memcpy((void *) (ctx->buffer + left), (void *) input, length);
> > > > >> > --
> > > > >> > 2.7.4
> > > > >> >
> > > > >>
> > > > >> I just came across this patch as it broke minnowmax.
> > > > >
> > > > >
> > > > > Ok, is it a build time or runtime break?
> > > >
> > > > Build, but you need the binary blobs to see it :-(
> > > > >>
> > > > >> This should be using driver model, not weak functions. Please can you
> > > > >> take a look?
> > >
> > > Just tested the minnowmax build (b69026c91f2e; minnowmax_defconfig;
> > > gcc-11.3.0), and I've not observed any issue (but I had to fake some
> > > of the binary blobs...). Could you share the build problem/error you
> >
> > Unfortunately you need the blobs!
> >
> > > encountered? As you mentioned it, Is the error specifically related to
> > > _weak function linking? Would like to have a simple and quick fix
> > > before trying to move on to a more proper DM based solution.
> >
> > It is just because of the code size increase, I believe. I am planning
> > to dig into it a bit as Bin Meng asked for more info as to why I sent
> > a revert for his patch moving U-Boot.
>
> That honestly makes more sense, having stared at the commit in
> question.  Perhaps Minnow needs LTO enabled.

Yes, that would likely help. But Bin's point is that it should be
possible to move the text base.

Anyway, the point of this thread is that this needs to be done as
drivers rather than weak functions. Unfortunately hash.c has grown a
few appendages...this is yet another.

Regards,
Simon