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: 3470 bytes --]

Hi!

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

Yes, but "struct snd_pcm_runtime *runtime = substream->runtime" above
already accessed that data without locking.

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

I believe your code will do fine in usual configurations. But
reordering it should be quite easy, and there are kernel options like
"irqpoll", "irqfixup" and CONFIG_DEBUG_SHIRQ.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]