| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <39190cdd-46e8-4c8f-a2e4-608e9ea19ac4@gmail.com>
Date: Sat, 21 Sep 2024 23:13:31 +0300
From: Péter Ujfalusi <peter.ujfalusi@...il.com>
To: Jai Luthra <jai.luthra@...ux.dev>, Vinod Koul <vkoul@...nel.org>
Cc: Vignesh Raghavendra <vigneshr@...com>,
Francesco Dolcini <francesco.dolcini@...adex.com>,
Devarsh Thakkar <devarsht@...com>, Rishikesh Donadkar <r-donadkar@...com>,
dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH] dmaengine: ti: k3-udma: Fix teardown for cyclic PDMA
transfers
Hi Jai,
I only have couple of suggestions, mainly for the comments and commit
message, otherwise looks good.
On 18/09/2024 16:16, Jai Luthra wrote:
> From: Jai Luthra <j-luthra@...com>
>
> When receiving data in cyclic mode from PDMA peripherals, where reload
> count is set to infinite, any TR in the set can potentially be the last
> one of the overall transfer. In such cases, the EOP flag needs to be set
> in each TR and PDMA's Static TR "Z" parameter should be set, matching
> the size of the TR.
>
> This is required for the teardown to function properly and cleanup the
> internal state memory. This only affects platforms using BCDMA and not
> those using UDMA-P, which could set EOP flag in the teardown TR
> automatically.
Since this only affects BCDMA, I would clarify the commit title to
something:
dmaengine: ti: k3-udma: Set EOP flag for all TRs in cyclic BCDMA transfer
And adjust the commit message accordingly.
Basically we kind of emulate PacketMode with this on BCDMA, right?
> Similarly when transmitting data in cyclic mode to PDMA peripherals, the
> EOP flag needs to be set to get the teardown completion signal
> correctly.
>
> Fixes: 017794739702 ("dmaengine: ti: k3-udma: Initial support for K3 BCDMA")
> Signed-off-by: Jai Luthra <j-luthra@...com>
> Signed-off-by: Jai Luthra <jai.luthra@...ux.dev>
> ---
> drivers/dma/ti/k3-udma.c | 61 ++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 406ee199c2ac..5a900b63dae5 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -3185,27 +3185,39 @@ static int udma_configure_statictr(struct udma_chan *uc, struct udma_desc *d,
>
> d->static_tr.elcnt = elcnt;
>
> - /*
> - * PDMA must to close the packet when the channel is in packet mode.
> - * For TR mode when the channel is not cyclic we also need PDMA to close
> - * the packet otherwise the transfer will stall because PDMA holds on
> - * the data it has received from the peripheral.
> - */
> if (uc->config.pkt_mode || !uc->cyclic) {
> + /*
> + * PDMA must close the packet when the channel is in packet mode.
> + * For TR mode when the channel is not cyclic we also need PDMA
> + * to close the packet otherwise the transfer will stall because
> + * PDMA holds on the data it has received from the peripheral.
> + */
> unsigned int div = dev_width * elcnt;
>
> if (uc->cyclic)
> d->static_tr.bstcnt = d->residue / d->sglen / div;
> else
> d->static_tr.bstcnt = d->residue / div;
> + } else if (uc->ud->match_data->type == DMA_TYPE_BCDMA &&
> + uc->config.dir == DMA_DEV_TO_MEM && !uc->config.pkt_mode &&
> + uc->cyclic) {
BCDMA and pkt_mode cannot be true at the same time, the check for
!uc->config.pkt_mode can be dropped.
> + /*
> + * For cyclic TR mode PDMA must close the packet after every TR
This is only valid for BCDMA, the comment should mention this.
> + * transfer, as we have to set EOP in each TR to prevent short
> + * packet errors seen on channel teardown.
> + */
> + struct cppi5_tr_type1_t *tr_req = d->hwdesc[0].tr_req_base;
>
> - if (uc->config.dir == DMA_DEV_TO_MEM &&
> - d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
> - return -EINVAL;
> + d->static_tr.bstcnt =
> + (tr_req->icnt0 * tr_req->icnt1) / dev_width;
> } else {
> d->static_tr.bstcnt = 0;
> }
>
> + if (uc->config.dir == DMA_DEV_TO_MEM &&
> + d->static_tr.bstcnt > uc->ud->match_data->statictr_z_mask)
> + return -EINVAL;
> +
> return 0;
> }
>
> @@ -3450,8 +3462,9 @@ udma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> /* static TR for remote PDMA */
> if (udma_configure_statictr(uc, d, dev_width, burst)) {
> dev_err(uc->ud->dev,
> - "%s: StaticTR Z is limited to maximum 4095 (%u)\n",
> - __func__, d->static_tr.bstcnt);
> + "%s: StaticTR Z is limited to maximum %u (%u)\n",
> + __func__, uc->ud->match_data->statictr_z_mask,
> + d->static_tr.bstcnt);
>
> udma_free_hwdesc(uc, d);
> kfree(d);
> @@ -3476,6 +3489,7 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
> u16 tr0_cnt0, tr0_cnt1, tr1_cnt0;
> unsigned int i;
> int num_tr;
> + u32 period_csf = 0;
>
> num_tr = udma_get_tr_counters(period_len, __ffs(buf_addr), &tr0_cnt0,
> &tr0_cnt1, &tr1_cnt0);
> @@ -3498,6 +3512,20 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
> period_addr = buf_addr |
> ((u64)uc->config.asel << K3_ADDRESS_ASEL_SHIFT);
>
> + /*
> + * For BCDMA <-> PDMA transfers, the EOP flag needs to be set on the
> + * last TR of a descriptor, to mark the packet as complete.
> + * This is required for getting the teardown completion message in case
> + * of TX, and to avoid short-packet error in case of RX.
> + *
> + * As we are in cyclic mode, we do not know which period might be the
> + * last one, so set the flag for each period.
> + */
> + if (uc->config.ep_type == PSIL_EP_PDMA_XY &&
> + uc->ud->match_data->type == DMA_TYPE_BCDMA) {
> + period_csf = CPPI5_TR_CSF_EOP;
> + }
> +
> for (i = 0; i < periods; i++) {
> int tr_idx = i * num_tr;
>
> @@ -3525,8 +3553,10 @@ udma_prep_dma_cyclic_tr(struct udma_chan *uc, dma_addr_t buf_addr,
> }
>
> if (!(flags & DMA_PREP_INTERRUPT))
> - cppi5_tr_csf_set(&tr_req[tr_idx].flags,
> - CPPI5_TR_CSF_SUPR_EVT);
> + period_csf |= CPPI5_TR_CSF_SUPR_EVT;
> +
> + if (period_csf)
> + cppi5_tr_csf_set(&tr_req[tr_idx].flags, period_csf);
>
> period_addr += period_len;
> }
> @@ -3655,8 +3685,9 @@ udma_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, size_t buf_len,
> /* static TR for remote PDMA */
> if (udma_configure_statictr(uc, d, dev_width, burst)) {
> dev_err(uc->ud->dev,
> - "%s: StaticTR Z is limited to maximum 4095 (%u)\n",
> - __func__, d->static_tr.bstcnt);
> + "%s: StaticTR Z is limited to maximum %u (%u)\n",
> + __func__, uc->ud->match_data->statictr_z_mask,
> + d->static_tr.bstcnt);
>
> udma_free_hwdesc(uc, d);
> kfree(d);
>
> ---
> base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
> change-id: 20240918-z_cnt-3ca5daec76bf
>
> Best regards,
--
Péter
Powered by blists - more mailing lists