Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files

[Qais Yousef <qais.yousef@imgtec.com>]

On 08/29/2015 10:47 AM, Mark Brown wrote:
> On Thu, Aug 27, 2015 at 03:21:17PM +0100, Qais Yousef wrote:
>> On 08/26/2015 07:43 PM, Mark Brown wrote:
>>> On Mon, Aug 24, 2015 at 01:39:14PM +0100, Qais Yousef wrote:
>>>> +	/*
>>>> +	 * must ensure we have one access at a time to the queue and rd_idx
>>>> +	 * to be preemption and SMP safe
>>>> +	 * Sempahores will ensure that we will only read after a complete write
>>>> +	 * has finished, so we will never read and write from the same location.
>>>> +	 */
>>> In what way will sempahores ensure that we will only read after a
>>> complete write?
>> This comment needs fixing. What it is trying to say is that if we reached
>> this point of the code then we're certainly allowed to modify the buffer
>> queue and {rd, wr}_idx because the semaphore would have gone to sleep
>> otherwise if the queue is full/empty.
>> Should I just remove the reference to Semaphores from the comment or worth
>> rephrasing it?
> Any comments need to be comprehensible.
>
>> Would it be better to rename {rd, wr}_{idx, sem} to {take, put}_{idx, sem}?
> I'm not sure that helps to be honest, the main issue is that the scheme
> is fairly complex and unexplained.
>
>>>> +	buf = bufferq->queue[bufferq->rd_idx];
>>> So buffers are always retired in the same order that they are acquired?
>> I don't think I get you here. axd_bufferq_take() and axd_bufferq_put() could
>> be called in any order.
> Retiring buffers in the order they are acquired means that buffers are
> always freed in the same order they are acquired, you can't free one
> buffer before another that was acquired first.
>> What this code is trying to do is make a contiguous memory area behave as a
>> ring buffer. Then this ring buffer behave as a queue. We use semaphore
>> counts to control how many are available to take/put. rd_idx and wr_idx
>> should always point at the next location to take/put from/to.
>> Does this help answering your question?
> No.  Why are we doing this?  Essentially all ALSA buffers are ring
> buffers handled in blocks, why does this one need this complex locking
> scheme?

There are 2 sides to this. The ALSA/driver iface and the driver/firmware 
one. The ALSA/driver iface is called from ALSA ops but the 
driver/firmware is handled by the interrupt and workqueues. The code is 
trying to deal with this concurrency. Also once AXD consumed a buffer it 
sends back an interrupt to the driver that it can reuse it, there's no 
guarantee that this returned buffer is in the same order it was sent.

I hear you though. Let me see how I can simplify this :-)

>>>> +void axd_bufferq_abort_put(struct axd_bufferq *bufferq)
>>>> +{
>>>> +	if (axd_bufferq_is_full(bufferq)) {
>>>> +		bufferq->abort_put = 1;
>>>> +		up(&bufferq->wr_sem);
>>>> +	}
>>>> +}
>>> These look *incredibly* racy.  Why are they here and why are they safe?
>> If we want to restart the firmware we will need to abort any blocking reads
>> or writes for the user space to react. I also needed that to implement
> I'm not questioning what the functionns are doing, I'm questioning their
> implementation - it doesn't look like they are safe or reliable.  They
> just set a flag, relying on something else to notice that the flag has
> been set and act appropriately before it goes on and corrupts data.
> That just screams concurrency issues.

OK. I'll see how I can rework the code to address all of your comments.

Thanks,
Qais

>> nonblocking access in user space when this was a sysfs based driver. It was
>> important then to implement omx IL component correctly.
> Nobody cares about OMX ILs in mainline or sysfs based interfaces.
>
>> Do I need to support nonblock reads and writes in ALSA? If I use SIGKILL as
>> you suggested in the other email when restarting and nonblock is not
>> important then I can remove this.
> It would be better to support non blocking access.