On Thu, Aug 21, 2014 at 06:20:40PM +0530, Subhransu S. Prusty wrote:
+static irqreturn_t intel_sst_irq_thread_mrfld(int irq, void *context) +{
- struct intel_sst_drv *drv = (struct intel_sst_drv *) context;
- struct ipc_post *__msg, *msg = NULL;
- unsigned long irq_flags;
- if (list_empty(&drv->rx_list))
return IRQ_HANDLED;- spin_lock_irqsave(&drv->rx_msg_lock, irq_flags);
- list_for_each_entry_safe(msg, __msg, &drv->rx_list, node) {
This still has the same weird empty check then lock thing that I queried last time - even if it's somehow safe (I'm not convinced it is) it looks buggy to check if the list is empty outside the lock.
One of the reason review has been slow here is that there seem to be rather a lot of review comments which have not been addressed, though kernel summit was part of it too as was not having PATCH in the subject.
- default:
pr_err("SST Driver capablities missing for pci_id: %x",sst->pci_id);
Still using pr_ prints instead of dev_ as well it seems.
+static int intel_sst_probe(struct pci_dev *pci,
const struct pci_device_id *pci_id)+{
- pr_debug("Probe for DID %x\n", pci->device);
If you used dev_ printks this (unneeded) log would get a sensibly formatted device name automatically!
- sst_drv_ctx = devm_kzalloc(&pci->dev, sizeof(*sst_drv_ctx), GFP_KERNEL);
- if (!sst_drv_ctx)
return -ENOMEM;
- if (0 != sst_driver_ops(sst_drv_ctx))
return -EINVAL;
To quote my previous review:
| if (!sst_driver_ops())
- pr_info("Got drv data max stream %d\n",
sst_drv_ctx->info.max_streams);
I can't see anything betwen the alocation above and here which might initialize anything in info.
- /* Init the device */
- ret = pci_enable_device(pci);
- if (ret) {
pr_err("device can't be enabled\n");
Print the return code.
- pr_info("%s successfully done!\n", __func__);
Don't include noisy prints like this.
+do_unmap_dram:
- iounmap(sst_drv_ctx->dram);
+do_unmap_iram:
- iounmap(sst_drv_ctx->iram);
+do_unmap_sram:
- iounmap(sst_drv_ctx->mailbox);
+do_unmap_shim:
- iounmap(sst_drv_ctx->shim);
Use the pcim_ functions and avoid the need for this cleanup.
- iounmap(sst_drv_ctx->dram);
- iounmap(sst_drv_ctx->iram);
- iounmap(sst_drv_ctx->mailbox);
- iounmap(sst_drv_ctx->shim);
- flush_scheduled_work();
- destroy_workqueue(sst_drv_ctx->post_msg_wq);
You're destroying the workqueue after unmapping all the regions - that doesn't seem right, what if something was running?
+static inline int sst_pm_runtime_put(struct intel_sst_drv *sst_drv) +{
- int ret;
- pm_runtime_mark_last_busy(sst_drv->dev);
- ret = pm_runtime_put_autosuspend(sst_drv->dev);
- if (ret < 0)
return ret;- return 0;
+}
There's really nothing device specific about this - if this helper is useful please add it to pm_runtime.h (it seems like a common enough pattern).
+#define MAX_BLOCKS 15
That's a generic name especially in a header...
+static inline unsigned int sst_assign_pvt_id(struct intel_sst_drv *sst_drv_ctx) +{
- unsigned int local;
- spin_lock(&sst_drv_ctx->block_lock);
- sst_drv_ctx->pvt_id++;
- if (sst_drv_ctx->pvt_id > MAX_BLOCKS)
sst_drv_ctx->pvt_id = 1;- local = sst_drv_ctx->pvt_id;
- spin_unlock(&sst_drv_ctx->block_lock);
- return local;
+}
The comments about overflow continue to apply here.
In general there's also a *lot* of code in this header most of which isn't obviously fast paths or anything and so should probably in regular C files, the header is currently bigger than the source file.
+static inline int sst_shim_write(void __iomem *addr, int offset, int value) +{
- writel(value, addr + offset);
- return 0;
+}
I'd have expected a helper like this to take a driver object rather than a raw address (this is something which could reasonably be in a header BTW).