Re: [PATCH v13 05/12] dmaengine: qcom: bam_dma: add support for BAM locking

[Bartosz Golaszewski]

On Mon, Mar 23, 2026 at 10:35 AM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
>
> On Tue, Mar 17, 2026 at 03:02:12PM +0100, Bartosz Golaszewski wrote:
> > Add support for BAM pipe locking. To that end: when starting DMA on an RX
> > channel - prepend the existing queue of issued descriptors with an
> > additional "dummy" command descriptor with the LOCK bit set. Once the
> > transaction is done (no more issued descriptors), issue one more dummy
> > descriptor with the UNLOCK bit.
>
> I've left some comments in v12, but looks like you've missed them.

Sorry for that, as I explained in private, this email did not end up
in my inbox and I didn't see it on lore.

> >
> > +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> > +{
> > +     struct bam_chan *bchan = to_bam_chan(desc->chan);
> > +     const struct bam_device_data *bdata = bchan->bdev->dev_data;
> > +     struct bam_desc_metadata *metadata = data;
> > +
> > +     if (!data)
> > +             return -EINVAL;
> > +
> > +     if (!bdata->pipe_lock_supported)
> > +             return -EOPNOTSUPP;
>
> As mentioned in v12, you should return 0 to avoid erroring out the clients if
> pipe lock is not supported.
>

If the client attaches the scratchpad register then it probably does
want to use locking, right? On the other hand, I assume you're
thinking about a situation where the client wants locking but BAM does
not support it. It's unlikely but ok, I'll change it.

> >
> > +static struct bam_async_desc *
> > +bam_make_lock_desc(struct bam_chan *bchan, struct scatterlist *sg,
> > +                struct bam_cmd_element *ce, unsigned long flag)
> > +{
> > +     struct dma_chan *chan = &bchan->vc.chan;
> > +     struct bam_async_desc *async_desc;
> > +     struct bam_desc_hw *desc;
> > +     struct virt_dma_desc *vd;
> > +     struct virt_dma_chan *vc;
> > +     unsigned int mapped;
> > +     dma_cookie_t cookie;
> > +     int ret;
> > +
> > +     sg_init_table(sg, 1);
> > +
> > +     async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> > +     if (!async_desc) {
> > +             dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> > +             return NULL;
> > +     }
> > +
> > +     async_desc->num_desc = 1;
> > +     async_desc->curr_desc = async_desc->desc;
> > +     async_desc->dir = DMA_MEM_TO_DEV;
> > +
> > +     desc = async_desc->desc;
> > +
> > +     bam_prep_ce_le32(ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
> > +     sg_set_buf(sg, ce, sizeof(*ce));
> > +
> > +     mapped = dma_map_sg_attrs(chan->slave, sg, 1, DMA_TO_DEVICE, DMA_PREP_CMD);
> > +     if (!mapped) {
> > +             kfree(async_desc);
> > +             return NULL;
> > +     }
> > +
> > +     desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> > +     desc->addr = sg_dma_address(sg);
> > +     desc->size = sizeof(struct bam_cmd_element);
> > +
> > +     vc = &bchan->vc;
> > +     vd = &async_desc->vd;
> > +
> > +     dma_async_tx_descriptor_init(&vd->tx, &vc->chan);
> > +     vd->tx.flags = DMA_PREP_CMD;
> > +     vd->tx.desc_free = vchan_tx_desc_free;
> > +     vd->tx_result.result = DMA_TRANS_NOERROR;
> > +     vd->tx_result.residue = 0;
> > +
> > +     cookie = dma_cookie_assign(&vd->tx);
> > +     ret = dma_submit_error(cookie);
> > +     if (ret)
> > +             return NULL;
>
> You are leaking async_desc here.
>

Yeah, not only that but also should unmap the sg here too. Thanks.

> > +
> > +     return async_desc;
> > +}
> > +
> > +static int bam_do_setup_pipe_lock(struct bam_chan *bchan, bool lock)
> > +{
> > +     struct bam_device *bdev = bchan->bdev;
> > +     const struct bam_device_data *bdata = bdev->dev_data;
> > +     struct bam_async_desc *lock_desc;
> > +     struct bam_cmd_element *ce;
> > +     struct scatterlist *sgl;
> > +     unsigned long flag;
> > +
> > +     lockdep_assert_held(&bchan->vc.lock);
> > +
> > +     if (!bdata->pipe_lock_supported || !bchan->scratchpad_addr ||
> > +         bchan->slave.direction != DMA_MEM_TO_DEV)
> > +             return 0;
> > +
> > +     if (lock) {
> > +             sgl = &bchan->lock_sg;
> > +             ce = &bchan->lock_ce;
> > +             flag = DESC_FLAG_LOCK;
> > +     } else {
> > +             sgl = &bchan->unlock_sg;
> > +             ce = &bchan->unlock_ce;
> > +             flag = DESC_FLAG_UNLOCK;
> > +     }
> > +
> > +     lock_desc = bam_make_lock_desc(bchan, sgl, ce, flag);
> > +     if (!lock_desc)
> > +             return -ENOMEM;
> > +
> > +     if (lock)
> > +             list_add(&lock_desc->vd.node, &bchan->vc.desc_issued);
> > +     else
> > +             list_add_tail(&lock_desc->vd.node, &bchan->vc.desc_issued);
> > +
> > +     bchan->locked = lock;
>
> What is this flag for?
>

Just a leftover. I'll drop it, thanks.

> >
> > +struct bam_desc_metadata {
> > +     phys_addr_t scratchpad_addr;
>
> I think it'd be worth adding a comment for this.
>

Will do.

Bart