Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1214602

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

Show all headers | View raw

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?

Would it be better to rename {rd, wr}_{idx, sem} to {take, put}_{idx, sem}?

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

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?

>
>> +int axd_bufferq_put(struct axd_bufferq *bufferq, char *buf, int buf_size)
>> +{
>> +	int ret;
>> +
>> +	if (!bufferq->queue)
>> +		return 0;
>> +
>> +	if (buf_size < 0)
>> +		buf_size = bufferq->stride;
> We've got strides as well?  What is that?

We break the contiguous buffer area allocated for us into smaller 
buffers separated by (or of size) stride.

>
>> +void axd_bufferq_abort_take(struct axd_bufferq *bufferq)
>> +{
>> +	if (axd_bufferq_is_empty(bufferq)) {
>> +		bufferq->abort_take = 1;
>> +		up(&bufferq->rd_sem);
>> +	}
>> +}
>> +
>> +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 nonblocking access in user space when this was a sysfs based 
driver. It was important then to implement omx IL component correctly.

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.

I just looked at the code history and I was in the past sending SIGBUS 
to the user if we needed to restart then I opted to the abort approach 
as it will allow the application to terminate gracefully as it should 
get EOF instead then and hide the need to restart the firmware in a 
better way. What do you think?

Thanks,
Qais
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 00/10] Add support for img AXD audio hardware decoder Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:40 +0200
  [PATCH 04/10] ALSA: axd: add fw binary header manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:40 +0200
  [PATCH 09/10] ALSA: axd: add alsa compress offload operations Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
  [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
    Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-08-26 20:50 +0200
      Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 16:30 +0200
        Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-08-29 11:50 +0200
          Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Qais Yousef <qais.yousef@imgtec.com> - 2015-09-01 12:10 +0200
            Re: [PATCH 05/10] ALSA: axd: add buffers manipulation files Mark Brown <broonie@kernel.org> - 2015-09-03 14:40 +0200
  [PATCH 08/10] ALSA: axd: add low level AXD platform setup files Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
  [PATCH 10/10] ALSA: axd: add Makefile Qais Yousef <qais.yousef@imgtec.com> - 2015-08-24 14:50 +0200
  Re: [PATCH 00/10] Add support for img AXD audio hardware decoder Mark Brown <broonie@kernel.org> - 2015-08-26 20:10 +0200
    Re: [PATCH 00/10] Add support for img AXD audio hardware decoder Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 11:10 +0200
  Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-08-26 20:40 +0200
    Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 14:20 +0200
      Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-08-27 17:40 +0200
        Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Qais Yousef <qais.yousef@imgtec.com> - 2015-08-28 11:30 +0200
          Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver Mark Brown <broonie@kernel.org> - 2015-09-03 14:50 +0200
  Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-08-26 21:20 +0200
    Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Qais Yousef <qais.yousef@imgtec.com> - 2015-08-27 17:50 +0200
      Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-08-29 12:20 +0200
        Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Qais Yousef <qais.yousef@imgtec.com> - 2015-09-01 12:50 +0200
          Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving  axd cmds Mark Brown <broonie@kernel.org> - 2015-09-03 14:50 +0200

csiph-web