Re: [PATCH 2/3] crypto: sahara: Add driver for SAHARA2 accelerator.

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 21 February 2013, Javier Martin wrote:
> +
> +struct sahara_dev {
> +	struct device		*device;
> +	void __iomem		*regs_base;
> +	struct clk		*clk_ipg;
> +	struct clk		*clk_ahb;
> +
> +	struct sahara_ctx	*ctx;
> +	spinlock_t		lock;
> +	struct crypto_queue	queue;
> +	unsigned long		flags;
> +
> +	struct tasklet_struct	done_task;
> +	struct tasklet_struct	queue_task;
> +
> +	struct sahara_hw_desc	*hw_desc[SAHARA_MAX_HW_DESC];
> +	dma_addr_t		hw_phys_desc[SAHARA_MAX_HW_DESC];
> +
> +	u8			*key_base;
> +	dma_addr_t		key_phys_base;
> +
> +	u8			*iv_base;
> +	dma_addr_t		iv_phys_base;
> +
> +	struct sahara_hw_link	*hw_link[SAHARA_MAX_HW_LINK];
> +	dma_addr_t		hw_phys_link[SAHARA_MAX_HW_LINK];
> +
> +	struct ablkcipher_request *req;
> +	size_t			total;
> +	struct scatterlist	*in_sg;
> +	unsigned int		nb_in_sg;
> +	struct scatterlist	*out_sg;
> +	unsigned int		nb_out_sg;
> +
> +	u32			error;
> +	struct timer_list	watchdog;
> +};
> +
> +static struct sahara_dev *dev_ptr;

Please remove this global device pointer, it should not be needed, since you
can store the pointer in the context object.


> +#ifdef DEBUG
> +
> +static char *sahara_state[4] = { "Idle", "Busy", "Error", "HW Fault" };
> +
> +static void sahara_decode_status(struct sahara_dev *dev, unsigned int status)
> +{
> +	u8 state = SAHARA_STATUS_GET_STATE(status);

You can simplify the code a lot if you replace the #ifdef around the function
with an

	if (!IS_ENBLED(DEBUG))
		return;

at the start of the function. That will lead to gcc completely removing the
code an everything referenced by it.


> +static void sahara_aes_done_task(unsigned long data)
> +{
> +	struct sahara_dev *dev = (struct sahara_dev *)data;
> +	unsigned long flags;
> +
> +	dma_unmap_sg(dev->device, dev->out_sg, dev->nb_out_sg,
> +		DMA_TO_DEVICE);
> +	dma_unmap_sg(dev->device, dev->in_sg, dev->nb_in_sg,
> +		DMA_FROM_DEVICE);
> +
> +	spin_lock_irqsave(&dev->lock, flags);
> +	clear_bit(FLAGS_BUSY, &dev->flags);
> +	spin_unlock_irqrestore(&dev->lock, flags);
> +
> +	dev->req->base.complete(&dev->req->base, dev->error);
> +}

Does dev->lock have to be irq-disabled? You don't seem to take it
from an interrupt handler.

Also, when you know that code is called without irqs enabled and
you just want to disable them, you can use the cheaper spin_lock_irq()
rather than spin_lock_irqsave().

In short, use either spin_lock_irq or spin_lock here. When protecting
against a tasklet, you will need spin_lock_bh.

> +
> +void sahara_watchdog(unsigned long data)
> +{
> +	struct sahara_dev *dev = (struct sahara_dev *)data;
> +	unsigned int err = sahara_read(dev, SAHARA_REG_ERRSTATUS);
> +#ifdef DEBUG
> +	unsigned int stat = sahara_read(dev, SAHARA_REG_STATUS);
> +	sahara_decode_status(dev, stat);
> +#endif

When you kill off the #ifdef, you should move this sahara_read()
call into the sahara_decode_status() function so it gets
compiled conditionally.

> +static struct platform_device_id sahara_platform_ids[] = {
> +	{ .name = "sahara-imx27" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, sahara_platform_ids);

You are missing the of_device_ids here. We probably don't even
need the platform_device_id list and can instead mandate that
this is only used by platforms that are already converted to
DT booting.

Please also add a DT binding document for this driver that mentions
the name and the resources that need to be provided.

	Arnd