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

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