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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3400 bytes --]

On Fri, Aug 28, 2015 at 10:22:57AM +0100, Qais Yousef wrote:
> On 08/27/2015 04:32 PM, Mark Brown wrote:
> >On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote:

> >>>>+#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?

> >It should be trivial to make things configurable shouldn't it?

> Yes and I am all with configurability but I don't think it makes sense here.
> AXD will always have its own MMU and will not share virtual address space,
> so the possibility of us wanting to move this somewhere else is really very
> thin. Also I don't think this is the kind of detail we need to concern the
> user with. I'll see if I can make the binary header parsing more flexible so
> we can add more info like this and the one above in the future and be more
> future proof.

So this is a virtual address in the memory map of the DSP?  That's not
what I thought it was.

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

> >Why do you get a warning from that?  Perhaps the warnings are trying to
> >tell us something...

> Because we try to assign an int to a pointer. So the error is 'makes pointer
> from integer without a cast'. To convert an address from uncached to cached
> we need to convert to an int as in MIPS it's a case of adding or subtracting
> a value then convert this value back to it's original form.
> I'll see if I can find a better way to fix the coherency issue when we copy
> through uncached.

Why can't you just use pointer arithmmetic?

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

> >Uncompress to a buffer then write that buffer to the final destination?

> Yes but the binary could be multi MiB so we can't get a temp buffer that
> large. If the crypto API allows decompressing in steps we can use a small
> buffer to move the data iteratively. I'll have a look.

A few megabytes doesn't seem like that big an ask (it's not *nice* but
it's doable with vmalloc()).  Iteratively copying is nicer though.

> >>>>+	/* 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()?
> >No, I'm suggesting tearing down the kernel side of any work and kicking
> >errors back to userspace if it continues to interact with anything that
> >was ongoing.

> OK. This is what we do (see my other email about abort). I'll have a think
> for a way to get rid of the ssleep(). Any ideas are welcome.

Just delete it?

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