Attached is the first of my fixes for the HIFN driver. I've
got it to a working state with tcrypt, IPsec and dm-crypt, a
few of the patches still need a bit work though, so I'll send
the ones that I already consider ready one by one.
Hi.
On Wed, May 07, 2008 at 02:19:36PM +0200, Patrick McHardy ([email protected]) wrote:
> hifn_setup_crypto() needs to return -EINPROGRESS on success to indicate
> asynchronous processing to the crypto API. This also means it must not
> return the errno code returned by hifn_process_queue(), if any.
Then how to indicate that error occured?
What if we will return -EINPROGRESS in case of success and negative
errno otherwise?
> @@ -2202,9 +2202,9 @@ static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op,
> return err;
>
> if (dev->started < HIFN_QUEUE_LENGTH && dev->queue.qlen)
> - err = hifn_process_queue(dev);
> + hifn_process_queue(dev);
>
> - return err;
> + return -EINPROGRESS;
Thus we are not able to return any error at all...
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:15:38PM +0200, Patrick McHardy ([email protected]) wrote:
> [HIFN]: Remove printk_ratelimit() for debugging printk
>
> Without debugging this spams the log with "printk: N messages surpressed"
> without any actual messages on error. With debugging its more useful to
> always see the message.
>
> Signed-off-by: Patrick McHardy <[email protected]>
Acked :)
--
Evgeniy Polyakov
I'm not entirely sure about the alignmask change at the end of
this patch, is an alignmask of 1 correct if no source buffer
alignment is required, but the destination buffer should be
(doesn't have to be though) 4 byte aligned?
Evgeniy Polyakov wrote:
> Hi.
>
> On Wed, May 07, 2008 at 02:19:36PM +0200, Patrick McHardy ([email protected]) wrote:
>
>> hifn_setup_crypto() needs to return -EINPROGRESS on success to indicate
>> asynchronous processing to the crypto API. This also means it must not
>> return the errno code returned by hifn_process_queue(), if any.
>>
>
> Then how to indicate that error occured?
> What if we will return -EINPROGRESS in case of success and negative
> errno otherwise?
>
The completion handler should be called with the error code,
a later patch of mine will change that. Returning it while
enqueing a new request is wrong since the error affects a
different request. Queue processing should happen on completion
of requests anyway to make sure the queue is kept running
while requests are queued (and avoid reordering).
>
>> @@ -2202,9 +2202,9 @@ static int hifn_setup_crypto(struct ablkcipher_request *req, u8 op,
>> return err;
>>
>> if (dev->started < HIFN_QUEUE_LENGTH && dev->queue.qlen)
>> - err = hifn_process_queue(dev);
>> + hifn_process_queue(dev);
>>
>> - return err;
>> + return -EINPROGRESS;
>>
>
> Thus we are not able to return any error at all...
>
We are, errors affecting *this request* are returned as they should be.
Two questions regarding this patch:
- do OFB and CFB also need ivsize initialization?
- the HIFN documentation doesn't mention OFB and CFB (the
values are marked reserved), do they actually work?
On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy ([email protected]) wrote:
> I'm not entirely sure about the alignmask change at the end of
> this patch, is an alignmask of 1 correct if no source buffer
> alignment is required, but the destination buffer should be
> (doesn't have to be though) 4 byte aligned?
> commit f76618d53e82c8905214e889a3f79f1816c680fb
> Author: Patrick McHardy <[email protected]>
> Date: Wed May 7 12:44:15 2008 +0200
>
> [HIFN]: Fix data alignment checks
>
> The check for misalignment of the scatterlist data has two bugs:
>
> - the source buffer doesn't need to be aligned at all
> - the destination buffer and its size needs to be aligned to a multiple
> of 4, not to the crypto alg blocksize
>
If memory serves me right, both src and dst addresses have to be 4 bytes
aligned. Protocol alignment is not needed.
If it is not the issue, then I have no objections.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:20:34PM +0200, Patrick McHardy ([email protected]) wrote:
> commit 7e24c849df30a5d2b0bb89165b933ea2faa747bd
> Author: Patrick McHardy <[email protected]>
> Date: Wed May 7 12:43:20 2008 +0200
>
> [HIFN]: Handle ablkcipher_walk errors
>
> ablkcipher_walk may return a negative error value, handle this properly
> instead of treating it as a huge number of scatter-gather elements.
>
> Signed-off-by: Patrick McHardy <[email protected]>
Ooops, ACK :)
--
Evgeniy Polyakov
Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy ([email protected]) wrote:
>
>> I'm not entirely sure about the alignmask change at the end of
>> this patch, is an alignmask of 1 correct if no source buffer
>> alignment is required, but the destination buffer should be
>> (doesn't have to be though) 4 byte aligned?
>>
>
>
>> commit f76618d53e82c8905214e889a3f79f1816c680fb
>> Author: Patrick McHardy <[email protected]>
>> Date: Wed May 7 12:44:15 2008 +0200
>>
>> [HIFN]: Fix data alignment checks
>>
>> The check for misalignment of the scatterlist data has two bugs:
>>
>> - the source buffer doesn't need to be aligned at all
>> - the destination buffer and its size needs to be aligned to a multiple
>> of 4, not to the crypto alg blocksize
>>
>>
>
> If memory serves me right, both src and dst addresses have to be 4 bytes
> aligned. Protocol alignment is not needed.
>
> If it is not the issue, then I have no objections.
>
Of the data buffers only the destination buffer needs to be aligned,
see the Source Pointer description in "2.2 Source Descriptors" in
the HIFN documentation.
On Wed, May 07, 2008 at 02:38:31PM +0200, Patrick McHardy ([email protected]) wrote:
>
> commit 21b18f9b22c7aa0a458c3f93fc1771a5eb5e70c8
> Author: Patrick McHardy <[email protected]>
> Date: Wed May 7 13:00:10 2008 +0200
>
> [HIFN]: Fix max queue length value
>
> All but the last element of the command and result descriptor rings can be
> used for crypto requests, fix HIFN_QUEUE_LENGTH.
>
> Signed-off-by: Patrick McHardy <[email protected]>
Yaah, that's right, we can also wrap it into (), although current usage
is ok.
Thanks Patric.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy wrote:
> I'm not entirely sure about the alignmask change at the end of
> this patch, is an alignmask of 1 correct if no source buffer
If no alignment is required you want 0, 1 means 2-byte aligned.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Note: the DMA setup fixes mentioned in the changelog still
need a bit of work, so I'm not sure if I'll manage to send
them today. This one is fine for review or applying anyway.
Herbert Xu wrote:
> On Wed, May 07, 2008 at 02:26:30PM +0200, Patrick McHardy wrote:
>
>> I'm not entirely sure about the alignmask change at the end of
>> this patch, is an alignmask of 1 correct if no source buffer
>>
>
> If no alignment is required you want 0, 1 means 2-byte aligned.
>
Thanks, fixed in the patch below.
On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy ([email protected]) wrote:
> [HIFN]: Endianess fixes
>
> HIFN uses little-endian by default, move cpu_to_le32 conversion to hifn_write_0/
> hifn_write_1, add sparse annotations and fix an invalid endian conversion in
> hifn_setup_src_desc.
We can also be safe without volatile annotations.
Ack, thanks Patrick.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:30:28PM +0200, Patrick McHardy ([email protected]) wrote:
> [HIFN]: Properly handle requests for less than the full scatterlist
>
> The scatterlist may contain more data than the crypto request, causing
> an underflow of the remaining byte count while walking the list.
>
> Use the minimum of the scatterlist element size and the remaining byte
> count specified in the crypto request to avoid this.
>
> Signed-off-by: Patrick McHardy <[email protected]>
That one is the nastiest! Thanks a lot Patrick for fixing that.
Ack of course.
--
Evgeniy Polyakov
Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy ([email protected]) wrote:
>
>> [HIFN]: Endianess fixes
>>
>> HIFN uses little-endian by default, move cpu_to_le32 conversion to hifn_write_0/
>> hifn_write_1, add sparse annotations and fix an invalid endian conversion in
>> hifn_setup_src_desc.
>>
>
> We can also be safe without volatile annotations.
>
Yes, will also be taken care of by a later patch :)
On Wed, May 07, 2008 at 02:45:15PM +0200, Patrick McHardy ([email protected]) wrote:
> Of the data buffers only the destination buffer needs to be aligned,
> see the Source Pointer description in "2.2 Source Descriptors" in
> the HIFN documentation.
Ok, thanks for the reference.
--
Evgeniy Polyakov
Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:45:15PM +0200, Patrick McHardy ([email protected]) wrote:
>> Of the data buffers only the destination buffer needs to be aligned,
>> see the Source Pointer description in "2.2 Source Descriptors" in
>> the HIFN documentation.
>
> Ok, thanks for the reference.
As a side-note: its also not just an overseight in the documentation,
not aligning the destination buffer to a multiple of 4 results in the
HW freezing, the source buffer alignment changes work fine though.
On Wed, May 07, 2008 at 02:32:45PM +0200, Patrick McHardy ([email protected]) wrote:
> [HIFN]: Use unique driver names for different algos
>
> When the CryptoAPI instantiates a new algorithm, it performs a lookup
> by driver name. Since hifn uses the same name for all modes of one
> algorithm, the lookup may return an incorrect algorithm.
>
> Change the name to use <mode>-<algo>-<devicename> to provide unique
> names for the different combinations and devices.
>
> Signed-off-by: Patrick McHardy <[email protected]>
Ack, thanks Patrick.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:50:29PM +0200, Patrick McHardy ([email protected]) wrote:
> commit 163c74fc4f18d086bbb7f114d55bc7dac939d6b7
> Author: Patrick McHardy <[email protected]>
> Date: Wed May 7 13:11:59 2008 +0200
>
> [HIFN]: Move command descriptor setup to seperate function
>
> Move command descriptor setup to seperate function as preparation
> for the following DMA setup fixes.
>
> Note 1: also fix a harmless typo while moving it: sa_idx is initialized
> to dma->resi instead of dma->cmdi.
Yep, resource and command indexes should be always equal.
> Note 2: errors from command descriptor setup are not propagated back,
> anymore, they can't be handled anyway and all conditions leading
> to errors should be checked earlier.
>
> Signed-off-by: Patrick McHardy <[email protected]>
I have no objections, thanks Patrick.
Ack.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:53:17PM +0200, Patrick McHardy ([email protected]) wrote:
> The descriptors need to be invalidated after processing for ring
> cleanup to work properly and to avoid using an old destination
> descriptor when the src and cmd descriptors are already set up
> and the dst descriptor isn't.
>
> Signed-off-by: Patrick McHardy <[email protected]>
Well, we only care about cmd register and do not process valid bit in
others, instead of cmd is not valid we do not use others, but we can
also check other descriptors too.
Thanks Patrick, I have no objections.
Ack.
--
Evgeniy Polyakov
Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 02:53:17PM +0200, Patrick McHardy ([email protected]) wrote:
>> The descriptors need to be invalidated after processing for ring
>> cleanup to work properly and to avoid using an old destination
>> descriptor when the src and cmd descriptors are already set up
>> and the dst descriptor isn't.
>>
>> Signed-off-by: Patrick McHardy <[email protected]>
>
> Well, we only care about cmd register and do not process valid bit in
> others, instead of cmd is not valid we do not use others, but we can
> also check other descriptors too.
I noticed the ring cleanup debugging printing ever increasing
use counts for src and dst descriptors, which went away with
this patch, but the more important point is avoiding that the
HW accidentally uses an old descriptors of course.
> Thanks Patrick, I have no objections.
Thanks Evgeniy. Thats it for now, next round will take until
tonight or maybe tommorrow.
On Wed, May 07, 2008 at 03:05:48PM +0200, Patrick McHardy ([email protected]) wrote:
> As a side-note: its also not just an overseight in the documentation,
> not aligning the destination buffer to a multiple of 4 results in the
> HW freezing, the source buffer alignment changes work fine though.
Probably I just mirrored dst to src alignment and thus forced mask to be
at least 3.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 03:01:55PM +0200, Patrick McHardy ([email protected]) wrote:
> >We can also be safe without volatile annotations.
> >
>
> Yes, will also be taken care of by a later patch :)
I just receive them in a strange order and do not know when others will
arrive :)
--
Evgeniy Polyakov
Evgeniy Polyakov wrote:
> On Wed, May 07, 2008 at 03:01:55PM +0200, Patrick McHardy ([email protected]) wrote:
>>> We can also be safe without volatile annotations.
>>>
>> Yes, will also be taken care of by a later patch :)
>
> I just receive them in a strange order and do not know when others will
> arrive :)
Appologies for imperfect ordering :) The patchset grew while fixing
issues as I ran into them and attempts to reorder caused too many
conflicts.
The removal of volatile doesn't belong in this patch though.
On Wed, May 07, 2008 at 03:46:12PM +0200, Patrick McHardy ([email protected]) wrote:
> Appologies for imperfect ordering :) The patchset grew while fixing
> issues as I ran into them and attempts to reorder caused too many
> conflicts.
I believe it is more to mail system, which delivers 5 before 1 and so
on, not how patches are organized in the patchset :)
> The removal of volatile doesn't belong in this patch though.
Ok, no problem.
--
Evgeniy Polyakov
On Wed, May 07, 2008 at 02:14:28PM +0200, Patrick McHardy wrote:
> Attached is the first of my fixes for the HIFN driver. I've
> got it to a working state with tcrypt, IPsec and dm-crypt, a
> few of the patches still need a bit work though, so I'll send
> the ones that I already consider ready one by one.
All 11 patches applied. Thanks a lot Patrick!
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt