Re: [v3 01/11] ASoC: Intel: mrfld - add the dsp sst driver

[Mark Brown <broonie@kernel.org>]

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

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).

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

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