lkml.org 
[lkml]   [2015]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 05/10] ALSA: axd: add buffers manipulation files
    Date
    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.



    \
     
     \ /
      Last update: 2015-09-01 12:21    [W:2.651 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site