Re: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking

[Bartosz Golaszewski]

On Tue, 14 Jul 2026 11:49:14 +0200, Stephan Gerhold
<stephan.gerhold@xxxxxxxxxx> said:
> On Mon, Jul 13, 2026 at 03:01:07PM +0200, 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.
>>
>> We *must* wait until the transaction is signalled as done because we
>> must not perform any writes into config registers while the engine is
>> busy.
>>
>> The dummy writes must be issued into a scratchpad register of the client
>> so provide a mechanism to communicate the right address via descriptor
>> metadata.
>>
>> Reviewed-by: Manivannan Sadhasivam <mani@xxxxxxxxxx>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxxxxxxxx>
>
> Thanks for the fixes. The lock/unlock sequence looks good to me now,
> I commented on a couple of minor things below that would be good to fix
> (some of them are also reported by Sashiko).
>

Thanks.

>> ---
>>  drivers/dma/qcom/bam_dma.c       | 191 +++++++++++++++++++++++++++++++++++++--
>>  include/linux/dma/qcom_bam_dma.h |  14 +++
>>  2 files changed, 198 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
>> index f3e713a5259c2c7c24cfdcec094814eb1202971a..f08549ee3872eece85884606d6ee9e540ee688ca 100644
>> --- a/drivers/dma/qcom/bam_dma.c
>> +++ b/drivers/dma/qcom/bam_dma.c
>> [...]
>> @@ -686,6 +702,35 @@ static int bam_slave_config(struct dma_chan *chan,
>>  	return 0;
>>  }
>>
>> +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)
>
> Doesn't really matter much, but since the parameter exists you might as
> well add
>
> 	&& len == sizeof(*metadata)
>
> here to be sure.
>

Ok.

>> +		return -EINVAL;
>> +
>> +	if (!bdata->pipe_lock_supported)
>> +		/*
>> +		 * The client wants to use locking but this BAM version doesn't
>> +		 * support it. Don't return an error here as this will stop the
>> +		 * client from using DMA at all for no reason.
>> +		 */
>> +		return 0;
>> +
>> +	guard(spinlock_irqsave)(&bchan->vc.lock);
>> +
>> +	bchan->scratchpad_addr = metadata->scratchpad_addr;
>> +	bchan->direction = metadata->direction;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct dma_descriptor_metadata_ops bam_metadata_ops = {
>> +	.attach = bam_metadata_attach,
>> +};
>
> I'm not sure if we have discussed this before, but could we avoid
> re-programming the scratchpad_addr all the time by placing it into
> struct dma_slave_config -> peripheral_config? It still feels awkward to
> me to place a global constant configuration value into per-descriptor
> metadata.
>

Yes, this was discussed several versions ago. I went with using
dmaengine_slave_config() until v11. I think Vinod or Dmitry requested we change
it to the metadata.

>> +
>>  /**
>>   * bam_prep_slave_sg - Prep slave sg transaction
>>   *
>> [...]
>> @@ -802,6 +851,7 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
>>  		}
>>
>>  		vchan_get_all_descriptors(&bchan->vc, &head);
>> +		bchan->bam_locked = false;
>
> I wonder about the implications of this. If the LOCK descriptor was
> already processed, will we cause a deadlock if we never submit the
> UNLOCK descriptor? Or I guess bam_reset_channel() might reset the lock
> as well?
>

I'll look into it.

>>  	}
>>
>>  	vchan_dma_desc_free_list(&bchan->vc, &head);
>> [...]
>> @@ -870,6 +929,7 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>>  {
>>  	u32 i, srcs, pipe_stts, offset, avail;
>>  	struct bam_async_desc *async_desc, *tmp;
>> +	struct bam_desc_hw *hdesc;
>>
>>  	srcs = readl_relaxed(bam_addr(bdev, 0, BAM_IRQ_SRCS_EE));
>>
>> @@ -919,13 +979,20 @@ static u32 process_channel_irqs(struct bam_device *bdev)
>>  			 * push back to front of desc_issued so that
>>  			 * it gets restarted by the work queue.
>>  			 */
>> +
>> +			list_del(&async_desc->desc_node);
>>  			if (!async_desc->num_desc) {
>> -				vchan_cookie_complete(&async_desc->vd);
>> +				hdesc = async_desc->desc;
>> +				u16 flags = le16_to_cpu(hdesc->flags);
>
> Is this unused? Also a bit odd to have hdesc declared outside of the
> loop and flags declared inside.
>
>> +
>> +				if (async_desc->is_lock_desc)
>> +					bam_dma_free_lock_desc(&async_desc->vd);
>> +				else
>> +					vchan_cookie_complete(&async_desc->vd);
>>  			} else {
>>  				list_add(&async_desc->vd.node,
>>  					 &bchan->vc.desc_issued);
>>  			}
>> -			list_del(&async_desc->desc_node);
>>  		}
>>  	}
>>
>> @@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>>  	bchan->reconfigure = 0;
>>  }
>>
>> +static struct bam_async_desc *
>> +bam_make_lock_desc(struct bam_chan *bchan, 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;
>> +
>> +	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 ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	sg_init_table(&async_desc->lock_sg, 1);
>> +
>> +	async_desc->num_desc = 1;
>> +	async_desc->curr_desc = async_desc->desc;
>> +	async_desc->dir = DMA_MEM_TO_DEV;
>> +	async_desc->is_lock_desc = true;
>> +
>> +	desc = async_desc->desc;
>> +
>> +	bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);
>> +	sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
>> +
>> +	mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);
>
> I agree with Sashiko that mapping using the BAM device
> (bchan->bdev->dev) would be more precise, since the BAM will be reading
> the descriptor. (It doesn't matter in practice since both BAM and
> consumer usually have the same IOMMUs defined.)
>

Ok

>> +	if (!mapped) {
>> +		kfree(async_desc);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
>> +	desc->addr = sg_dma_address(&async_desc->lock_sg);
>
> cpu_to_le32()
>

Right.

>> +	desc->size = cpu_to_le16(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_result.result = DMA_TRANS_NOERROR;
>> +	vd->tx_result.residue = 0;
>> +
>> +	return async_desc;
>> +}
>> +
>> [...]
>> @@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
>>  		return;
>>
>>  	while (vd && !IS_BUSY(bchan)) {
>> +		/*
>> +		 * Open a LOCK/UNLOCK bracket around each fresh sequence.
>> +		 * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
>> +		 * already have bam_locked set and must not trigger a second pair.
>> +		 */
>> +		if (!bchan->bam_locked &&
>> +		    !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
>
> Do we need the ->is_lock_desc check here? Looks redundant to me.
>

I don't remember anymore but I think I did it on purpose for some reason. I'll
double check.

>> +			ret = bam_setup_pipe_lock(bchan);
>> +			if (ret == 0 && bchan->bam_locked)
>> +				vd = vchan_next_desc(&bchan->vc);
>
> Do we want some error handling here? If there is an error, this will
> silently continue queuing everything without any locking.
>

Ok, makes sense.

Bart