Re: [PATCH v7 12/14] crypto: iaa - Add support for deflate-iaa compression algorithm

[Tom Zanussi]

Hi Rex,

On Mon, 2023-07-17 at 10:12 +0800, Rex Zhang wrote:
> Hi, Tom,
> 

[snip]

> > +
> > +static int iaa_comp_acompress(struct acomp_req *req)
> > +{
> > +       struct iaa_compression_ctx *compression_ctx;
> > +       struct crypto_tfm *tfm = req->base.tfm;
> > +       dma_addr_t src_addr, dst_addr;
> > +       int nr_sgs, cpu, ret = 0;
> > +       struct iaa_wq *iaa_wq;
> > +       u32 compression_crc;
> > +       struct idxd_wq *wq;
> > +       struct device *dev;
> > +
> > +       compression_ctx = crypto_tfm_ctx(tfm);
> > +
> > +       if (!iaa_crypto_enabled) {
> > +               pr_debug("iaa_crypto disabled, not compressing\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       if (!req->src || !req->slen) {
> > +               pr_debug("invalid src, not compressing\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       cpu = get_cpu();
> > +       wq = wq_table_next_wq(cpu);
> > +       put_cpu();
> > +       if (!wq) {
> > +               pr_debug("no wq configured for cpu=%d\n", cpu);
> > +               return -ENODEV;
> > +       }
> > +
> > +       ret = iaa_wq_get(wq);
> > +       if (ret) {
> > +               pr_debug("no wq available for cpu=%d\n", cpu);
> > +               return -ENODEV;
> > +       }
> > +
> > +       iaa_wq = idxd_wq_get_private(wq);
> > +
> > +       if (!req->dst) {
> > +               gfp_t flags = req->flags & CRYPTO_TFM_REQ_MAY_SLEEP ? GFP_KERNEL : GFP_ATOMIC;
> > +               /* incompressible data will always be < 2 * slen */
> > +               req->dlen = 2 * req->slen;
> 2 * req->slen is an estimated size for dst buf. When slen is greater
> than 2048 bytes, dlen is greater than 4096 bytes.

Right, so you're saying that because sgl_alloc uses order 0, this could
result in nr_sgs > 1.  Could also just change this to sg_init_one like
all the other callers.

> > +               req->dst = sgl_alloc(req->dlen, flags, NULL);
> > +               if (!req->dst) {
> > +                       ret = -ENOMEM;
> > +                       goto out;
> > +               }
> > +       }
> > +
> > +       dev = &wq->idxd->pdev->dev;
> > +
> > +       nr_sgs = dma_map_sg(dev, req->src, sg_nents(req->src), DMA_TO_DEVICE);
> > +       if (nr_sgs <= 0 || nr_sgs > 1) {
> > +               dev_dbg(dev, "couldn't map src sg for iaa device %d,"
> > +                       " wq %d: ret=%d\n", iaa_wq->iaa_device->idxd->id,
> > +                       iaa_wq->wq->id, ret);
> > +               ret = -EIO;
> > +               goto out;
> > +       }
> > +       src_addr = sg_dma_address(req->src);
> > +       dev_dbg(dev, "dma_map_sg, src_addr %llx, nr_sgs %d, req->src %p,"
> > +               " req->slen %d, sg_dma_len(sg) %d\n", src_addr, nr_sgs,
> > +               req->src, req->slen, sg_dma_len(req->src));
> > +
> > +       nr_sgs = dma_map_sg(dev, req->dst, sg_nents(req->dst), DMA_FROM_DEVICE);
> > +       if (nr_sgs <= 0 || nr_sgs > 1) {
> when dlen is greater than 4096 bytes, nr_sgs maybe greater than 1,
> but the actual output size maybe less than 4096 bytes.
> In other words, the condition nr_sgs > 1 may block a case which could
> have been done.

Currently all existing callers use sg_init_one(), so nr_sgs is never >
1.  But yes, we should add code to be able to handle > 1, I agree.

Thanks,

Tom