Re: [PATCH 3/7] Asoc: sti: add CPU DAI driver for playback

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3566 bytes --]

On Mon, Apr 27, 2015 at 03:58:56PM +0200, Arnaud Pouliquen wrote:
> On 04/24/2015 08:15 PM, Mark Brown wrote:
> >On Tue, Apr 14, 2015 at 03:35:27PM +0200, Arnaud Pouliquen wrote:

Please delete unneeded context from your mails and fix your mailer to
format things normally.  I'm not entirely sure what it's doing, it seems
to be be that it's not leaving blanks between paragraphs and wrapping at
odd and inconsistent places.  It is very hard to read your messages as a
result.

> >There's a lot of very coarse grained spinlock usage in this driver which
> >I'm having a hard time understanding, at the very least the decisions
> >about locking need to be documented much more clearly and I suspect that
> >either things need to be finer grained, we should be using mutexes more
> >or both.

> This part is linked to the standby mode described in binding ( patch[1/7])
> it manage a runtime suspend, because ASOC runtime suspend is dedicated to
> DAPM.
> As you recommend i will try to change it by a DAPM linked to CPU_DAI. Just
> need to find a wait
> to retrieve CPU_DAI context in DAPM..
> Concerning spinlock
> I use it to protect context ( called by IRQ, on suspend, by user...). As
> some code is called in atomic i can not use mutex.
> I will review it and document it.

Please note my comments about these locks being very coarse grained - I
suspect that the spinlock is covering too much.

> >>+	/* Get interrupt status & clear them immediately */
> >>+	preempt_disable();
> >>+	status = GET_UNIPERIF_ITS(player);
> >>+	SET_UNIPERIF_ITS_BCLR(player, status);
> >>+	preempt_enable();

> >preempt_disable()?  Why is this being done, if you're doing unusual
> >stuff like this the code needs to be very clear about what the locking
> >rules are?

> This is used to be sure to not miss an interrupt. If preempted between both
> instruction i can clear an interruption flag before treat it. In this case i
> will receive
> a second interrupt with all flag to 0.

That doesn't sound right, the interrupt appears to be write to clear so
if we get a second interrupt between the read and write surely we'd not
get another spurious interrupt?  It would look like we were acknowleding
the second raise.  It sounds like something else is going on here.

In any case the spurious interrupt doesn't seem like it should happen a
lot, nor like it should be especially serious if we can handle it.

> >>+	snd_pcm_stream_lock(player->substream);

> >Again please explain the locking if we're doing something unusual.
> This protect from a race condition, if application request a stop while we
> receive Error from IP.
> I call it here to avoid toprotect all snd_pcm_stop calls... but i can
> protect only the snd_pcm_stop
> calls if more clear

Please do something to make it clear what you're doing.

> >>+	/* Check for fifo error (under run) */
> >>+	if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(player))) {
> >>+		dev_err(player->dev, "FIFO underflow error detected");
> >>+
> >>+		/* Interrupt is just for information when underflow recovery */
> >>+		if (player->info->underflow_enabled) {
> >>+			/* Update state to underflow */
> >>+			player->state = UNIPERIF_STATE_UNDERFLOW;

> >Why would underflow recovery be optional?

> To propose 2 strategies:
> - stop on underrun ( because plop will occurs)
> - try to recover it:  time is recovered when new data is available ( sample
> dropping)

Well, the standard behaviour is to halt on error (so that's what the
rest of the stack will expect) and it doesn't sound like the recovery is
really that great anyway.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]