Re: [PATCH 5.10.y-cip 03/31] ASoC: sh: Add RZ/G2L SSIF-2 driver

[Pavel Machek <pavel@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 5184 bytes --]

Hi!

> From: Biju Das <biju.das.jz@bp.renesas.com>
> 
> commit 03e786bd43410fa93e5d2459f7a43e90ff0ae801 upstream.
> 
> Add serial sound interface(SSIF-2) driver support for
> RZ/G2L SoC.
> 
> Based on the work done by Chris Brandt for RZ/A SSI driver.

I'm not sure what the locking rules are here.

> --- /dev/null
> +++ b/sound/soc/sh/rz-ssi.c
> +static int rz_ssi_stream_is_valid(struct rz_ssi_priv *ssi,
> +				  struct rz_ssi_stream *strm)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&ssi->lock, flags);
> +	ret = !!(strm->substream && strm->substream->runtime);
> +	spin_unlock_irqrestore(&ssi->lock, flags);
> +
> +	return ret;
> +}

Nit: I don't think !!() is useful here, as it is boolean expression
anyway.

But I notice that code is very careful to access strm->substream with
spinlock held.

> +static void rz_ssi_pointer_update(struct rz_ssi_stream *strm, int frames)
> +{
> +	struct snd_pcm_substream *substream = strm->substream;
> +	struct snd_pcm_runtime *runtime;
> +	int current_period;
> +
> +	if (!strm->running || !substream || !substream->runtime)
> +		return;

But here we do same checks, and this time without the spinlock?

> +static int rz_ssi_pio_recv(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
> +{
> +	struct snd_pcm_substream *substream = strm->substream;
> +	struct snd_pcm_runtime *runtime;
> +	u16 *buf;
> +	int fifo_samples;
> +	int frames_left;
> +	int samples = 0;
> +	int i;
> +
> +	if (!rz_ssi_stream_is_valid(ssi, strm))
> +		return -EINVAL;
> +
> +	runtime = substream->runtime;

Again, access without locking. 

> +	/*
> +	 * If we finished this period, but there are more samples in
> +	 * the RX FIFO, call this function again
> +	 */
> +	if (frames_left == 0 && fifo_samples >= runtime->channels)
> +		rz_ssi_pio_recv(ssi, strm);

Here we call ourselves recurively. Without checking the return
value.. but more importantly recursion is unwelcome in kernel due to
limited stack use.


> +static int rz_ssi_pio_send(struct rz_ssi_priv *ssi, struct rz_ssi_stream *strm)
> +{
> +	struct snd_pcm_substream *substream = strm->substream;
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int sample_space;
> +	int samples = 0;
> +	int frames_left;
> +	int i;
> +	u32 ssifsr;
> +	u16 *buf;
> +
> +	if (!rz_ssi_stream_is_valid(ssi, strm))
> +		return -EINVAL;

Access without locking before verifying that stream is valid with the
lock. This is wrong. 

> +static irqreturn_t rz_ssi_interrupt(int irq, void *data)
> +{
> +	struct rz_ssi_stream *strm = NULL;
> +	struct rz_ssi_priv *ssi = data;
> +	u32 ssisr = rz_ssi_reg_readl(ssi, SSISR);
> +
> +	if (ssi->playback.substream)
> +		strm = &ssi->playback;
> +	else if (ssi->capture.substream)
> +		strm = &ssi->capture;
> +	else
> +		return IRQ_HANDLED; /* Left over TX/RX interrupt */

You mark interrupt as handled even when it is not. Probably not a
problem w/o shared interrupts.

> +static int rz_ssi_probe(struct platform_device *pdev)
> +{
...
> +	/* Error Interrupt */
> +	ssi->irq_int = platform_get_irq_byname(pdev, "int_req");
> +	if (ssi->irq_int < 0)
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "Unable to get SSI int_req IRQ\n");
> +
> +	ret = devm_request_irq(&pdev->dev, ssi->irq_int, &rz_ssi_interrupt,
> +			       0, dev_name(&pdev->dev), ssi);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "irq request error (int_req)\n");
> +
> +	/* Tx and Rx interrupts (pio only) */
> +	ssi->irq_tx = platform_get_irq_byname(pdev, "dma_tx");
> +	if (ssi->irq_tx < 0)
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "Unable to get SSI dma_tx IRQ\n");
...

So you registered interrupt handlers...

> +
> +	ret = devm_request_irq(&pdev->dev, ssi->irq_tx, &rz_ssi_interrupt, 0,
> +			       dev_name(&pdev->dev), ssi);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "irq request error (dma_tx)\n");
> +
> +	ssi->irq_rx = platform_get_irq_byname(pdev, "dma_rx");
> +	if (ssi->irq_rx < 0)
> +		return dev_err_probe(&pdev->dev, -ENODEV,
> +				     "Unable to get SSI dma_rx IRQ\n");
> +
> +	ret = devm_request_irq(&pdev->dev, ssi->irq_rx, &rz_ssi_interrupt, 0,
> +			       dev_name(&pdev->dev), ssi);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "irq request error (dma_rx)\n");
> +
> +	ssi->rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(ssi->rstc))
> +		return PTR_ERR(ssi->rstc);
> +
> +	reset_control_deassert(ssi->rstc);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_resume_and_get(&pdev->dev);
> +
> +	spin_lock_init(&ssi->lock);
> +	dev_set_drvdata(&pdev->dev, ssi);

But only here you have data structures ready to handle the
interrupts. You won't see obvious problems w/o shared interrupts, but
I believe there are debugging modes that trigger this intentionally.

Best regards,
								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]