lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160524164151.GM8206@sirena.org.uk>
Date:	Tue, 24 May 2016 17:41:51 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Jose Abreu <Jose.Abreu@...opsys.com>
Cc:	alsa-devel@...a-project.org,
	Carlos Palminha <CARLOS.PALMINHA@...opsys.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>, Rob Herring <robh@...nel.org>,
	Alexey Brodkin <Alexey.Brodkin@...opsys.com>,
	linux-snps-arc@...ts.infradead.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

On Mon, May 23, 2016 at 11:02:24AM +0100, Jose Abreu wrote:

> +config SND_DESIGNWARE_PCM
> +	bool "PCM PIO extension for I2S driver"

Why can't this be built as a module?

> +
> +	return irq_valid ? IRQ_HANDLED : IRQ_NONE;

Please write a normal if statement, the ternery operator doesn't help
legibility.

>  static void i2s_start(struct dw_i2s_dev *dev,
>  		      struct snd_pcm_substream *substream)
>  {
>  	struct i2s_clk_config_data *config = &dev->config;
>  
>  	i2s_write_reg(dev->i2s_base, IER, 1);
> -	i2s_enable_irqs(dev, substream->stream, config->chan_nr);
> +
> +	if (dev->use_pio)
> +		i2s_enable_irqs(dev, substream->stream, config->chan_nr);
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
>  		i2s_write_reg(dev->i2s_base, ITER, 1);

That seems wrong, or at least something that should be separate?
Previously we needed interrupts for DMA operation but now we enable
interrupts only if we don't use DMA.  It feels like we want to make the
change for DMA separately if only to make it clear for bisection, are we
100% sure that masking the interrupt won't also mask the DMA request
signals?

> +	irq = platform_get_irq(pdev, 0);
> +	if (irq >= 0) {
> +		dev_dbg(&pdev->dev, "using PIO mode\n");
> +		dev->use_pio = true;
> +
> +		ret = devm_request_irq(&pdev->dev, irq, i2s_irq_handler, 0,
> +				pdev->name, dev);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "failed to request irq\n");
> +			return ret;
> +		}
> +	}

This also seems wrong.  We're forcing PIO if an interrupt is provided
rather than based on DMA being configured which means that if the
interrupt is wired up and happens to be described in DT we'll get worse
performance.  People should be able to just describe the system without
worrying about this, and we might find some other use for the interrupts
in future.  Indeed right now it would probably be reasonable to use the
error interrupts all the time if they're available.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists