linux-kernel - Re: [PATCH 3/3 v2] sound/soc/lapis: add platform driver for ML7213 IOH I2S

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111222005816.GB27144@opensource.wolfsonmicro.com>
Date:	Thu, 22 Dec 2011 00:58:16 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Tomoya MORINAGA <tomoya.rohm@...il.com>
Cc:	Liam Girdwood <lrg@...com>, Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.de>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Dimitris Papastamos <dp@...nsource.wolfsonmicro.com>,
	Mike Frysinger <vapier@...too.org>,
	Daniel Mack <zonque@...il.com>, alsa-devel@...a-project.org,
	linux-kernel@...r.kernel.org, qi.wang@...el.com,
	yong.y.wang@...el.com, joel.clark@...el.com, kok.howg.ewe@...el.com
Subject: Re: [PATCH 3/3 v2] sound/soc/lapis: add platform driver for ML7213
	IOH I2S

On Wed, Dec 21, 2011 at 08:22:56PM +0900, Tomoya MORINAGA wrote:
> 2011/12/20 Mark Brown <broonie@...nsource.wolfsonmicro.com>:
> > On Tue, Dec 20, 2011 at 11:45:44AM +0900, Tomoya MORINAGA wrote:

> >> +     iowrite32(I2S_TX_FINT | I2S_TX_AFINT | I2S_TX_EINT | I2S_TX_AEINT,
> >> +             i2s_data->iobase + I2SISTTX_OFFSET + offset);

> > This appears to unconditionally acknowledge all interrupts, generally
> > you should only acknowledge interrupts that have been handled.
> > Otherwise it's possible that interrupts may be dropped if they're
> > flagged between the status read and acknowledgement write.

> I can understand your saying.
> If following your saying, only called by i2s_dma_tx_complete is enough.

> However I think adding clearing interrupt status at both before and
> after i2s communicating start/finish is better.

No, the issue is that you're acknowleding a hard coded set of
interrupts, not the interrupts that were actually handled.  The ordering
isn't really the issue, the issue is that you could acknowledge things
that weren't handled.  For example:

   1. ISR runs, reads status A
   2. Condition B happens
   3. ISR handles condition A.
   4. ISR acknowledges both A and B.

would mean that B would just get ignored.

> >> +static bool filter(struct dma_chan *chan, void *slave)

> > This needs a better name.  It's not namespaced and it's not clear what
> > it's filtering.

> In case of using Linux standard DMA API, function "filter" is the most
> popular name.
> Almost drivers which use standard use "filter".

I suspect they may have the filter immediately next to the code that
uses it, which certainly isn't the case here.

> >> +ioh_i2s_write(struct snd_pcm_substream *substream, const void *data, int len)
> >> +{
> >> +     int rem1;
> >> +     int rem2;

> > What is this supposed to do?  There's an awful lot of it, and it looks
> > like it's supposed to be rewriting the data format for some reason which
> > isn't something that a driver ought to be doing (apart from anything
> > else it does rather defeat the point of DMA).

> This driver has buffer which is for preventing noisy sound  by jitter
> So, this buffer is variable.

You're implementing some sort of custom buffering in your driver?  That
sounds terribly unidiomatic - pretty much all DMA drivers are very thin
and manage to work well, I'd expect this is masking some problems in the
code rather than anything else.  Can you provide more detail on what
this is working around?

> >> +     num = ((int)dma->tx_avail) / (I2S_AEMPTY_THRESH * 4);

> > That case looks *very* suspect.

> This is correct.
> Why do you think so ?

I can't tell what it's supposed to do, and casting that isn't super
clear especially casting to integer types tends to be a big red flag.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/