Re: [PATCH 2/4] crypto: sun8i-ce - wrap accesses to descriptor address fields

[Andre Przywara <andre.przywara@arm.com>]

On Fri, 21 Jun 2024 22:53:47 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi,

> On Mon, Jun 17, 2024 at 6:08 AM Andre Przywara <andre.przywara@arm.com> wrote:
> >
> > The Allwinner H616 (and later) SoCs support more than 32 bits worth of
> > physical addresses. To accommodate the larger address space, the CE task
> > descriptor fields holding addresses are now encoded as "word addresses",
> > so take the actual address divided by four.
> > This is true for the fields within the descriptor, but also for the
> > descriptor base address, in the CE_TDA register.
> >
> > Wrap all accesses to those fields in a function, which will do the
> > required division if needed. For now this in unused, so there should be
> > no change in behaviour.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>  
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Thanks for that!

> though you need to fix up the reported sparse warning in sun8i_ce_run_task().

Yeah, that turned out to be a nasty one, and uncovered an actual big
endian bug.
I dropped your R-b from v2 (in your inbox after testing), since I split up
the function and used a different variant for this one writel() caller. So
if you are happy with the changes in sun8i-ce.h and sun8i-ce-core.c: the
other files just use the renamed function name and didn't change otherwise.

Cheers,
Andre

