* Evgeniy Polyakov | 2009-03-18 01:42:43 [+0300]:
>Hi.
Hi Evgeniy,
>On Tue, Mar 17, 2009 at 10:58:44PM +0100, Sebastian Andrzej Siewior (arm-kernel@xxxxxxxxxxxxxxxx) wrote:
>> +struct crypto_priv {
>
>Please use less generic names over the file.
will do.
>> + void __iomem *reg;
>> + void __iomem *sram;
>> + int irq;
>> + struct task_struct *queue_th;
>> +
>> + spinlock_t lock;
>> + struct crypto_queue queue;
>> + enum engine_status eng_st;
>> + struct ablkcipher_request *cur_req;
>> + struct req_progress p;
>> +};
>> +
>> +static struct crypto_priv *cpg;
>> +
>
>This rises several questions: why some of its fields are accessed under
>the lock and others are modified without. Some of them are only used in
>the kernel thread, while others are used in request context.
>Please document locking bits in the code.
Will do. Right now, I could switch to the _bh spinlock since it is only
required to access the queue. Earlier I planned to enqueue the first
request directly on the hw but then I got into the scatter walk.....
>> +static void reg_write(void __iomem *mem, u32 val)
>> +{
>> + __raw_writel(val, mem);
>> +}
>> +
>> +static u32 reg_read(void __iomem *mem)
>> +{
>> + return __raw_readl(mem);
>> +}
>> +
>
>Seems like you do not like underscores otherwise you would use those
>functions directly.
Yes, I could do so. I wasn't sure whether those are the correct
functions to access the memory. If there were for some reason I could
replace them in one place. I switch to __.
>> +
>> +#define MAX_REQ_SIZE (8000)
>> +
>
>Parentheses are not needed.
yup.
>> +irqreturn_t crypto_int(int irq, void *priv)
>> +{
>> +// struct crypto_priv *cp = priv;
>> + u32 val;
>> +
>> + val = reg_read(cpg->reg + SEC_ACCEL_INT_STATUS);
>> + reg_write(cpg->reg + SEC_ACCEL_INT_MASK, 0);
>
>Why do you ack interrupt before checking if interrupt belongs to
>this driver?
I don't know. I better fix this.
>> + if (!(val & SEC_INT_ACCEL0_DONE))
>> + return IRQ_NONE;
>> +
>> + BUG_ON(cpg->eng_st != engine_busy);
>> + cpg->eng_st = engine_w_dequeue;
>> + wake_up_process(cpg->queue_th);
>> + return IRQ_HANDLED;
>> +}
>
>--
> Evgeniy Polyakov
Thanks for looking over.
Sebastian
--
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
