by Qais Yousef

On Wed, 26 Aug 2015, Qais Yousef wrote:
> On 08/26/2015 04:08 PM, Thomas Gleixner wrote:
> > IPI = Inter Processor Interrupt
> >
> > As the name says that's an interrupt which goes from one cpu to
> > another. So an IPI has a very clear target.
>
> OK understood. My interpretation of the processor here was the difference. I
> was viewing the whole linux cpus as one unit with regard to its coprocessors.

You can only view it this way if you talk about peripheral interrupts
which are not used as per cpu interrupts and can be routed to a single
cpu or a set of cpus via set_affinity.

> > Whether the platform implements IPIs via general interrupts which are
> > made affine to a particular cpu or some other specialized mechanism is
> > completely irrelevant. An IPI is not subject to affinity settings,
> > period.
> >
> > So if you want to use an IPI then you need a target cpu for that IPI.
> >
> > If you want something which can be affined to any cpu, then you need a
> > general interrupt and not an IPI.
>
> We are using IPIs to exchange interrupts. Affinity is not important to me.

That's a bold statement. If you chose CPU x as the target for the
interrupts received from the coprocessor, then you have pinned the
processing for this stuff on to CPU x. So you limit the freedom of
moving stuff around on the linux cpus.

And if your root irq controller supports sending normal device
interrupts in the same or a similar way as it sends IPIs you can spare
quite some extra handling on the linux side for receiving the
coprocessor interrupt, i.e. you can use just the bog standard
request_irq() mechanism and have the ability to set the affinity of
that interrupt from user space so you can move it to the core on which
your processing happens. Definitely simpler and more flexible, so I
would go there if the hardware allows.

But back to the IPIs. We need infrastructure and DT support to:

1) reserve an IPI

2) send an IPI

3) request/free an IPI

#1 We have no infrastructure for that, but we definitely need one.

We can look at the IPI as a single linux irq number which is
replicated on all cpu cores. The replication can happen in hardware
or by software, but that depends on the underlying root irq
controller. How that is implemented does not matter for the
reservation.

The most flexible and platform independent solution would be to
describe the IPI space as a seperate irq domain. In most cases this
would be a hierarchical domain stacked on the root irq domain:

[IPI-domain] --> [GIC-MIPS-domain]

on x86 this would be:

[IPI-domain] --> [vector-domain]

That needs some change how the IPIs which are used by the kernel
(rescheduling, function call ..) are set up, but we get a proper
management and collision avoidance that way. Depending on the
platform we could actually remove the whole IPI compile time
reservation and hand out IPIs at boot time on demand and
dynamically.

So the reservation function would be something like:

unsigned int irq_reserve_ipi(const struct cpumask *dest, void *devid);

@dest contains the possible targets for the IPI. So for generic
linux IPIs this would be cpu_possible_mask. For your coprocessor
the target would be a cpumask with just the bit of the coprocessor
core set. If you need to use an IPI for sending an interrupt from
the coprocessor to a specific linux core then @dest will contain
just that target cpu.

@devid is stored in the IPI domain for sanity checks during
operation.

The function returns a linux irq number or 0 if allocation fails.

We need a complementary interface as well, so you can hand back the
IPI to the core when the coprocessor is disabled:

void irq_destroy_ipi(unsigned int irq, void *devid);

To configure your coprocessor proper, we need a translation
mechanism from the linux interrupt number to the magic value which
needs to be written into the trigger register when the coprocessor
wants to send an interrupt or an IPI.

int irq_get_irq_hwcfg(unsigned int irq, struct irq_hwcfg *cfg);

struct irq_hwcfg needs to be defined, but it might look like this:

{
/* Generic fields */
x;
...
union {
mips_gic;
...
};
};

The actual hw specific value(s) need to be filled in from the irq
domain specific code.

#2 We have no generic mechanism for that either.

Something like this is needed:

void irq_send_ipi(unsigned int irq, const struct cpumask *dest,
void *devid);

@dest is for generic linux IPIs and can be NULL so the IPI is sent to
the core(s) which have been handed in at reservation time

@devid is used to sanity check the driver call.

So that finally will call down via a irq chip callback into the
code which sends the IPI.

#3 Now you get lucky, because we actually have an interface for this

request_percpu_irq()
free_percpu_irq()
disable_percpu_irq()
enable_percpu_irq()

Though there is a caveat. enable/disable_percpu_irq() must be
called from the target cpu, but that should be a solvable problem.

