Re: [v3 02/11] ASoC: Intel: mrfld - Add DSP load and management

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4453 bytes --]

On Thu, Aug 21, 2014 at 06:20:41PM +0530, Subhransu S. Prusty wrote:

> +#ifndef CONFIG_X86_64
> +#define MEMCPY_TOIO memcpy_toio
> +#else
> +#define MEMCPY_TOIO memcpy32_toio
> +#endif

This is a counterintuitve way to write this (with a reversed test) and
surely if this is needed it would be better done by the architecture
headers defining memcpy32_toio on 32 bit platforms too?  I'm also not
immediately seeing memcpy32_toio() defined when I grep except as a local
definition in the Atmel NAND driver.  Oh, except...

> +/**
> + * memcpy32_toio: Copy using writel commands
> + *
> + * This is needed because the hardware does not support
> + * 64-bit moveq insructions while writing to PCI MMIO
> + */
> +void memcpy32_toio(void *dst, const void *src, int count)
> +{
> +	int i;
> +	const u32 *src_32 = src;
> +	u32 *dst_32 = dst;
> +
> +	for (i = 0; i < count/sizeof(u32); i++)
> +		writel(*src_32++, dst_32++);
> +}

...which is very similar but missing static, __iomem and const
annotations.  I'd suggest lifting the existing version into generic
code.

> +#define SST_CALC_DMA_DSTN(lpe_viewpt_rqd, ia_viewpt_addr, elf_paddr, \
> +			lpe_viewpt_addr) ((lpe_viewpt_rqd) ? \
> +		elf_paddr : (ia_viewpt_addr + elf_paddr - lpe_viewpt_addr))

Can we make this a function please?

> +static int sst_fill_dstn(struct intel_sst_drv *sst, struct sst_info info,
> +		Elf32_Phdr *pr, void **dstn, unsigned int *dstn_phys,
> +		int *mem_type)
> +{
> +	/* work arnd-since only 4 byte align copying is only allowed for ICCM */

around.

> +static inline int sst_validate_elf(const struct firmware *sst_bin, bool dynamic)

Why is this inlined?  My previous comments about this looking very
generic also still remain unaddressed - it looks a lot like the
remoteproc ELF code for example though a bit less thorough.  I'm really
not thrilled about the idea of duplicating something like ELF parsing,
it seems like an excellent candidate for a library.

> + * Adds the node to the list after required fields
> + * are populated in the node
> + */
> +
> +static int sst_fill_memcpy_list(struct list_head *memcpy_list,
> +			void *destn, const void *src, u32 size, bool is_io)

Extra blank line.

> +	for (count = 0; count < module->blocks; count++) {

> +		block = (void *)block + sizeof(*block) + block->size;

We're not doing any validation that the data we're reading from the
firmware file isn't corrupt here - we're just trusting both the number
of blocks and the sizes of the blocks.  We should be a bit less trusting
about userspace data.

> +static void sst_memcpy_free_lib_resources(struct intel_sst_drv *sst_drv_ctx)
> +{
> +	struct sst_memcpy_list *listnode, *tmplistnode;
> +
> +	pr_debug("entry:%s\n", __func__);
> +
> +	/*Free the list*/
> +	if (!list_empty(&sst_drv_ctx->libmemcpy_list)) {

Why the list_empty() check here?

> +		list_for_each_entry_safe(listnode, tmplistnode,
> +				&sst_drv_ctx->libmemcpy_list, memcpylist) {
> +			list_del(&listnode->memcpylist);
> +			kfree(listnode);
> +		}
> +	}
> +}
> +
> +void sst_memcpy_free_resources(struct intel_sst_drv *sst_drv_ctx)
> +{
> +	struct sst_memcpy_list *listnode, *tmplistnode;
> +
> +	pr_debug("entry:%s\n", __func__);
> +
> +	/*Free the list*/
> +	if (!list_empty(&sst_drv_ctx->memcpy_list)) {

So we have a memcpy() list and a libmemcpy() list?  That seems confusing
and redundant...

> +	if (ctx->sst_state != SST_RESET ||
> +			ctx->fw_in_mem != NULL)
> +		goto out;

Is this perhaps an error conditon we should complain about?

> +	if (ctx->info.use_elf == true)
> +		ret = sst_validate_elf(fw, false);

I can't find anything that ever sets use_elf...

> +static int sst_request_fw(struct intel_sst_drv *sst)
> +{
> +	int retval = 0;
> +	char name[20];
> +	const struct firmware *fw;
> +
> +	snprintf(name, sizeof(name), "%s%04x%s", "fw_sst_",
> +				sst->pci_id, ".bin");
> +	pr_debug("Requesting FW %s now...\n", name);
> +
> +	retval = request_firmware(&fw, name, sst->dev);

There was a name in the driver struct for async requests and this does
appear to duplicate a lot of code from the async callback...

> +static inline void print_lib_info(struct snd_sst_lib_download_info *resp)
> +{
> +	pr_debug("codec Type %d Ver %d Built %s: %s\n",
> +		resp->dload_lib.lib_info.lib_type,
> +		resp->dload_lib.lib_info.lib_version,
> +		resp->dload_lib.lib_info.b_date,
> +		resp->dload_lib.lib_info.b_time);
> +}

sysfs or debugfs?  There don't seem to be any users of this anyway...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]