From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CE6DD3A3835 for ; Mon, 20 Apr 2026 12:51:06 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776689468; cv=none; b=RgBX9+Ui4Rmz7Ni2IpKQDDOCi6p40auXGblef2tM6Cvu0WmHjweUyu3Y6iER6gCR9D7g3SkcgRQtYaTb23VjKkp/aP3B+KDUn6hlinZm4JUv+5ubpt3HdkfcfxLsRjRbydKC2hElefcfVSpexyK5KRZ3iPGfGjiJRfKE0BkJOjI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776689468; c=relaxed/simple; bh=GjzpQGSlzmq4JGLEUzkRDbk43l5X0HJhsEu+aFSgxak=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=UytdTRmXLVmqb9a+B0RQPsoM0D3UTziaZ4tfYLsesFnFN+PU5JZ7sx1petdWjV0KF8ofn7NAGftJCS6Ccytl/+5kIkYFl13XjjZc0KwtBd4cBHayVMGhAz57sh4c1k4hdflke9zwyQG4+842K2MBBvBwS6ipDpUsUKRxMdS9elQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev; spf=pass smtp.mailfrom=tuxon.dev; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b=Hk91EX9i; arc=none smtp.client-ip=209.85.128.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=tuxon.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=tuxon.dev header.i=@tuxon.dev header.b="Hk91EX9i" Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-488af96f6b2so39067105e9.0 for ; Mon, 20 Apr 2026 05:51:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tuxon.dev; s=google; t=1776689465; x=1777294265; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=DwmFMA4VViFtwcMmrRs7UsG2wa49WxvRCvYdPiRzSRg=; b=Hk91EX9iPQQFRWP7bM+GPkpq9Y8JpinhvrnRbGS9JC+4MWxY0+vUYsgOIcgY2ogBnD 1EYM7ttCOhnq9AbuSAK5u5WF6oMG9pSE9JFqVK1bKG8nRBInEVZez9C+vQR9AC4LYkYr EOobxykzNmG1knboi5GFrGGFyjLguPprNhiEg7AkgPEOI3BWt2ecS+2J++OZjGYzBBCV frdbEWZ2iGmbOfEnT3tUJUh75GzYmB0R8F3Kwx0yED2RcpBpCh5YnL/eLkT/I3+Q/YjY KsxbjNtWzdLcGtpQ6/NIXwr0oZhXpqc3uX40QW8gy5Kf9ymnnBXu8lkzWWFRG29yjjx2 d0og== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776689465; x=1777294265; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=DwmFMA4VViFtwcMmrRs7UsG2wa49WxvRCvYdPiRzSRg=; b=N7ILHpzN/gjdqVXGSeVBToCQApXOupzLyUt+w2gy2LN3kiiUtgxOGcbHg159R7XJyJ PomjXr03f44E/sQJDeH4rOslj3iA6hv8kL4/S58SpC5LGzEj8yNmT0DLudzg4jBef6am ujlJtHZCJRV8fVv5Ejq64Ked089VPU0gtcD2nTht0dlqRMeqcfc82pCRRm8YGrbfPhEM tfKQe5h5SZnPrV1VbD+8j92+VpIvHWrWQBw7bs/JeXnXwsUjlTACESaIRM7N2ZdNKI73 cGyfrF3UU9ZAD4r5XHzt6PEPpebTlInQZUwa4TBV+UWsthurlJxpl2hAo7udJoKXp5nt hu8w== X-Forwarded-Encrypted: i=1; AFNElJ9TZk2FvP/jI4uDXqLwmoUWXn5YFAKemShoUfo/UvPYUpR29fjpGD2LIhZ2+ZEDMjfYprpaLEn4r9zjZ6I=@vger.kernel.org X-Gm-Message-State: AOJu0Yx7LmcS3WQcGDWsGbuXII6S1Kgk50PWymNLEOFDfppoJcxhA7Vu u8YYfnPCQ1xG5XFK7RSOY1XCQOmvnPmsOgX1BtvDsJrby2+jhHN/uSvGVw1QlrXi3Wk= X-Gm-Gg: AeBDieud5Rx54B5ycTq6rXi6BwM1avRXKRMw+/a3D3O6uiXzBfnFiLx4Iy34UTUHmNJ EYDoqBDIZLdSXgJoKDLxAaQExEMdqY+ifdRoVpV+wlIEnHjjFiCP+3OEYjI3FMCHORYjxNRr6LX veZi9NjmVYCQTCncqZtwZnf9sagFP3VKlCeBAY9Nk3MX+UU9xb0kgvQS+0zzuGe9j48lBD2Hk1J eCseuWVBgmsmP43DhVyiEhgDdOffCiVwBQPY5rK9gILjTFCrV9GgC+WkHHISITdcXKa1pSP/fWD pC5HuY5cKQ73eIJShfKGSxKRVrDQZnC4AGiT7OMHblyC1pw/XHYlP4gJiXr4l1uYbSBnrhPUXR4 oK8YKf846/8MLn4AikkUV/M3X4nsbvKMcB1Bfw4an9dnqOndjzeycRY2LuRa/BbAXaBd/OWCoYl 8naF90fKv9DyBQFWzPHoDTkQhmI9Hulv8zOx0Kf2/ATw== X-Received: by 2002:a05:600c:348b:b0:489:5022:39a4 with SMTP id 5b1f17b1804b1-4895040c025mr31948545e9.9.1776689465141; Mon, 20 Apr 2026 05:51:05 -0700 (PDT) Received: from [192.168.50.4] ([82.78.167.123]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-488fc100162sm307580375e9.5.2026.04.20.05.51.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Apr 2026 05:51:04 -0700 (PDT) Message-ID: <047007f9-d5cc-401a-846a-ab13ad0380b6@tuxon.dev> Date: Mon, 20 Apr 2026 15:51:03 +0300 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: sashiko.dev review (Re: [PATCH v4 13/17] dmaengine: sh: rz-dmac: Add cyclic DMA support) To: vkoul@kernel.org, Frank.Li@kernel.org, lgirdwood@gmail.com, broonie@kernel.org, perex@perex.cz, tiwai@suse.com, biju.das.jz@bp.renesas.com, prabhakar.mahadev-lad.rj@bp.renesas.com, p.zabel@pengutronix.de, geert+renesas@glider.be, fabrizio.castro.jz@renesas.com, long.luu.ur@renesas.com Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sound@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Claudiu Beznea References: <20260411114303.2814115-1-claudiu.beznea.uj@bp.renesas.com> <20260411114303.2814115-14-claudiu.beznea.uj@bp.renesas.com> Content-Language: en-US From: Claudiu Beznea In-Reply-To: <20260411114303.2814115-14-claudiu.beznea.uj@bp.renesas.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit > commit cb66d54ec04fe2598560135c66410142d7295633 > Author: Claudiu Beznea > dmaengine: sh: rz-dmac: Add cyclic DMA support > This commit introduces cyclic DMA support to the RZ DMAC driver. It adds a > per-channel status bit to mark cyclic channels and sets it during the DMA > prepare callback. The IRQ handler then uses this bit to determine whether > to invoke the cyclic callback function. > > diff --git a/drivers/dma/sh/rz-dmac.c b/drivers/dma/sh/rz-dmac.c > > index 958ee45abc700..9a10430109e59 100644 > > --- a/drivers/dma/sh/rz-dmac.c > > +++ b/drivers/dma/sh/rz-dmac.c > [ ... ] > > @@ -586,6 +645,55 @@ rz_dmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > [ ... ] > > + scoped_guard(spinlock_irqsave, &channel->vc.lock) { > > + if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) > > + return NULL; > > + > > + desc = list_first_entry_or_null(&channel->ld_free, struct rz_dmac_desc, node); > > + if (!desc) > > + return NULL; > > + > > + list_del(&desc->node); > > + > > + channel->status |= BIT(RZ_DMAC_CHAN_STATUS_CYCLIC); > > + } > > + > > + desc->type = RZ_DMAC_DESC_CYCLIC; > > + desc->sgcount = period_len; > Can tracking cyclic state via the global channel->status flag lead to > cross-descriptor state corruption? > If a prepared cyclic descriptor is freed via dmaengine_desc_free(), the > global flag might not be cleared during virt-dma desc release, leaving > the channel poisoned. dmaengine_desc_free() works only for reusable descriptors. This functionality is not implemented by the rz-dmac driver. > Also, because the lock is dropped when exiting the scoped_guard before > vchan_tx_prep() is called, a concurrent terminate_all() could potentially > clear the flag before the descriptor is queued. > Could the driver rely on the desc->type field instead of a global state > flag to identify cyclic transfers? I'm going to explore that but I think this will complicates the code further. Vinod, Frank, Could you please let me know if this is a valid expected usecase? > [ ... ] > > @@ -733,9 +841,18 @@ static u32 rz_dmac_calculate_residue_bytes_in_vd(struct rz_dmac_chan *channel, > > } > > > > /* Calculate residue from next lmdesc to end of virtual desc */ > > - while (lmdesc->chcfg & CHCFG_DEM) { > > - residue += lmdesc->tb; > > - lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc); > > + if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) { > > + u32 start_lmdesc_addr = rz_dmac_lmdesc_addr(channel, desc->start_lmdesc); > > + > > + while (lmdesc->nxla != start_lmdesc_addr) { > > + residue += lmdesc->tb; > > + lmdesc = rz_dmac_get_next_lmdesc(channel->lmdesc.base, lmdesc); > > + } > Will this loop calculate a monotonically decreasing residue for cyclic > transfers? Yes, according to my testing. > ALSA requires the residue to be the exact, monotonically decreasing number > of bytes remaining in the buffer. Iterating until the end of the descriptor > ring and adding the lengths of subsequent periods might cause the returned > residue to jump wildly depending on which period is currently executing. > For example, in a 3-period buffer (A, B, C), executing period B might > return a much larger residue than executing period A, since it would add > the remaining bytes in the ring differently. > Could this non-monotonic behavior trigger 'PCM position is invalid' > errors in the ALSA framework, breaking audio playback? Never see this in my testing. > [ ... ] > > @@ -924,10 +1041,14 @@ static irqreturn_t rz_dmac_irq_handler_thread(int irq, void *dev_id) > > if (!desc) > > return IRQ_HANDLED; > > > > - vchan_cookie_complete(&desc->vd); > > - channel->desc = NULL; > > + if (channel->status & BIT(RZ_DMAC_CHAN_STATUS_CYCLIC)) { > > + vchan_cyclic_callback(&desc->vd); > > + } else { > > + vchan_cookie_complete(&desc->vd); > > + channel->desc = NULL; > If a non-cyclic transfer completes while the global channel->status flag > is set, could the IRQ handler erroneously call vchan_cyclic_callback() > instead of vchan_cookie_complete(), permanently hanging the channel? > Checking desc->type == RZ_DMAC_DESC_CYCLIC here instead might prevent > misidentifying non-cyclic transfers as cyclic. Once the channel is configured as cyclic it cannot be used otherwise. The use has to call terminate_all(), synchornize() + re-get + re-prepare the transfers. So, this exact scenario here is invalid according to the current use case. If the status is set it means the channel->desc = NULL, which means the IRQ handler is not executed up to this point.