Re: [PATCH] AMCC Crypto4xx Device Driver v4]

[James Hsiao <jhsiao@amcc.com>]

Hi Kim,


I try to address some of the comments. I am not mentioning things that I
agree with you.


> + */
> +static int get_sg_count(struct scatterlist *sg_list, int nbytes)
> +{
> +     struct scatterlist *sg = sg_list;
> +     int sg_nents = 0;
> +
> +     while (nbytes) {
> +             sg_nents++;
> +             if (sg->length > nbytes)
> +                     break;

this is slightly different - this condition shouldn't need checking
here - see [1] below..

> +             nbytes -= sg->length;
> +             sg = sg_next(sg);
> +     }
> +
> +     return sg_nents;
> +}

Without the check, nbytes could become negative. The aead test case
with .np will crash(ie. gcm tests), those test  have sg->length >
nbytes.


About aad_len, we didn't release code that use aad yet. We did test this
function with aad_len none zero(the gcm tests).

About the irq_disable or spin_lock. 
The driver could be used by a kernel thread and esp4 at same time. As I
know process is preemptable. When the driver is used by a process it is
possible to be preempted. The hardware require scatter/gather descriptor
to be consecutive. So, if the process get a gather descriptor and then
it is preempted by another process or esp4 which get a gather descriptor
and return to the original process, the origianl process could get a non
consecutive gather descriptor.   
So, if spin_lock is recommended then I have to use spin_lock_irq_save,
which use irq_disable too. Do you think that is acceptable? 

Thanks and regards

James