And at the IPI-domain side we need sanity checks whether the cpu
from which enable/disable is called is actually configured in the
reservation mask.

There are a few other nasty details, but that's not important for
the big picture.

As I said above, I really would recommend to avoid that if possible
because a bog standard device interrupt is way simpler to deal
with.

That's certainly not the quick and dirty solution you are looking for,
but exposing IPIs to drivers by anything else than a well thought out
infrastructure is not going to happen.

Thanks,

tglx

2015-08-27 02:22:53

by Jiang Liu

[permalink] [raw]

Subject: Re: [PATCH 01/10] irqchip: irq-mips-gic: export gic_send_ipi

On 2015/8/27 5:40, Thomas Gleixner wrote:
> But back to the IPIs. We need infrastructure and DT support to:
>
> 1) reserve an IPI
>
> 2) send an IPI
>
> 3) request/free an IPI
>
> #1 We have no infrastructure for that, but we definitely need one.
>
> We can look at the IPI as a single linux irq number which is
> replicated on all cpu cores. The replication can happen in hardware
> or by software, but that depends on the underlying root irq
> controller. How that is implemented does not matter for the
> reservation.
>
> The most flexible and platform independent solution would be to
> describe the IPI space as a seperate irq domain. In most cases this
> would be a hierarchical domain stacked on the root irq domain:
>
> [IPI-domain] --> [GIC-MIPS-domain]
>
> on x86 this would be:
>
> [IPI-domain] --> [vector-domain]
>
> That needs some change how the IPIs which are used by the kernel
> (rescheduling, function call ..) are set up, but we get a proper
> management and collision avoidance that way. Depending on the
> platform we could actually remove the whole IPI compile time
> reservation and hand out IPIs at boot time on demand and
> dynamically.
Hi Thomas,
Good point:) That will make the code more clear.
Thanks!
Gerry

2015-08-27 09:07:14

by Qais Yousef

[permalink] [raw]

Subject: Re: [PATCH 00/10] Add support for img AXD audio hardware decoder

On 08/26/2015 07:04 PM, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:39:09PM +0100, Qais Yousef wrote:
>
>> Qais Yousef (10):
>> irqchip: irq-mips-gic: export gic_send_ipi
>> dt: add img,axd.txt device tree binding document
>> ALSA: add AXD Audio Processing IP alsa driver
>> ALSA: axd: add fw binary header manipulation files
>> ALSA: axd: add buffers manipulation files
>> ALSA: axd: add basic files for sending/receiving axd cmds
>> ALSA: axd: add cmd interface helper functions
>> ALSA: axd: add low level AXD platform setup files
>> ALSA: axd: add alsa compress offload operations
>> ALSA: axd: add Makefile
> Please try to use subject lines matching the style for the subsystem, I
> very nearly deleted this unread because it looks like an ALSA patch
> series, not an ASoC one.

OK sorry about that. I'll fix this in the next series.

Thanks,
Qais

2015-08-27 12:15:56

by Qais Yousef

[permalink] [raw]

Subject: Re: [PATCH 03/10] ALSA: add AXD Audio Processing IP alsa driver

On 08/26/2015 07:37 PM, Mark Brown wrote:
> On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:
>
>> +#define THREAD_COUNT 4
> This is a very generic name that looks likely to collide with something
> else, please namespace.

OK.

>> +#define AXD_INPUT_DESCRIPTORS 10
>> +struct axd_input {
>> + struct axd_buffer_desc descriptors[AXD_INPUT_DESCRIPTORS];
>> +};
> Where do these numbers come from? Are they hardware limits or something
> else?

These numbers are what the firmware designed to work with. We had to set
a limit and we sought 10 to be a good one for our purposes. We don't
expect to need to change this number.

>> +/* this is required by MIPS ioremap_cachable() */
>> +#include <asm/pgtable.h>
> Don't work around this here, fix it in the relevant header.
>

Will do.

>> +#define AXD_BASE_VADDR 0xD0000000
> This sounds like something that is going to be platform dependant,
> should this be supplied from board configuration?

I don't expect this to change. Can we add the configuration later if we
hit the need to change it?

>> +extern struct snd_compr_ops axd_compr_ops;
> Prototype shared definitions in headers not in C files please so we know
> the definition matches.

OK.

