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: <CANKRQnjCTAX02wJt7StjB0rXkg3Q6bPBRYCbkgq6vS5NFd97Fg@mail.gmail.com>
Date:	Thu, 22 Dec 2011 17:10:24 +0900
From:	Tomoya MORINAGA <tomoya.rohm@...il.com>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.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

2011/12/22 Mark Brown <broonie@...nsource.wolfsonmicro.com>:
> 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.
I can understand your saying.
However, in case of Tx interrupt, only ALMOST-EMPTY occurs. in case of
Rx, only Rx ALMOST-FULL occurs.
So as long as a user uses ML7213 I2S, your mentioned behavior is never happened.


>
>> >> +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.
They have the "filter" function immediately next to the code uses
"dma_request_channel".
Anyway, if you want me to change the name, I can change the name.
Please decide it.

>
>> >> +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?
Not sorted but queuing only.
In sound/voice control system, queuing is not rare, I think.
If necessary, though this method is very common, I can send the method
of the queue.

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

Currently, "tx_avail" is "int" type. So, "(int)" is redundant.
Maybe, once type of "tx_avail" is not "int" but something pointer type.
I'll modify like below.
num = dma->tx_avail / (I2S_AEMPTY_THRESH * 4);


thanks,
tomoya
--
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/