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


Lad Prabhakar
 

Hi Pavel,

Thank you for the review.

-----Original Message-----
From: Pavel Machek <pavel@...>
Sent: 30 December 2021 11:15
To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@...>
Cc: cip-dev@...; Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...>; Pavel Machek
<pavel@...>; Biju Das <biju.das.jz@...>
Subject: Re: [PATCH 5.10.y-cip 03/31] ASoC: sh: Add RZ/G2L SSIF-2 driver

Hi!

From: Biju Das <biju.das.jz@...>

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.
Agreed can be dropped.

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?
... agree we need to lock...
+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.
... and here too.

+ /*
+ * 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.

Agreed, this needs to be handled differently with recursion removed.

+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.
rz_ssi_stream_is_valid() does lock/unlock to check stream validity.

+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.
ATM the interrupts aren't shared so this should be OK.

+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.
But the devm_snd_soc_register_component() is after this so it should be OK I believe unless there is a spurious interrupt which will trigger the handler.

Cheers,
Prabhakar

Join cip-dev@lists.cip-project.org to automatically receive all group messages.