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

[javier Martin]

Hi Arnd,
thanks for your review.

On 21 February 2013 14:13, Arnd Bergmann <arnd at arndb.de> 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