Re: PROBLEM: deadlock when imx-sdma logs "restart cyclic channel"

[Tim van der Staaij | Zign]

Hi Fabio,

Fabio Estevam <festevam@xxxxxxxxx> wrote:
>5.19 is not a supported kernel version. Please test it with the latest 6.5-rc1.

Apologies, I was supposed to be using the 5.15 LTS kernel but due to a misconfiguration it got mixed up with a 5.19 image.

Unfortunately the circumstances for this bug are difficult to reproduce and I cannot do so at the moment.

Please disregard the bug report for now, and let me restate the issue as a theoretical question about the locking behavior of the driver.

Consider the following functions in drivers/dma/imx-sdma.c in the latest master revision:

static irqreturn_t sdma_int_handler(int irq, void *dev_id)
{
	struct sdma_engine *sdma = dev_id;
	unsigned long stat;

	stat = readl_relaxed(sdma->regs + SDMA_H_INTR);
	writel_relaxed(stat, sdma->regs + SDMA_H_INTR);
	/* channel 0 is special and not handled here, see run_channel0() */
	stat &= ~1;

	while (stat) {
		int channel = fls(stat) - 1;
		struct sdma_channel *sdmac = &sdma->channel[channel];
		struct sdma_desc *desc;

		spin_lock(&sdmac->vc.lock);
		desc = sdmac->desc;
		if (desc) {
			if (sdmac->flags & IMX_DMA_SG_LOOP) {
				if (sdmac->peripheral_type != IMX_DMATYPE_HDMI)
					sdma_update_channel_loop(sdmac);
				else
					vchan_cyclic_callback(&desc->vd);
			} else {
				mxc_sdma_handle_channel_normal(sdmac);
				vchan_cookie_complete(&desc->vd);
				sdma_start_desc(sdmac);
			}
		}

		spin_unlock(&sdmac->vc.lock);
		__clear_bit(channel, &stat);
	}

	return IRQ_HANDLED;
}

static void sdma_update_channel_loop(struct sdma_channel *sdmac)
{
	struct sdma_buffer_descriptor *bd;
	int error = 0;
	enum dma_status	old_status = sdmac->status;

	/* redacted for conciseness */

	/*
	 * SDMA stops cyclic channel when DMA request triggers a channel and no SDMA
	 * owned buffer is available (i.e. BD_DONE was set too late).
	 */
	if (sdmac->desc && !is_sdma_channel_enabled(sdmac->sdma, sdmac->channel)) {
		dev_warn(sdmac->sdma->dev, "restart cyclic channel %d\n", sdmac->channel);
		sdma_enable_channel(sdmac->sdma, sdmac->channel);
	}
}

As I understand it, sdma_update_channel_loop potentially calls dev_warn while sdmac->vc.lock is still held by sdma_int_handler. Then, dev_warn will also try to claim this lock somewhere down the call stack to safely access the device details. Is this a recipe for deadlock as I suspect, or am I overlooking something?

Kind regards,
Tim