Hi Arnd,
thanks for your review.
On 21 February 2013 14:13, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> 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.
Ok. Looks cleaner this way.
>
>> +#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.
>
Great. Thank you for the tip.
>> +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.
You are right, dev->lock is only held by encrypt()/decrypt() callbacks
and some tasklets so spin_lock() and spin_lock_bh() seem suitable
here.
>> +
>> +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.
>
All right.
>> +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.
Please, take a look at 0/3 to discuss about this matter.
Regards.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
