Re: [v3 04/11] ASoC: Intel: sst: Add IPC handling

[Mark Brown <broonie@kernel.org>]

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

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

> +	spin_unlock_bh(&ctx->block_lock);
> +	pr_debug("Block not found or a response is received for a short message for ipc %d, drv_id %d\n",
> +			ipc, drv_id);

Is this not an error?

> +int sst_free_block(struct intel_sst_drv *ctx, struct sst_block *freed)
> +{
> +	struct sst_block *block = NULL, *__block;
> +
> +	pr_debug("in %s\n", __func__);
> +	spin_lock_bh(&ctx->block_lock);
> +	list_for_each_entry_safe(block, __block, &ctx->block_list, node) {
> +		if (block == freed) {
> +			list_del(&freed->node);
> +			kfree(freed->data);
> +			freed->data = NULL;
> +			kfree(freed);

Can you safely kfree() inside a spinlock?

> +			spin_unlock_bh(&ctx->block_lock);
> +			return 0;
> +		}
> +	}
> +	spin_unlock_bh(&ctx->block_lock);
> +	return -EINVAL;

I'd expect much louder complaints if we try to free something that's not
allocated - what happens if we end up reallocating something quickly and
then double freeing?  Better to complain if we hit such a code path.

> +	/* check busy bit */
> +	header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> +	if (header.p.header_high.part.busy) {
> +		spin_unlock_irqrestore(&sst_drv_ctx->ipc_spin_lock, irq_flags);
> +		pr_debug("Busy not free... post later\n");
> +		return;
> +	}
> +	/* copy msg from list */

Blank line here.

> +	/* check busy bit */
> +	header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> +	while (header.p.header_high.part.busy) {
> +		if (loop_count > 10) {
> +			pr_err("sst: Busy wait failed, cant send this msg\n");
> +			retval = -EBUSY;
> +			goto out;
> +		}
> +		cpu_relax();
> +		loop_count++;
> +		header.full = sst_shim_read64(sst_drv_ctx->shim, SST_IPCX);
> +	}

10 spins seems short but OK.

> +	pr_debug("sst: Post message: header = %x\n",
> +					msg->mrfld_header.p.header_high.full);
> +	pr_debug("sst: size = 0x%x\n", msg->mrfld_header.p.header_low_payload);
> +	if (msg->mrfld_header.p.header_high.part.large)
> +		memcpy_toio(sst_drv_ctx->mailbox + SST_MAILBOX_SEND,
> +			msg->mailbox_data,
> +			msg->mrfld_header.p.header_low_payload);
> +
> +	sst_shim_write64(sst_drv_ctx->shim, SST_IPCX, msg->mrfld_header.full);

Can we factor out the I/O bit with the non-blocking case here here?
It's just a small bit at the top of the function that's really
duplicated.

> +	case IPC_IA_FW_ASYNC_ERR_MRFLD:
> +		pr_err("FW sent async error msg:\n");
> +		for (i = 0; i < (data_size/4); i++)
> +			pr_err("0x%x\n", (*((unsigned int *)data_offset + i)));

print_hex_dump()?

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

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