Hi Arnd,
thanks for your review.

On 21 February 2013 14:13, Arnd Bergmann [off-list ref] wrote:

On Thursday 21 February 2013, Javier Martin wrote:

quoted

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

quoted

+#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.

quoted

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

quoted

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

quoted

+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