>> +static struct snd_soc_dai_driver axd_dai[] = {
>> + {
> Why an array with only one entry?

Will fix it.

>> + if (!*offp) {
>> + unsigned int flags = axd_platform_lock();
>> + unsigned int log_offset = ioread32(log_addr);
>> + unsigned int log_wrapped = ioread32(log_addr + 8);
>> + char __iomem *log_buff = (char __iomem *)(log_addr + 12);
>> +
>> + /* new read from beginning, fill up our internal buffer */
>> + if (!log_wrapped) {
>> + memcpy_fromio(axd->log_rbuf, log_buff, log_offset);
>> + axd->log_rbuf_rem = log_offset;
>> + } else {
>> + char __iomem *pos = log_buff + log_offset;
>> + unsigned int rem = log_size - log_offset;
>> +
>> + memcpy_fromio(axd->log_rbuf, pos, rem);
>> + memcpy_fromio(axd->log_rbuf + rem, log_buff, log_offset);
>> + axd->log_rbuf_rem = log_size;
>> + }
>> + axd_platform_unlock(flags);
> I didn't see the lock being taken?

The lock is the first line in the block (unsigned int flags =
axd_platform_lock()). I'll tidy it up to make it more readable.

>> +static ssize_t axd_write_mask(struct file *filep,
>> + const char __user *buff, size_t count, loff_t *offp)
>> +{
>> + struct axd_dev *axd = filep->f_inode->i_private;
>> + unsigned int mask;
>> + char buffer[32] = {};
>> + int ret;
>> +
>> + /* ensure we always have null at the end */
>> + ret = copy_from_user(buffer, buff, min(31u, count));
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (!kstrtouint(buffer, 0, &mask))
>> + axd_write_reg(&axd->cmd, AXD_REG_DEBUG_MASK, mask);
> What are we writing here? If we're going behind the driver's back on
> something that might confuse it it's generally better to taint the
> kernel so we know dodgy stuff happened later on.

The debug mask will cause AXD firmware to provide more or less debug
information. We are not going behind the driver's back.

>> +static void axd_debugfs_create(struct axd_dev *axd)
>> +{
>> + axd->debugfs = debugfs_create_dir(dev_name(axd->dev), NULL);
>> + if (IS_ERR_OR_NULL(axd->debugfs)) {
>> + dev_err(axd->dev, "failed to create debugfs node\n");
>> + return;
>> + }
> It'd be nicer to create this under the relevant ASoC debugfs directory
> so it's easier to find.

Sure. I'll try to find an example and follow what it does.

>> +#ifdef CONFIG_CRYPTO_LZO
>> +#include <linux/crypto.h>
> This include should be with all the other includes, not down here.

Was trying to reduce the ifdefery. Will fix.

>> + size = axd->fw_size;
>> + cached_fw_base = (char *)CAC_ADDR((int)axd->fw_base_m);
>> + ret = crypto_comp_decompress(tfm, fw->data + 8,
>> + fw->size - 8, cached_fw_base, &size);
>> + if (ret)
>> + dev_err(axd->dev, "Failed to decompress the firmware\n");
> Print return codes if you get them.

Will do.

>> +
>> + if (size != axd->fw_size) {
>> + dev_err(axd->dev, "Uncompressed file size doesn't match reported file size\n");
>> + ret = -EINVAL;
>> + }
> Should we be checking this if the decompression failed?

Nope. I'll fix it.

>> +}
>> +#else /* !CONFIG_CRYPTO_LZO */
>> +static int decompress_fw(struct axd_dev *axd, const struct firmware *fw)
> Blank lines between things please.

OK.

>> +{
>> + dev_err(axd->dev, "The firmware must be lzo decompressed first, compile driver again with CONFIG_CRYPTO_LZO enabled in kernel or do the decompression in user space.\n");
> Please split this up into a few prints for wrapping, similarly in
> several other places.

OK. I thought the convention for strings to leave them as is to allow
grepping. I'll fix it.

>> + return -EIO;
> -ENOTSUPP.

OK.

>> + return -EIO;
>> + }
>> + /*
> More vertical blanks missing.

OK.

>
>> + * We copy through the cache, fw will do the necessary cache
>> + * flushes and syncing at startup.
>> + * Copying from uncached makes it more difficult for the
>> + * firmware to keep the caches coherent with memory when it sets
>> + * tlbs and start running.
>> + */
>> + memcpy_toio((void *)cached_fw_base, fw->data, fw->size);
> Why the cast here? I'm also not seeing where we handled the copying to
> I/O in the decompression case?

I couldn't avoid the cast. If cached_fw_base is 'void *' I'll get a
warning when initialising cached_fw_base from CAC_ADDR().
So I'll have to either cast here or there, I chose here.
If I pass axd->fw_base_m I encounter the issue described in the commit
message.

Good point. When decompressing crypto_comp_decompress() will write
directly to the memory. It is safe but it doesn't go through the correct
API. Not sure what I can do here.

>> + dev_info(axd->dev, "Loading firmware at 0x%p ...\n", axd->fw_base_m);
> This should be _dbg() at most, otherwise it's going to get noisy.
>
>> + t0_new_pc = (unsigned long) axd->fw_base_m + (t0_new_pc - AXD_BASE_VADDR);
> Those casts look fishy...

I am happy to try something else. axd->fw_base_m is of type void *
__iomem but we want to do some arithmetic on it.
Is there a better way to do it?

>
>> + for (i = 0; i < AXD_LDFW_RETRIES; i++) {
>> + ret = axd_wait_ready(axd_cmd->message);
>> + if (!ret) {
>> + /*
>> + * Let the firmware know the address of the buffer
>> + * region
>> + */
>> + ret = axd_write_reg(axd_cmd,
>> + AXD_REG_BUFFER_BASE, axd->buf_base_p);
>> + if (ret) {
>> + dev_err(axd->dev,
>> + "Failed to setup buffers base address\n");
> Again print errors please.
>
>> + goto out;
>> + }
>> + return 0;
>> +
>> + }
>> + }
> I'm not seeing any diagnostics if we fall out of the retry loop here?

Will add one.

>
>> +static void axd_reset(struct work_struct *work)
>> +{
>> + unsigned int major, minor, patch;
>> + int i;
>> +
>> + struct axd_dev *axd = container_of(work, struct axd_dev, watchdogwork);
>> +
>> +
>> + /* if we got a fatal error, don't reset if watchdog is disabled */
>> + if (unlikely(!axd->cmd.watchdogenabled))
>> + return;
> There's generally no need for unlikely() annotations outside of hot
> paths.

OK.

>> + /* stop the watchdog timer until we restart */
>> + del_timer(&axd->watchdogtimer);
> I'd expect del_timer_sync() to make sure that the timer stopped.

OK.

>> + if (!axd_get_flag(&axd->cmd.fw_stopped_flg)) {
>> + /* ping the firmware by requesting its version info */
>> + axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
>> + if (!major && !minor && !patch) {
>> + dev_warn(axd->dev, "Firmware stopped responding...\n");
>> + axd_set_flag(&axd->cmd.fw_stopped_flg, 1);
>> + } else {
>> + goto out;
>> + }
>> + }
> It might be useful to display the firmware version we loaded.

OK.

>
>> + axd_platform_print_regs();
>> + dev_warn(axd->dev, "Reloading AXD firmware...\n");
> This is going to get noisy and isn't adding much.

OK.

>
>> + /* wake up any task sleeping on command response */
>> + wake_up(&axd->cmd.wait);
>> + /* give chance to user land tasks to react to the crash */
>> + ssleep(2);
> This looks horribly racy, I'd expect us to be trashing and/or killing
> off any active work and resources here.

OK. I was trying to play nicely by giving the chance to userland to
repond to -ERESTART which would be sent from aborting any pending
reads/writes.

Are you suggesting to send SIGKILL using force_sig()?

>
>> +static void axd_watchdog_timer(unsigned long arg)
>> +{
>> + struct axd_dev *axd = (struct axd_dev *)arg;
>> +
>> + /* skip if watchdog is not enabled */
>> + if (unlikely(!axd->cmd.watchdogenabled))
>> + goto out;
>> +
>> + schedule_work(&axd->watchdogwork);
>> + return;
>> +out:
>> + mod_timer(&axd->watchdogtimer, jiffies + WATCHDOG_TIMEOUT);
>> +}
> So we have a timer that just schedules some work? Why not just
> schedule_delayed_work()?

Either wasn't there the time this was first written or was missed.
Either case thanks for the suggestion I'll change it.

>
>> + /*
>> + * Verify that the firmware is ready. In normal cases the firmware
>> + * should start immediately, but to be more robust we do this
>> + * verification and give the firmware a chance of 3 seconds to be ready
>> + * otherwise we exit in failure.
>> + */
>> + for (i = 0; i < AXD_LDFW_RETRIES; i++) {
>> + axd_cmd_get_version(&axd->cmd, &major, &minor, &patch);
>> + if (major || minor || patch) {
>> + /* firmware is ready */
>> + break;
>> + }
>> + /* if we couldn't read the version after 3 tries, error */
>> + if (i == AXD_LDFW_RETRIES - 1) {
>> + dev_err(axd->dev, "Failed to communicate with the firmware\n");
>> + ret = -EIO;
>> + goto error;
>> + }
>> + /* wait for 10 ms for the firmware to start */
>> + msleep(10);
>> + }
>> + dev_info(axd->dev, "Running firmware version %u.%u.%u %s\n",
>> + major, minor, patch, axd_hdr_get_build_str());
> Why is this code not shared with the restart case?

I didn't think it's necessary but I see how it can be better to move it
inside axd_fw_start() now.

>
>> + ret = of_property_read_u32_array(of_node, "gic-irq", val, 2);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "'gic-irq' parameter must be set\n");
>> + return ret;
>> + }
> This appears to have a DT binding but the binding is not documented.
> All new DT bindings must be documented. I'm concerned that some of the
> properties being read from DT may not be ideal here...

It is documented on a different patch. Sorry I think I just added the DT
maintainers to the CC for that patch and sent it to the ALSA list. I'll
be more careful in the next series to include all ALSA maintainers for
all patches.

Yes the DT will need to be enhanced. There's a separate discussion
generated by one of the patches on this series about how IPI should be
defined in DT.

See this

https://lkml.org/lkml/2015/8/26/713

Again sorry for not explicitly adding you to the CC list for all the
patches.

Thanks,
Qais

2015-08-27 14:21:21

by Qais Yousef

[permalink] [raw]

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

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

2015-08-27 15:32:54

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?

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

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

Attachments:

(No filename) (3.13 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-08-29 10:18:38

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 06/10] ALSA: axd: add basic files for sending/receiving axd cmds

On Thu, Aug 27, 2015 at 04:40:09PM +0100, Qais Yousef wrote:
> On 08/26/2015 08:16 PM, Mark Brown wrote:
> >On Mon, Aug 24, 2015 at 01:39:15PM +0100, Qais Yousef wrote:

> >>+unsigned long axd_cmd_get_datain_address(struct axd_cmd *cmd)
> >>+{
> >>+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> >>+
> >>+ return (unsigned long) axd->buf_base_m;
> >>+}
> >What's going on with these casts?

> As with the other cases. buf_base_m is void * __iomem but we want to do some
> arithmatic to help AXD start up and understand where it needs to run. I
> agree they don't look nice and if I can avoid them I'd be happy to do so.

C supports poinnter arithmetic...

> >>+static inline void axd_set_flag(unsigned int *flag, unsigned int value)
> >>+{
> >>+ *flag = value;
> >>+ smp_wmb(); /* guarantee smp ordering */
> >>+}

> >>+static inline unsigned int axd_get_flag(unsigned int *flag)
> >>+{
> >>+ smp_rmb(); /* guarantee smp ordering */
> >>+ return *flag;
> >>+}

> >Please use a normal locking construct rather than hand rolling
> >something, or alternatively introduce new generic operations. The fact
> >that you're hand rolling these things that have no driver specific
> >content is really worrying in terms of their safety.

> I need to check atomic_ops.txt again but I think atomic_t is not always smb
> safe. I definitely was running on a version of Meta archicture in the past
> where atomic_t wasn't always smp safe.

> I'll check if the rules have changed or something new was introduced to deal
> with this.

It is true that when using atomic_t with multiprocessor you still need
memory barriers but that doesn't mean atomics bring no benefit. But
that's not really the point here - the point is the more general one
that the whole idea of open coding memory barrier concurrency constructs
doesn't look great. It makes the code much more complex and error prone
compared to using normal locking and other concurrency constructs (which
Linux has a rich set of).

If we really need performance then it can make sense but I'm not seeing
anything here that appears to motivate this. In general all the
concurrency code looks much more complex than I would expect.

> >>+#define PIPE_STARTED 1
> >>+#define PIPE_RUNNING 2

> >Why is the case with in place buffers not a simple zero iteration loop?

> This is important when AXD is not consuming the data through I2S and
> returning them to Linux. What we're trying to deal with here is the firmware
> processed some data and expects Linux to consume whatever it has sent back
> to it. We want to ensure that if the user suddenly stopped consuming this
> data by closing the pipe to drop anything we receive back from AXD otherwise
> the workqueue would block indefinitely waiting for the user that disappeared
> to consume it causing a deadlock.

That doesn't seem to address the question...

> >>+ axd_platform_irq_ack();

> >When would this ever be called anywhere else? Just inline it (and it's
> >better practice to only ack things we handle...).

> It wouldn't be called anywhere else but its implementation could be platform
> specific that's why it's abstracted. At the moment it does nothing now we're
> using MIPS but we shouldn't assume that this will always be the case.
> The main purpose of this function is to deassert the interrupt line if the
> way interrrupts are wired for that platform required so. In the past we were
> running in hardware where interrupts are sent through special slave port and
> the interrupt required to be acked or deasserted.

This sounds like something that should be in the interrupt controller
implementation not the leaf driver, just remove this unless you're
actually abstracting something.

> >>+ flags = axd_platform_lock();
> >>+ int_status = ioread32(&cmd->message->int_status);
> >>+ iowrite32(0, &cmd->message->int_status);
> >>+
> >>+ if (!int_status)
> >>+ goto out;

> >This should cause us to return IRQ_NONE.

> I don't think it's necessary. It could happen that AXD sent a DATAIN
> interrupt and shortly after sent DATAOUT interrupt but the handler was
> running before the DATAOUT case is handled causing both interrupts to be
> handled in one go but the handler could be called again to find out that
> there's nothing to do.

Please implement your interrupt handler properly so that the genirq
error handling code can work and it is robust against things going
wrong in future. It's not like it's a huge amount of complex code.

> >>+ if (int_status & AXD_INT_ERROR) {
> >>+ struct axd_dev *axd = container_of(cmd, struct axd_dev, cmd);
> >>+ int error = ioread32(&cmd->message->error);
> >>+
> >>+ pr_debug("<---- Received error interrupt\n");
> >>+ switch (error) {
> >>+ default:
> >>+ case 0:
> >>+ break;

> >We just ignore these?

> Case 0 doesn't indicate anything anymore. I can print a warning about
> unexpected error code for the default case.

That's more what I'd expect, yes.

> >>+ /*
> >>+ * if we could lock the semaphore, then we're guaranteed that the
> >>+ * current rd_idx is valid and ready to be used. So no need to verify
> >>+ * that the status of the descriptor at rd_idx is valid.
> >>+ */
> >>+ spin_lock(&desc_ctrl->rd_lock);

> >It really feels like this locking is all complicated and fragile. I'm
> >not entirely sure the optimisation is worth it - are we really sending
> >compressed audio at such a high rate that it's worth having concurrency
> >handling that's hard to think about?

> This is similar to how the bufferq implementation work. What is the other
> alternative to this? We do want this to be as fast as possible.

Why not just reuse the bufferq implementation if that's what you want to
use? More generally most audio ring buffers just keep track of the last
place read or written and don't bother with semaphores (why do we even
need to block?). It's not just the semaphore you're using here but also
some non-atomic variables accessed with memory barriers and mutexes all
scattered over a very large block of code. It is far too much effort to
reason about what the locking scheme is supposed to be here to determine
if it is safe, and that's not going to get any easier when reviewing
future changes.

Trying to make something overly optimised at the expense of
comprehensibility and maintainability is not good, if there is a
pressing performance reason then by all means but that needs to be
something concrete not just a statement that we want things to run
faster.

> Maybe my use of the semaphore count to keep track of how many descriptors
> are available and cause the caller to block is the confusing part? Would
> better comments help?

Documentation would help somewhat but I really think this is far too
complicated for what it's trying to do. As far as I can tell all this
is doing is simple FIFO type tracking of where the last write and last
read in the buffer were (which is what most audio drivers do with their
data buffers). That should be something that can be done with something
more like just a single lock.

Based on some of your other comments I think this may have been
overengineered for some other APIs you were implementing but with the
ALSA API it should be possible to dramatically simplify it.

> >>+ /*
> >>+ * Based on the defined axd_pipe->buf_size and number of input pipes
> >>+ * supported by the firmware, we calculate the number of descriptors we
> >>+ * need to use using this formula:
> >>+ *
> >>+ * axd_pipe->buf_size * num_desc = total_size / num_inputs
> >>+ */
> >>+ num_desc = total_size / (cmd->num_inputs * axd_pipe->buf_size);

> >I'm not sure that was an especially tricky line of code to follow... am
> >I missing something here?

> The driver receive a pointer to a contiguous buffer area that it needs to
> divide it into buffers based on its size, number of pipes in the system, and
> the desired buffer size.

That is what I think the code is doing but apparently it's sufficiently
complex that it needs a five line comment including a rewritten version
of the equation?

Attachments:

(No filename) (7.86 kB)
signature.asc (473.00 B)
Digital signature Download all attachments