> 
> > ---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c |  8 ++++----
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c   |  3 ++-
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c   |  6 +++---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c   |  6 +++---
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c   |  2 +-
> >  drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h        | 10 ++++++++++
> >  6 files changed, 23 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > index de50c00ba218f..3a5674b1bd3c0 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> > @@ -190,7 +190,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >                 err = -EFAULT;
> >                 goto theend;
> >         }
> > -       cet->t_key = cpu_to_le32(rctx->addr_key);
> > +       cet->t_key = sun8i_ce_desc_addr(ce, rctx->addr_key);
> >
> >         ivsize = crypto_skcipher_ivsize(tfm);
> >         if (areq->iv && crypto_skcipher_ivsize(tfm) > 0) {
> > @@ -208,7 +208,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >                         err = -ENOMEM;
> >                         goto theend_iv;
> >                 }
> > -               cet->t_iv = cpu_to_le32(rctx->addr_iv);
> > +               cet->t_iv = sun8i_ce_desc_addr(ce, rctx->addr_iv);
> >         }
> >
> >         if (areq->src == areq->dst) {
> > @@ -236,7 +236,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >
> >         len = areq->cryptlen;
> >         for_each_sg(areq->src, sg, nr_sgs, i) {
> > -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_src[i].len = cpu_to_le32(todo / 4);
> >                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> > @@ -251,7 +251,7 @@ static int sun8i_ce_cipher_prepare(struct crypto_engine *engine, void *async_req
> >
> >         len = areq->cryptlen;
> >         for_each_sg(areq->dst, sg, nr_sgd, i) {
> > -               cet->t_dst[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_dst[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_dst[i].len = cpu_to_le32(todo / 4);
> >                 dev_dbg(ce->dev, "%s total=%u SG(%d %u off=%d) todo=%u\n", __func__,
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > index 0408b2d5d533b..89ab3e08f0697 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-core.c
> > @@ -172,7 +172,8 @@ int sun8i_ce_run_task(struct sun8i_ce_dev *ce, int flow, const char *name)
> >         writel(v, ce->base + CE_ICR);
> >
> >         reinit_completion(&ce->chanlist[flow].complete);
> > -       writel(ce->chanlist[flow].t_phy, ce->base + CE_TDQ);
> > +       writel(sun8i_ce_desc_addr(ce, ce->chanlist[flow].t_phy),
> > +              ce->base + CE_TDQ);
> >
> >         ce->chanlist[flow].status = 0;
> >         /* Be sure all data is written before enabling the task */
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > index ee2a28c906ede..a710ec9aa96f1 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c
> > @@ -403,7 +403,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >
> >         len = areq->nbytes;
> >         for_each_sg(areq->src, sg, nr_sgs, i) {
> > -               cet->t_src[i].addr = cpu_to_le32(sg_dma_address(sg));
> > +               cet->t_src[i].addr = sun8i_ce_desc_addr(ce, sg_dma_address(sg));
> >                 todo = min(len, sg_dma_len(sg));
> >                 cet->t_src[i].len = cpu_to_le32(todo / 4);
> >                 len -= todo;
> > @@ -414,7 +414,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >                 goto theend;
> >         }
> >         addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE);
> > -       cet->t_dst[0].addr = cpu_to_le32(addr_res);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, addr_res);
> >         cet->t_dst[0].len = cpu_to_le32(digestsize / 4);
> >         if (dma_mapping_error(ce->dev, addr_res)) {
> >                 dev_err(ce->dev, "DMA map dest\n");
> > @@ -445,7 +445,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq)
> >         }
> >
> >         addr_pad = dma_map_single(ce->dev, buf, j * 4, DMA_TO_DEVICE);
> > -       cet->t_src[i].addr = cpu_to_le32(addr_pad);
> > +       cet->t_src[i].addr = sun8i_ce_desc_addr(ce, addr_pad);
> >         cet->t_src[i].len = cpu_to_le32(j);
> >         if (dma_mapping_error(ce->dev, addr_pad)) {
> >                 dev_err(ce->dev, "DMA error on padding SG\n");
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > index 80815379f6fc5..f030167f95945 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-prng.c
> > @@ -132,10 +132,10 @@ int sun8i_ce_prng_generate(struct crypto_rng *tfm, const u8 *src,
> >         cet->t_sym_ctl = cpu_to_le32(sym);
> >         cet->t_asym_ctl = 0;
> >
> > -       cet->t_key = cpu_to_le32(dma_iv);
> > -       cet->t_iv = cpu_to_le32(dma_iv);
> > +       cet->t_key = sun8i_ce_desc_addr(ce, dma_iv);
> > +       cet->t_iv = sun8i_ce_desc_addr(ce, dma_iv);
> >
> > -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
> >         cet->t_dst[0].len = cpu_to_le32(todo / 4);
> >         ce->chanlist[flow].timeout = 2000;
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > index 9c35f2a83eda8..465c1c512eb85 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-trng.c
> > @@ -77,7 +77,7 @@ static int sun8i_ce_trng_read(struct hwrng *rng, void *data, size_t max, bool wa
> >         cet->t_sym_ctl = 0;
> >         cet->t_asym_ctl = 0;
> >
> > -       cet->t_dst[0].addr = cpu_to_le32(dma_dst);
> > +       cet->t_dst[0].addr = sun8i_ce_desc_addr(ce, dma_dst);
> >         cet->t_dst[0].len = cpu_to_le32(todo / 4);
> >         ce->chanlist[flow].timeout = todo;
> >
> > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > index 93d4985def87a..8fa58f3bb7f86 100644
> > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce.h
> > @@ -149,6 +149,7 @@ struct ce_variant {
> >         bool hash_t_dlen_in_bits;
> >         bool prng_t_dlen_in_bytes;
> >         bool trng_t_dlen_in_bytes;
> > +       bool needs_word_addresses;
> >         struct ce_clock ce_clks[CE_MAX_CLOCKS];
> >         int esr;
> >         unsigned char prng;
> > @@ -241,6 +242,15 @@ struct sun8i_ce_dev {
> >  #endif
> >  };
> >
> > +static inline __le32 sun8i_ce_desc_addr(struct sun8i_ce_dev *dev,
> > +                                       dma_addr_t addr)
> > +{
> > +       if (dev->variant->needs_word_addresses)
> > +               return cpu_to_le32(addr / 4);
> > +
> > +       return cpu_to_le32(addr);
> > +}
> > +
> >  /*
> >   * struct sun8i_cipher_req_ctx - context for a skcipher request
> >   * @op_dir:            direction (encrypt vs decrypt) for this request
> > --
> > 2.39.4
> >