On Wed, Aug 27, 2014 at 09:37:37PM +0100, Mark Brown wrote:
On Thu, Aug 21, 2014 at 06:20:43PM +0530, Subhransu S. Prusty wrote:
+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?
I think we can use kfree() inside a spinlock, but will move it out of it.
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.
"freed" is a block which is passed by the caller to be freed up. Will add a comment.
- /* 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.
Will remove.
- /* 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.
Ok.
- 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()?
Ok.
--