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

[Mark Brown]

On Thu, Aug 27, 2015 at 01:15:51PM +0100, Qais Yousef wrote:
> On 08/26/2015 07:37 PM, Mark Brown wrote:
> >On Mon, Aug 24, 2015 at 01:39:12PM +0100, Qais Yousef wrote:

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

So we have hard coded numbers in the firmware that we need in the driver
but we can't read those numbers back from the firmware.  That's sad.

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

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

It's very bad practice to bury lock taking in with the variable
declaration.

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

You don't need any ifdefs for the include, you can just include the
header.

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

You should keep strings that are displayed as a single string together
but if you are splitting something in the output then that split won't
hurt grepping in the source.

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

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

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

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

Pointer arithmetic or converting it to a number?

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

Attachment: signature.asc
Description: Digital signature